Repository navigation
fix(engine): a background image that never finishes decoding no longer stalls the frame - #6972
miguel-heygen wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Approving at 09ec50f3. I found no blockers.
Verified locally: I installed the workspace, built parsers and core, and ran frameCapture-cssBackgroundReady.test.ts. At the head, all 7 tests pass. With frameCapture.ts restored to the base and only the new BACKGROUND_DECODE_TIMEOUT_MS export added, stops waiting for a url whose decode never settles and names it fails ("Test timed out in 5000ms") and the other 6 pass. The fails without the fix: line holds.
Logic checks:
- Each decode races a 15 s timer, and the timer is cleared in
finally. A decode that rejects after losing the race is still handled, becausePromise.racesubscribed to it, so there's no unhandled rejection. - The page-local decoder reads
__hf_css_background_timeout_mswhen called. That value is always set before the firstdecode(), because installation happens only through this function. So the?? 0fallback, which would time out every image immediately, can't be hit. - Timed-out urls go into
__hf_css_background_timed_outand are skipped on later frames. The batch drawElement loop (drawElementService.ts:1109) gets the same limit.
Non-blocking, follow-ups if you want them:
finalizeDrawElementInit(frameCapture.ts:1382) discards the returned list. A url that times out during that init call goes into the timed-out set and is skipped from then on, so it is never written to diagnostics. That's a small gap in "its url is reported". The batch loop also drops the in-page return value, so timeouts on that path aren't named either.drawElementService.ts:1073still types__hfDecodeDynamicCssBackgroundImagesas() => Promise<void>. That's harmless because the result is ignored, but it now differs from the producer's type.- As the body says, no real Chromium run with a stuck decode was done.
Reviewed by Genesis V2 on behalf of @miguel-heygen.
Edit accuracy: accurate 271 (base branch 271), smooth 121 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (2)
|
terencecho
left a comment
There was a problem hiding this comment.
Reviewed current #6972 head 28d6abb4d46e18b348971e4ad4411db6bc15c36d against 9fa02badfc20319a292573333c72da715c263bcf, including both PR commits rather than carrying forward the approval at 09ec50f3.
The decoder starts distinct URLs concurrently, clears each timer in finally, leaves ordinary decode rejection retryable, and handles late rejection through the existing Promise.race subscription (packages/engine/src/services/frameCapture.ts:531-557). The required named fails without the fix: line is present; its never-settling mocked decode is a valid static witness for the new bounded helper, not real Chromium/batch verification.
Blocker [P2] — sanitize the new timeout diagnostic before retaining it. packages/engine/src/services/frameCapture.ts:3324 interpolates the raw background URL, bypassing the same file's sanitizeDiagnosticUrl (:655-678) used by request/navigation/HTTP diagnostics. If an inline asset URL containing authentication/query components times out and capture subsequently fails while the line is retained, appendBrowserDiagnostic (:570-574) preserves it and captureFrameErrorDiagnostics (:3231-3244) writes it verbatim into browserConsoleTail in frame-error JSON. This is a newly unsanitized diagnostic representation; external telemetry/publication exposure is not established or claimed. Sanitize at the text sink while keeping the original decode/cache identity, and add a timeout-diagnostic redaction test. Existing authored HTML snapshots do not make the new URL-diagnostic sink follow the redaction policy.
Important — timeout names are lost on initialization and batch capture. As Genesis already noted at the earlier head, finalizeDrawElementInit (frameCapture.ts:1382) and the in-page batch loop (drawElementService.ts:1109) discard the returned URL array. The new permanent timed-out set (frameCapture.ts:527,547) prevents later normal frames from rediscovering that event, so a successfully degraded render on those paths can never name it through the new diagnostic. Preserve a sanitized timeout event at every caller, preferably in shared structured render warnings; the test's returned-list assertion and batch ordering regex do not cover this. This is credited prior observability feedback, not a newly discovered duplicate finding or the reason for the security blocker above.
Boundaries, not additional proved regressions: the limit is per decoder invocation/URL, not end-to-end frame/render. Existing outer 15s drawElement, prestarted 30s batch encode and 60s no-progress budgets can win; distinct later seeks/motion-blur samples can accumulate waits. The implementation captures the DOM as it stands—it does not remove an image or guarantee it never appears after late decoding. I have not proved a newly broken formerly-successful base render, deterministic fallback pixels, or universal continuation in those modes. The PR description's broader claims should be qualified.
Scope/evidence: complete changed helper/test and real init/main/batch/cache/diagnostic/watchdog integration paths were inspected; a fresh source adversary also completed. The large frame-capture/producer integrations were bounded in the primary recovery, not a claimed end-to-end whole-repository audit. The original primary failed; its completion was not adopted. Automatic memory-index/context exposure is disclosed. No tests, replay, browser/native/render or CI execution was performed; old peer runtime claims are not tai evidence. The current successor only adds nullish-list tolerance; that does not repair the diagnostic sink. Check color is not the basis of this verdict. No merge action.
— tai
Verdict: REQUEST CHANGES
Reasoning: The new raw-URL timeout diagnostic bypasses the established sanitizer and can persist sensitive URL components in frame-error JSON. The bounded-wait implementation does not require abandoning that redaction invariant.
…r stalls the frame
28d6abb to
dcc3da7
Compare
There was a problem hiding this comment.
Approving at dcc3da71 after the rebase onto #6984. I found no blockers.
What changed since 09ec50f3: I compared the two patches. The only content difference is one line in prepareFrameForCapture: the loop now reads (await decodeDynamicCssBackgroundImages(page)) ?? [], with a comment about mocked pages. The test file and the rest of frameCapture.ts are the same patch.
Why the guard is needed: I ran vitest run src/services/frameCapture at this head: 31 files and 339 tests pass, 11 skipped. When I removed the ?? [] and ran the same suites again, about 10 existing tests failed (the motionBlur seek display, serial/pipelined untrusted capture, boundary screenshots, pipelined diagnostics). Their mocked page.evaluate returns undefined, and iterating undefined throws. So the guard is load-bearing for the existing suite, and it is harmless with a real page. tsc --noEmit is clean in packages/engine.
Earlier checks still hold: The patch is otherwise identical, so the red/green check on stops waiting for a url whose decode never settles and names it from my previous review still applies.
Non-blocking:
- My earlier notes still apply: urls that time out in
finalizeDrawElementInitor the batch loop are not named in diagnostics, and the type indrawElementService.tsis stale. - A cleaner long-term option is for the shared mock to return
[], so the production code doesn't have to allow for test pages.
Reviewed by Genesis V2 on behalf of @miguel-heygen.
A frame is no longer held for as long as one CSS background image takes to decode.
Before,
decodeDynamicCssBackgroundImagesawaitedimage.decode()for every newly assigned inline background url with no deadline. One url whose decode never settles held the frame until the capture stall guard fired (60 s with no progress), and the render failed.Now each decode races a 15 s limit. A url that does not settle in time is captured without it, is not waited on again on later frames, and is written to the browser diagnostics so a failed or odd frame names it. The page-local decoder used by the draw-element batch path gets the same limit.
fails without the fix:
stops waiting for a url whose decode never settles and names itWhat I measured
tsc --noEmitclean inpackages/engine.What I did NOT exercise
Size
2 files, +65 -14.