fix(player): keep the parent tick clock alive in a cross-origin iframe - #3554
fix(player): keep the parent tick clock alive in a cross-origin iframe#3554miguel-heygen wants to merge 3 commits into
Conversation
A composition in a cross-origin iframe froze a few seconds in while the player still reported `paused === false`. Three separate things had to be true for it to play, and none of them were. The parent tick clock exists to drive a composition whose own rAF Chromium has throttled, which is the normal state of a cross-origin frame. It was being torn down before it delivered a single tick: - The rAF loop re-read `_paused` every frame and returned permanently on the first `true`. That field has a second writer: the runtime's "state" message, which reports the iframe's playback as of the moment it was posted. A state message already in flight when `play()` runs carries the pre-play `isPlaying: false` and lands a frame later, ending the loop for good. The loop now runs between its explicit start and stop and nothing else decides its lifetime; the end of the timeline, which is the one stop the runtime initiates on its own, is relayed to it explicitly. - The iframe `load` handler reset readiness and cancelled the clock. `load` is the end of subresource fetching, not the start of a document, and the runtime announces its timeline on DOMContentLoaded — so on a composition still fetching images or fonts it arrives after the player is already driving that same document. A new document is always preceded by a `src`/`srcdoc` assignment, which clears `_ready` first, so a load seen while ready has nothing to reset. - With the clock alive, the runtime advanced the timeline but never told the embedder where it had got to: the parent-driven tick seeked but did not post state, so the playhead, `timeupdate` and end-of-playback detection stayed frozen at the last frame the (now throttled) local transport managed to post. The reported runaway on a second, late `play()` was a consequence of the above rather than a clock bug: `TransportClock` is wall-clock and `play()` is idempotent, so time kept running correctly while the starved frame fell behind, and the first tick delivered afterwards caught up in one step. With the clock running from the first `play()` there is nothing to catch up. Verified in Chrome against a runtime-injected composition served same-origin, under `Content-Security-Policy: sandbox allow-scripts`, from a genuinely cross-origin host, and with the cross-origin frame scrolled out of the viewport so Chromium suspends its rAF. All four now hold 1x for the full 15s and end on time; the throttled one previously sat at the same frame for the whole run. The same-origin direct-timeline path is untouched.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Requesting changes. The core move is right: the tick loop had two owners deciding its lifetime and this gives it one, which is the correct fix rather than a special case for the racing message. Both findings below are about the completeness claims that the single-owner shape now rests on, since removing the _paused read means an exit path that does not call _stopParentTickClock no longer has a backstop.
Blocker: _reloadShaderOptions reloads the iframe without clearing _ready, so the new early return skips the handshake
The added comment states the invariant the early return depends on:
A genuinely new document is always preceded by assigning
src/srcdoc, which clears_readyfirst.
That holds for the two attributeChangedCallback branches, which set _ready = false at :210 and :216 before assigning at :212 and :218. It does not hold for _reloadShaderOptions at :643-651, which assigns this.iframe.srcdoc at :646 or this.iframe.src at :650 and never touches _ready. That method is reached from attributeChangedCallback at :271-273 on SHADER_CAPTURE_SCALE_ATTR and SHADER_LOADING_ATTR.
So changing either shader attribute on a ready player loads a genuinely new document while _ready is still true, and if (this._ready) return; then skips the entire teardown for it: _runtimeBridgeReady stays true against a document that never handshook, _directTimelineAdapter keeps pointing at the previous timeline, _media.resetForIframeLoad() does not run, and probe.start() never fires, so the new document has no path to ready at all.
Before this change _onIframeLoad unconditionally ran that teardown, so the shader-option reload path worked. The fix is to clear _ready inside _reloadShaderOptions before assigning, matching what the src/srcdoc branches already do, with a regression test that flips a shader attribute on a ready player and asserts the probe restarts.
Gap: a runtime-initiated pause before the end of the timeline leaves the clock running
stopPlaybackClock() is called only under completedPlayback, and that predicate requires currentTime >= current.duration (playback-state.ts:53). Any state message reporting isPlaying: false at a position short of the duration therefore writes _paused = true through setPlaybackState (hyperframes-player.ts:745) while leaving _parentTickRaf scheduled.
The docstring's enumeration says every transition out of playback goes through pause(), seek(), a new document, disconnect, or the end of the timeline. I checked each: pause() (:327), seek() (:351) and disconnectedCallback (:194) all stop the clock before setting _paused, and end-of-timeline is covered by the new stopPlaybackClock relay. The path not in that list is the runtime pausing itself mid-timeline, which is reachable through the runtime player seam: packages/core/src/runtime/player.ts calls deps.setIsPlaying(false) from its own pause() at :198 and from the seek helpers at :230 and :253, and that surface is exposed to composition code as window.__player.
The old if (this._paused) read in the rAF was the accidental backstop for exactly this, so removing it is what exposes the gap. Severity depends on a divergence worth confirming: onTick guards on clock.isPlaying() (init.ts:3366) while setIsPlaying only writes state.isPlaying without pausing the transport. If the transport really did stop, this is a leaked rAF posting ticks that early-return, so it is wasted work rather than a visible fault. If the two disagree, the composition keeps advancing while the player reports paused === true, which is the mirror image of the bug being fixed here.
I do not think this needs to grow the fix. The cheapest close is to relay the stop on any observed transition to not-playing rather than only on completedPlayback, since the clock is idempotent to stop and play() restarts it. Whichever way it goes, the docstring's list should name the runtime seam so the next reader does not have to re-derive it, because that list is now load-bearing rather than descriptive.
What is clean
The three new player tests pin the right things: the in-flight pre-play state message, the late load event, and the end-of-timeline stop asserted through _parentTickRaf being null rather than through a proxy. The init.ts change to post state on a parent-driven tick is necessary and its placement after the reachedEnd early return is correct, so the end-of-playback message still wins. Verified that postState's existing change and interval filter (init.ts:2172-2178) does bound the added message rate as the comment claims.
One note on the docstring at :707: it says a stale message would end the loop "while the player still reports paused === false". As written the message does write _paused = true through the same handler, so the player reports paused too. It self-corrects on the next tick's state message, so nothing turns on it, but the parenthetical describes a state that does not occur.
Review by Rames
…untime pause Review follow-up on two holes in the previous commit. The `_onIframeLoad` early return rests on "a new document is always preceded by assigning src/srcdoc, which clears readiness first". That was true of the two attributeChangedCallback branches and false of `_reloadShaderOptions`, which reassigns the document on a shader-option change without touching readiness. Flipping `shader-loading` or `shader-capture-scale` on a ready player therefore loaded a genuinely new document that the load handler then skipped the teardown for, leaving a stale bridge flag and no probe running, so it had no path to ready at all. All six reassignment sites now route through `_setIframeSrc` / `_setIframeSrcdoc`, which own clearing readiness, so the load handler's test holds by construction rather than by convention. Collapsing the tick loop to a single owner also removed an accidental backstop. `_paused` was gating the loop, so a runtime that stopped itself short of the end used to end it; the replacement only covered end-of-timeline, which needs `currentTime >= duration`. A composition calling `pause()` on `window.__player` left the loop scheduled forever. That is a leaked rAF and not a divergence: `createRuntimePlayer` returns its transport-backed object whenever a transport is passed, and the runtime always passes one, so the `setIsPlaying` seam that writes `state.isPlaying` without touching the clock is unreachable there. Every `state.isPlaying = false` in the runtime sits in the transport beside a `clock.pause()`, so the composition cannot advance while the player reports paused. The fix covers the whole class instead of the one instance: the raw `isPlaying` from the wire is forwarded to the player, which stops the clock on any runtime-reported stop. A report seen before the runtime has echoed our own play is ignored, since postMessage delivery between two windows is ordered and such a report was posted before the play arrived — that is what keeps the original stale-message immunity intact. All four playback configurations re-measured and unchanged: 1x throughout, ended at ~14.99s of a 15s composition, clock started once and stopped once.
|
Both findings verified at source and fixed in Blocker:
|
| reverted | fails with |
|---|---|
the _paused read removal |
expected +0 to be 2 |
the load early return |
expected false to be true |
_reloadShaderOptions routing |
expected true to be false |
| the runtime-report stop | expected 1 to be null (×2 tests) |
| the staleness guard | expected +0 to be 2 |
postState in the runtime tick |
frame 60 → 0 |
The last row of that table is the one worth noting: removing the staleness guard reopens the original bug, so the guard is load-bearing in both directions.
One test was corrected rather than kept: the end-of-timeline case previously jumped from play() straight to the end report with no intervening playing report, which the runtime cannot produce — 15s of playback always echoes. Assertion unchanged, construction now matches production. I also wrote a loop-wrap test, could not make it fail under any realistic mutation, and deleted it rather than ship a vacuous one.
All four configurations re-measured after the change, unchanged:
| config | t+2000 | t+5000 | t+8000 | t+14500 | end |
|---|---|---|---|---|---|
| same-origin | 2.000 | 4.967 | 8.000 | 14.500 | 15.000, ended 14996ms |
CSP sandbox allow-scripts |
2.000 | 5.000 | 8.000 | 14.500 | 15.000, ended 15001ms |
| cross-origin host | 2.000 | 4.967 | 7.967 | 14.500 | 15.000, ended 14991ms |
| cross-origin + offscreen (rAF suspended) | 2.000 | 5.000 | 8.000 | 14.500 | 15.000, ended 14988ms |
readyEvents: 1 in all four; clock started exactly once and stopped exactly once per run.
@hyperframes/player 350 passed, @hyperframes/core 2580 passed, repo-wide typecheck / lint / format clean.
One thing I did not change
The no-transport fallback in createRuntimePlayer is dead from the runtime's entry point — every deps.setIsPlaying, deps.onSyncMedia, deps.onDeterministicPause path is unreachable via initSandboxRuntimeModular, exercised only by player.test.ts and one script. That is a deletion worth making, but it is a separate change and not this PR's job.
Review follow-up. The tick-clock docstring said a stale pre-play `state` message ends the loop "while the player still reports `paused === false`". That message runs through `applyRuntimeStateMessage` like any other, which writes `paused = !data.isPlaying`, so the player reports paused at that instant and the parenthetical described a state that never occurs. The reported symptom is real, it is just one report later: nothing actually paused the composition, so the runtime's next state message carries `isPlaying: true` and puts `paused` back to `false`, against a timeline the now-dead loop is no longer advancing. Say that instead of the instant.
|
Third point from the review addressed in The docstring parentheticalYou were right that it describes a state that does not occur, and the reason is exactly the one you gave: the stale message runs through Rather than delete the clause I made it say the thing that is true one report later. Nothing actually paused the composition, so the runtime's next Re-verified by breaking each fix on purposeEach fix reverted alone, on the current head, witness test named and failure quoted verbatim.
The runtime-report stop removed ( Two tests, which is the point of the second finding: end-of-timeline was one instance of the class, and the mid-timeline runtime pause is another. The staleness discriminator removed (stop on any not-playing report, no That last one is the one worth keeping in view: the guard is load-bearing in both directions. Without it the fix for the second finding reopens the original bug. SuitesFormat, lint and typecheck clean on the touched package. CI on the branch is green apart from jobs still in flight at the time of writing. Re-requesting review. |
2 participants
A composition in a cross-origin iframe froze a few seconds in while the player still reported
paused === false. The parent tick clock is what should have kept it moving, and three separate things stopped it.The parent tick clock
It exists to drive a composition whose own rAF Chromium has throttled, which is the normal state of a cross-origin frame. It was being torn down before it delivered a single tick.
Its lifetime had two owners. The rAF loop re-read
_pausedevery frame and returned permanently on the firsttrueit saw. That field has a second writer: the runtime'sstatemessage, which reports the iframe's playback as of the moment it was posted. A state message already in flight whenplay()runs carries the pre-playisPlaying: false, lands a frame later, and ends the loop for good — there is no restart path. The loop now runs between its explicit start and stop and nothing else decides its lifetime. Reaching the end of the timeline is the one playback stop the runtime initiates on its own, so that one is relayed to the clock explicitly.The
loadhandler tore down a document that was already playing.loadmarks the end of a document's subresource fetching, not the start of a document, and the runtime announces its timeline onDOMContentLoaded— so on a composition still fetching images or fonts it arrives after the player has gone ready and started driving that same document, and cancelled the clock aplay()in thereadyhandler had just started. A genuinely new document is always preceded by assigningsrc/srcdoc, which clears_readyfirst, so a load seen while ready has nothing to reset.The tick advanced the timeline without reporting it. With the clock alive, the runtime seeked on each parent tick but never posted state, so the embedder's playhead,
timeupdateand end-of-playback detection stayed frozen at the last frame the (now throttled) local transport managed to post. The parent drives this tick precisely when nothing else will report the position, so it now posts state like the local transport does.postState's own change/interval filter keeps the message rate unchanged.On the reported runaway
A second, late
play()was reported to jump the playhead forward at many times real speed. That is a consequence of the above rather than a clock bug:TransportClockis wall-clock and itsplay()is idempotent, so time kept running correctly while the starved frame fell behind, and the first tick delivered afterwards caught up in one step. With the clock running from the firstplay()there is nothing to catch up, and playback holds 1x throughout.Verification
Driven in Chrome against a composition with the runtime injected,
play()issued from inside thereadyhandler, in four configurations:Content-Security-Policy: sandbox allow-scriptsreadydispatched twicereadyoncecurrentTimestuck at 0 for the whole run withpaused === falseendedat 14.99s of a 15s compositioncurrentTimeafter the change tracks wall clock to the millisecond in all four (2.000 / 5.000 / 8.000 / 14.500 at 2000 / 5000 / 8000 / 14500 ms, then 15.000 and paused). The tick clock is stopped exactly once per run, at the end of the timeline. The same-origin direct-timeline path is untouched.Three unit regressions cover the player side and one covers the runtime side; each was confirmed to fail with only its own fix reverted.
bun run --filter @hyperframes/player test348 passed,bun run --filter @hyperframes/core test2580 passed,bun run --filter '*' typecheckandbun run lintclean.