fix: settle popup and download event handles without deadlock - #463
Merged
Conversation
waitForEvent('popup'|'download') can deadlock past its own timeout when the
event fires: the Playwright dispatcher delivers the Page/Download channel
reentrantly while QuickJS is still inside transport send, so the waiter
never settles and the timer cannot run. Queue inbound protocol messages to
a microtask so handle materialization runs after send returns.
Contributor
🟠 Maintainer review suggested — low confidenceThe automated review could not reach a fully supported conclusion. Limitations
This review is advisory and does not block merging. |
Catch throws from the queued inbound flush and cancel the host, and pin popup/download waitForEvent tests to exact values and per-event timeouts.
Do not expose handleServerMessage on the public class; the delivery-error test reaches it with a narrow cast. Drop the suite-wide mock restore.
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
1 participant
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.
Cause
Hosted
page.waitForEvent('popup'|'download')honors its timeout when no event fires, but can hang until the outer browser-run ceiling when the event does fire. Tracing the QuickJS Playwright bridge:PlaywrightTransport.senddispatches into the Node dispatcher, which delivered__create__Page/Download channels synchronously viaonmessage→deliverTransport→ reentrantcallFunction. The waiter then never settles and the QuickJS timeout callback cannot run.This is the shared event-to-proxy materialization path for both popup and download. It is not CDP
acceptDownloadsand not a broad bridge rewrite.Inbound flush now catches a throwing
deliverand routes it toonDeliveryErrorso it cannot become an uncaught microtask. The runner cancels the host if it already exists.Contract
waitForEvent('popup', { timeout })resolves to a Page whoseurl(),title(), andbodycan be read, or rejects at that timeout.waitForEvent('download', { timeout })resolves to a Download that cansaveAs, or rejects at that timeout.Tests
Independent gates in
src/browser/run/runner.test.ts:https://popup.example.test/, resolvewaitForEvent('popup'), read urlhttps://popup.example.test/, titlePopup Title, bodyPopup Body.<a download>, resolvewaitForEvent('download'),saveAs.it.each(['popup', 'download'])with 80ms timeout fails asBROWSER_RUN_TIMEOUTcontainingTimeout 80ms exceeded while waiting for event "…"in under 1s.onDeliveryError(no uncaught microtask).npx vitest run src/browser/run/runner.test.ts— 89 passed.Anti-overfitting
No eval scenario IDs, CSV fixtures, or vendor-verification URLs. Any browser-run waiter for popup or download benefits.
Related
Closed webcmd-cloud#79: CDP download-behavior enablement is not this fix. If it is still required independently, it needs a per-session download directory and cleanup, not a global
/tmppath.Does not merge.