Fix Tunnel.toIframe breaking on a duplicate same-key handshake offer (SITES-48958) - #151
Merged
Merged
Conversation
…d tunnel Adds a #/duplicate-offer host scenario where a guest's real first handshake offer is captured and replayed verbatim, from inside the guest's own realm, ~800ms after the connection has already succeeded - simulating Tunnel.toParent's 100ms retry loop firing once more before noticing its own tunnel had connected. Tunnel.toIframe's offerListener (after the SITES-42495 fix removed the !tunnel.isConnected guard) reprocesses that duplicate: it opens a second MessageChannel and calls tunnel.connect() again, closing the port the guest is still using and replacing it with one paired to a port the guest already stopped listening for. Both sides report isConnected, but neither can reach the other - a silent, permanent hang with no error or timeout, since the initial connection timeout was already cleared on first connect. The test asserts the correct behavior (a post-connect RPC call still succeeds) and currently fails with "Calling host.probe.ping() timed out after 10000ms". A second test with duplicate injection disabled (?inject=0) is a positive control, proving the harness itself is sound. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Iframe The SITES-42495 fix removed the !tunnel.isConnected guard in offerListener so a genuinely reloaded guest (a new Tunnel.toParent instance, with a new key) could reconnect. But it also removed protection against a *still-connecting* guest re-sending its offer: Tunnel.toParent's sendOffer retries every 100ms until the guest's own tunnel reports connected, which only happens after it processes this host's "accepted" reply - an async round trip. If that takes longer than 100ms (plausible with several GuestUIFrame instances of the same extension initializing concurrently and contending for main-thread time), a second, redundant offer carrying the *same* key arrives after the host already connected, gets reprocessed, and tears down the working MessagePort in favor of one the guest already stopped listening for - both sides report connected while unable to reach each other, hanging forever with no error (SITES-48958). Track the key of the offer last accepted and only treat a *different* key as a fresh connection attempt (an actual reload); a repeat of the same key while already connected is now ignored as a retry echo. Verified against e2e/tests/tests/duplicate-offer-race.js, which now passes without any changes to the test itself, and against multifield-reorder.js (SITES-42495), which still passes - confirming this doesn't regress the original reconnect-after-reload fix. Full suite: 17/17 e2e, 83/85 unit (2 pre-existing skips), no regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Rename the offer's destructured `offers` to `offerKey` in offerListener - reads more clearly at the equality check site than the wire-protocol field name does as a local variable. - Set `acceptedOfferId` only after tunnel.connect() succeeds, so it unambiguously represents the key of the active connection rather than merely the last offer seen, and clear it in cleanup() for explicit lifecycle intent (not currently load-bearing - cleanup already removes offerListener from window - but this function has now caused two regressions from two different people modifying it, so the extra clarity is worth it). - Add a focused unit test for the early-return guard itself, dispatching a manually-constructed MessageEvent with an explicit `source` instead of a real window.postMessage() call, which sidesteps the jsdom limitation that keeps the neighboring describe.skip block skipped (jsdom's postMessage still doesn't populate event.source correctly, confirmed against jsdom's current source - but MessageEvent's own constructor handles source/origin/ports correctly, so dispatching one directly bypasses the broken code path entirely). This also surfaced and fixes a latent bug in the FakeIframe test helper: contentWindow's paired port was discarded, so nothing could ever observe messages the SDK sent via contentWindow.postMessage() - exposed as guestSidePort so tests can listen on it. Verified: npm run test:unit (15/15 suites, 84 passed [+1], 2 pre-existing skips) and npm run test:e2e (17/17 passing, including both the SITES-42495 and SITES-48958 scenarios). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A regression introduced by the SITES-42495 fix (#150):
Tunnel.toIframecould silently break an already-established, healthy connection if the
guest's own handshake retry loop sent one more redundant offer before it
noticed its own tunnel had connected. This PR adds an e2e test that
reproduces the failure deterministically and a small fix that restores
protection against it without reintroducing SITES-42495.
Related Issue
Reported internally as SITES-48958 (Adobe internal Jira - Content Fragment
Editor customer escalation, Casio): https://jira.corp.adobe.com/browse/SITES-48958
No public GitHub issue exists for this - it came in through an internal
customer support ticket, same as SITES-42495.
Motivation and Context
uix-sdk1.1.10(containing both #148 and #150) shipped to CF EditorPROD
v1.58.0on July 30. That same day, a customer (Casio) startedreporting Content Fragment custom fields intermittently stuck on "Loading
custom field..." forever - some fields on a page would render, others
wouldn't, with no error surfaced anywhere.
Root cause:
Tunnel.toIframe'sofferListener, fixed in #150 to stoppermanently ignoring every offer once connected (so a genuinely reloaded
guest could reconnect), had no way to distinguish that legitimate case from
a much more mundane one - a guest that hasn't reloaded at all, just hasn't
finished processing the host's "accepted" reply yet:
Tunnel.toParent'ssendOfferretries every 100ms, using the samekey, until the guest's own tunnel reports connected - which only
happens after it receives and processes the host's "accepted" message,
an async round trip.
GuestUIFrameinstances of the same extension are initializingconcurrently on one CF Editor page, all competing for main-thread time -
a second, redundant offer with the same key arrives after the host has
already connected.
MessageChannel,posts a new port to the guest, and calls
tunnel.connect()again -closing the port pair the guest is actually using and replacing it with
one paired to a port the guest already stopped listening for (its
acceptListenerunsubscribes right after the first accept).isConnected: true, but neither can reach theother. Nothing throws and nothing times out (the one-shot connection
timeout was already cleared on the first successful connect) - a silent,
permanent hang. That matches the "stuck loading" screenshots in the
ticket exactly, and being a timing race, it only bites some fields on
some page loads, matching "intermittent."
The fix
Track the
offerskey of the handshake the tunnel is currently connectedwith (
acceptedOfferId). A repeat of that same key while alreadyconnected is now ignored as a retry echo. A different key - a genuinely
new
Tunnel.toParentinstance, i.e. an actual reload - is still treated asa fresh connection attempt, preserving the SITES-42495 behavior.
How Has This Been Tested?
Added
e2e/tests/tests/duplicate-offer-race.jsplus supporting fixtures(
e2e/host-app/src/HostAppDuplicateOffer.jsx,e2e/guest-app/src/DuplicateOfferProbe.jsx, routing wire-up in both apps'App.js). The scenario can't rely on incidental timing jitter to bereliable in CI, so it forces the race deterministically: the guest
monkey-patches its own
window.parent.postMessageto capture its realfirst handshake offer and replay the identical message, from inside its
own realm (so
event.sourcegenuinely reflects the guest, not the host),~800ms after the connection has already succeeded. The guest then makes a
real post-connect RPC call (
host.probe.ping()) exposed viaGuestUIFrame'sprivateMethods."Calling host.probe.ping() timed out after 10000ms"(the SDK's own RPC-call timeout), since the tunnel wassilently swapped for a dead port pair.
?inject=0) is apositive control, confirming the harness itself is sound (ping succeeds
immediately absent any interference).
multifield-reorder.js(SITES-42495) still passes unmodified, confirmingthis doesn't regress the original reconnect-after-reload fix.
npm run test:e2e: 17/17 passing, noregressions to any of the 15 pre-existing e2e tests.
npm run test:unit: 15 suites, 83 passed, 2 pre-existing skips (thedocumented JSDOM
postMessage/MessageEventlimitation onTunnel.toIframe's own describe block) - no change from before this PR.Types of changes
Checklist
restores behavior consistent with
Tunnel.toIframe's existing doccomment; no public API changed.)
Investigated and authored with Claude Code.