fix(cdp): deliver Runtime.bindingCalled to the session that subscribed - #632
Open
xrip wants to merge 1 commit into
Open
fix(cdp): deliver Runtime.bindingCalled to the session that subscribed#632xrip wants to merge 1 commit into
xrip wants to merge 1 commit into
Conversation
drain_binding_calls collapsed every session of a page into one entry of a HashMap and addressed the event to whichever one came out first. A client that opens a page the ordinary way holds two sessions, because Target.createTarget opens one and the Target.attachToTarget after it opens another, and it discards any event whose sessionId is not the one it attached with. So `page.exposeFunction()` registered its binding, the page called it, and the callback fired on a session no client was listening to. Against `obscura serve`, over the browser socket: before: my sessionId page-1-session-1, bindingCalled -> page-1-session after : my sessionId page-1-session-1, bindingCalled -> page-1-session-1 Runtime.addBinding is a session-scoped subscription in CDP, so record which sessions asked for each name and deliver the call back to those, narrowed to the page it came from. A binding with no recorded subscriber — installed as a preload, or by an embedder driving dispatch without a session — still goes to every session on the page rather than being dropped. The regression guard has two sessions subscribe to one name, so the correct result is two events. Addressing a single session of the page can only ever produce one, which fails whichever session the map ordering picks; asserting only "my session got it" passes about half the time on that ordering.
5 tasks
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.
What changed
Runtime.bindingCalledwas addressed to one arbitrary session of the page, sopage.exposeFunction()fired its callback into a session no client was listening on.drain_binding_callsbuilt apage_id -> session_idmap by collapsing every session of a page into oneHashMapentry:A page normally has more than one session.
Target.createTargetopens one, and theTarget.attachToTargetthat follows opens another — and a client discards any event whosesessionIdis not the one it attached with. Which of the two survived that.collect()was down to map ordering.Against
obscura serve, over/devtools/browser, doingcreateTarget→attachToTarget {flatten:true}→addBinding→ call it:Runtime.addBindingis a session-scoped subscription in CDP, so the fix records which sessions asked for each name and delivers the call back to those, narrowed to the page it came from. A binding with no recorded subscriber — installed as a preload, or by an embedder drivingdispatchwithout a session — still goes to every session on the page rather than being dropped, so nothing that works today starts failing.Found while fixing #623; this is the same one-arbitrary-session flaw the frame events had, in the drain next door. Split out because it is pre-existing on
mainand independent of the frame work.Validation
The one failure is
obscura-cdp::max_connections_cap max_connections_refuses_then_recovers, which fails identically on an unmodifiedmainworktree in this environment.New test
crates/obscura-cdp/tests/binding_called_session.rs(3 cases) goes through the realTarget.createTarget+Target.attachToTargethandshake rather than inserting a session for a hand-made page.The regression guard needed care. Asserting only "my session received it" passes roughly half the time on the old code, because it is a coin flip on map ordering — a flaky test would be worse than none. So the guard has two sessions subscribe to one name, making the correct result two events: addressing a single session of the page can only ever produce one, whichever way the ordering falls. Verified failing on unmodified
mainacross three consecutive runs:Rendering
Not applicable.
Performance
One
HashMap<&str, Vec<&str>>built per drain instead of oneHashMap<String, String>, borrowing session ids rather than cloning them, plus a subscriber lookup per queued call. A page with no binding calls returns before any of it.Checklist