fix(download): bound trigger lifetime and retire cancelled input - #354
Conversation
|
Thanks for this PR and your other recent fixes. I’ve really appreciated the quality of the implementation notes and regression coverage. The explicit distinction between bounded waiting and actually undoing browser-side effects is particularly helpful here. I reviewed I think this is a worthwhile fix to land. Before merging, could we tighten up three remaining lifecycle edges? 1. Keep the fallback detach scoped to the original attachment In I reproduced this sequence using the production
Could the fallback also be conditional on the original attachment identity? Ideally, cleanup sends and detach would enforce that identity at the point of execution. A regression covering replacement during cleanup would complement the existing replacement-before-cleanup test. 2. Carry cancellation through to the actual CDP dispatch boundary The wrapper checks cancellation before calling With the production download handler, I reproduced a connection drop after click preparation, followed by:
This differs from the existing browser tests, which hold an acknowledgement after Chrome has already processed the input. The reproduced late input was a move; I have not established an extra press or duplicate download. Could the operation signal be checked after connection preparation and immediately before native dispatch? This would prevent commands that have not yet been sent from escaping retirement, without requiring cancellation of commands Chrome already received. 3. Include overlay bypass ownership in the independent cleanup When In a handler-level reproduction, the request returned and download listeners were removed, but the bypass count remained This leak also exists while main is stuck, so I’m not treating it as a newly introduced defect. However, now that subsequent work can proceed, it would be useful to retire this resource at the same boundary. Could the operation own an independently releasable, idempotent cleanup for the bypass/passthrough resources, so a late inner These look like focused follow-ups to the current approach rather than a reason to redesign it. I would keep the existing scope—including the documented pre-capture limitation—and address these three cases before merging. Thanks again for the careful work and the unusually clear reproduction evidence. |
|
Thank you for the careful review and the concrete reproductions. I reproduced all three cases and addressed them in b967b27c, after synchronizing the branch with main
Validation on this head:
All 9 reported PR checks passed on this head (CI run). The frontend log includes all 9 lifecycle regressions, and the dedicated browser log reports 3 passed with I also updated the PR body with the before/after evidence, reproducible commands, and verification limits. The pre-capture limitation remains unchanged. Queued click commands are fenced before native dispatch; already delivered input and page-side effects cannot be undone by cancellation. |
|
LGTM |
Summary
A stalled native click acknowledgement can keep
captureBrowserDownloadpending after timeout or cancellation. Its listeners remain installed andhandleDownloadcannot reach thefinallythat releases the extension-wide download gate, so other sessions receiveanother bsk download is active.Bound capture/trigger waiting, fence retired input at native dispatch, and clean up the operation's input and overlay resources within the shared one-second cleanup budget. Fixes #347.
Current head: b967b27c, synchronized with main aaf77145. The dispatch-effect metadata fix from #351 is now included in the base.
Reproduction and evidence
Original hang
The original deterministic reproduction on 8cbcc490 held the trigger promise, or the
Input.dispatchMouseEvent(mousePressed)reply inside productionhandleDownload:100 ms; advance fake time to10,000 msThe second-session test deliberately omits its capability directory and expects that ordinary validation error. It proves gate release, not completion of a second file transfer.
Review follow-up: three independently reproduced edges
The review identified three remaining cases. With the new regression probes added to pre-follow-up commit 04975a6d, all three failed with the observations below. They pass with b967b27c.
undefined[]to["mouseMoved"]after return[]2instead of returning to11and this click's passthrough lease is removed before the press replyThese handler/driver regressions use production
ChromiumCdpwith controlled debugger API replies. The overlay cases also start a subsequent download and deliver the previous operation's late reply: the new operation's bypass and passthrough ownership remain intact. Pending/rejected passthrough-end replies are covered; they reportcleanup_state: failedwithout preventing the independent bypass release.Implementation
sendCommand, after any connection setup. Root and child targets share this boundary. Record the press and its actual attachment only when it reaches this boundary, so a cancelled queued press does not cause a spurious cleanup release.detach(tabId, expectedAttachmentId)checks identity before invalidating or detaching anything. An already dispatched release is not replayed.finallyjoin the same cleanup promise. Bypass and the click's unique passthrough lease release independently, preserving unrelated hover and subsequent click ownership.1,000 mswaiting budget. Rejection or expiration is reported ascleanup_state: failed; the handler can then release the gate.Validation
Local environment: macOS, Node 26.10.0, pnpm 10.17.0, Vitest 4.1.6, Chrome 153.0.8010.54. Final validation used b967b27c.
download-lifecycle.test.tsgit diff --checkThe download browser suite now tests both sides of dispatch:
Use the declared pnpm 10.17.0 from the repository root. Local runs selected it with
npm exec --yes --package pnpm@10.17.0 -- pnpm .... Browser cases skip whenBSK_CLICK_CHROMEis unset.Hosted status checked 2026-09-27 UTC for this head: all 9 reported PR checks passed (CI run). The frontend job reports the 9 lifecycle regressions passing and the extension total of 2,341 passed / 122 skipped. The dedicated download browser job reports 3 passed and emits:
The existing browser suite, Rust, Windows, Node scripts and CodeCC also passed.
Scope and limitations