fix(desktop): attribute bind calls to the registering window id#35654
Conversation
A bindCall from a renderer is dispatched by looking up the bound callback in a per-window map keyed by window id. On the CEF/Windows backend, an entrypoint that pulls in extra modules (e.g. a "jsr:@std/path" import that is actually used -- an unused import is elided by the TS transform, which is why commenting out the call appears to "fix" it) delays startup enough that the window id the call arrives under no longer lines up with the id bind() recorded the callback under. The lookup misses and the call is rejected with "No callback bound for: <name>", even though the binding was registered. When the window-scoped lookup misses, fall back to an unambiguous match: if exactly one window registered a callback for that name, route to it. Per-window same-named bindings keep strict matching. Fixes denoland#35647
Replace the substring assertions (which checked nothing about behaviour) with a test that runs the actual `resolveBindCallback` shipped in DESKTOP_JS inside a V8 isolate and asserts its routing: exact-window match, the denoland#35647 unique-name fallback on a window-id mismatch, unknown names, and that an ambiguous (same name on two windows) mismatch refuses to guess. The resolver is factored into a closure-free function extracted between TEST-EXTRACT markers so the test runs the shipped bytes.
bartlomieju
left a comment
There was a problem hiding this comment.
The fallback itself looks correct and the ambiguity guard is sensible, but I'm not convinced this is the right layer for the fix.
The root cause of the id drift is still undiagnosed, and ev.windowId keys every other dispatch in this event loop too: keyboardEvent, mouse events, appMenuClick, contextMenuClick, resize/close all do windows.get(ev.windowId) and silently break on a miss. If the CEF backend really reports a different id for bindCall, it's presumably doing so for all of those as well, meaning input events are being dropped for the same reason and this only patches the one case that produces a visible error.
Before hardening the JS side, it would be good to rule out two candidates:
bind()readsthis.windowIdsynchronously at registration; if the native constructor returns before the backend finalizes the id (the larger module graph from thejsr:import would widen that race), the callback gets registered under a stale/placeholder id.- The backend assigns ids per renderer creation order while the runtime assigns them per constructor order, and delayed startup reorders them.
If either of those is it, a bridge/backend fix would resolve bindings and event routing at once. Happy to have this land as belt-and-suspenders on top of that, but as the only fix it papers over a broader misrouting.
One more behavioral note: the fallback makes routing timing-dependent when two windows bind the same name at different times (a mismatched call resolves against window A before window B's bind(), rejects after). Not a blocker given the ambiguity rule, but another sign the fix sits downstream of the actual bug.
| const callbacks = registry.get(windowId); | ||
| const scoped = callbacks?.get(name); | ||
| if (scoped) return scoped; |
There was a problem hiding this comment.
As written, the fallback also fires when the incoming id is a known window that simply didn't bind this name. Per-window bindings are a security boundary (a binding typically exposes a privileged host function to renderer content), so with this change any window can invoke another window's callback as long as the name is globally unique, including a window that was deliberately not given that binding.
Restricting the fallback to ids we've never seen still covers #35647 (the call arrives under an id bind() never registered) without letting known windows reach across:
| const callbacks = registry.get(windowId); | |
| const scoped = callbacks?.get(name); | |
| if (scoped) return scoped; | |
| // Known window: strict per-window matching only. | |
| const callbacks = registry.get(windowId); | |
| if (callbacks) return callbacks.get(name) ?? null; |
There was a problem hiding this comment.
Good catch — done in 9549d43. A known window id now matches strictly (if (callbacks) return callbacks.get(name) ?? null;); only an unknown id (the #35647 drift, where the call arrived under an id that registered nothing) falls back to the unique-name match. So a known window can no longer reach across to another window's uniquely-named callback. Added assertion #5 covering exactly that cross-reach case.
| // the target is unambiguous, so route to it rather than dropping the call. | ||
| // Per-window same-named bindings keep strict matching: ambiguous names | ||
| // return null so the caller still rejects. | ||
| // TEST-EXTRACT:resolveBindCallback:START |
There was a problem hiding this comment.
Running the real function in a V8 isolate instead of DESKTOP_JS.contains(...) is a genuine improvement over the existing tests in this file. But these markers (and the test-facing part of the comment above) ship in the binary payload that executes at startup, and marker scraping is fragile as a convention.
Consider defining resolveBindCallback in its own const RESOLVE_BIND_CALLBACK_JS: &str that gets concatenated into DESKTOP_JS, so the test can reference the constant directly with no extraction step.
There was a problem hiding this comment.
Done in 9549d43. Dropped the TEST-EXTRACT markers and the extract_marked scraper. resolveBindCallback is now a resolve_bind_callback_js! macro that expands to a string literal, spliced into DESKTOP_JS via concat! and referenced directly by the test through RESOLVE_BIND_CALLBACK_JS — the test runs the exact shipped bytes with no extraction step, and nothing test-facing ships in the payload anymore.
Address review feedback on denoland#35654: - Per-window bindings are a security boundary, so a *known* window must never reach another window's uniquely-named callback via the fallback. Known window ids now match strictly; only an unknown id (the denoland#35647 drift case) falls back to a unique-name match. Adds a regression assertion for the cross-reach case. - Replace the fragile TEST-EXTRACT marker scraping with a `resolve_bind_callback_js!` macro spliced into DESKTOP_JS via `concat!` and referenced directly by the test, so the test runs the exact shipped bytes with no extraction step. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Pushed 9549d43 addressing the two inline points (security-boundary scoping + dropping the marker scraping in favour of a On the broader root-cause question: I agree this sits downstream of the actual id drift and shouldn't be the only fix long-term. What the security scoping changes is the framing — because the fallback now fires only for window ids we have never registered anything under, it is a pure safety net for the drift case rather than a behavioural change for known windows. A known window still strictly matches its own callbacks (same as I don't have the CEF/Windows backend set up to confirm which of your two candidates (stale |
bartlomieju
left a comment
There was a problem hiding this comment.
Nice work on the test rig — the macro_rules!/concat! trick so the unit test executes the exact shipped bytes in a real isolate is genuinely nice, and the harness itself is well constructed. But I think this hardens the wrong layer, and the fallback as written opens a cross-window hole. Details inline; two points that don't fit a diff line:
unbind()keys offthis.windowIdtoo, so under the same drift it misses the map entry while this fallback keeps routing to the "removed" callback — another artifact of patching one consumer of an inconsistent id rather than the id itself.- The end-to-end verification (simulated
BindCall.window_idmismatch in the bridge) was manual and isn't committed, so the suite tests the fallback's semantics, not the bug.
My ask: before we ship a routing heuristic, let's find where this.windowId at bind() time diverges from the id the backend delivers events under — logging both ids in the #35647 repro should pin it down quickly. If a fallback is still wanted as defense-in-depth after that, it needs to not be reachable by real windows that simply never bound anything (e.g. seed the registry at window construction), and the security-boundary comment should state what the fallback trades away.
| // name. Ambiguous names return null so the caller still rejects. | ||
| function resolveBindCallback(registry, windowId, name) { | ||
| const callbacks = registry.get(windowId); | ||
| if (callbacks) return callbacks.get(name) ?? null; |
There was a problem hiding this comment.
The "known window keeps strict matching" guard only holds for windows that have called bind() at least once — a window enters windowBindCallbacks on its first bind(). A real window that never bound anything presents an id with no registry entry, so it takes this fallback and can invoke another window's uniquely-named callback. In a multi-window app where one window hosts untrusted/remote content and binds nothing, that's a cross-window escalation — and that window is indistinguishable from the drift case this fallback targets.
Seeding an empty map per window in the OrigBW wrapper would shrink the hole, but there's a more fundamental tension: the fallback exists because ev.windowId can't be trusted, yet the comment claims per-window matching on that same id is a security boundary. If the id is unreliable at this layer, a name-based fallback keyed on its unreliability weakens the boundary by construction.
| case "bindCall": { | ||
| const callbacks = windowBindCallbacks.get(ev.windowId); | ||
| const fn_ = callbacks?.get(ev.name); | ||
| const fn_ = resolveBindCallback( |
There was a problem hiding this comment.
ev.windowId is the routing key for every other window event too — windows.get(ev.windowId) at ~15 dispatch sites (keydown, close, focus, resize, menuclick, …). If the backend genuinely delivers a drifted id, all of those lookups silently miss as well; this fixes one of sixteen consumers.
The error-message detail in the PR body actually points at the root cause: the failure is the Deno-side No callback bound for, not the backend's No binding for — so the native registry found the binding under the incoming id. That means nativeBind.call(this, name) and event delivery agree on an id, and it's the JS map keyed by this.windowId at bind() time that diverged (stale id during construction?). I'd rather fix the id at the source — read it lazily at dispatch, or re-key when the native id settles — than fuzzy-match here. Logging both ids in the #35647 repro should pin down the mechanism.
| // reach another window's uniquely-named callback via the fallback. Window | ||
| // 3 exists (it bound "other") so asking it for "ping" resolves strictly and | ||
| // returns null, even though window 7's "ping" is globally unique. | ||
| check(resolveBindCallback(reg, 3, "ping"), null, "known window no cross-reach"); |
There was a problem hiding this comment.
Case 5 uses a window that bound something ("other"), so it's "known" and stays strict. The dangerous case is a real window with zero bindings: it has no registry entry, takes the fallback, and reaches win7's "ping". Worth adding that case asserting it does NOT resolve — with the current implementation it resolves, which is the gap described above.
Route the fix to the source of the id drift instead of compensating for it in DESKTOP_JS. The bind-call bridge emitted `DesktopEvent::BindCall` with `js_call.window_id` — the backend's per-call renderer id, which can drift from the id `bind()` registered the callback under (seen on CEF/Windows when a larger module graph delays startup; denoland#35647). The runtime keys `windowBindCallbacks` by the registration id, so the drift made the lookup miss and reject an otherwise-registered call. The bind closure already has the window id the binding was registered on in scope, and that id always matches the map key. Emit it instead. This fixes the misrouting deterministically at the boundary, so the runtime side needs no window-id fallback heuristic — revert DESKTOP_JS to the strict per-window lookup, removing the name-based fallback (and its cross-window reach) along with the resolver macro and its unit test. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
You were right to push back on hardening the JS side — I dug into the bridge and found the actual id drift, so I've replaced the routing heuristic with a fix at the source (c74642e). Root cause. The drift is Fix. The bind closure already captures the On the "1 of 16 consumers" point: this was never a shared-drift problem across all sixteen dispatch sites. The other fifteen route on I can't exercise the CEF/Windows repro locally, but this fix doesn't depend on reproducing the drift: it removes the runtime's dependency on the untrusted per-call id entirely rather than fuzzy-matching around it. |
bartlomieju
left a comment
There was a problem hiding this comment.
The fix itself LGTM — attributing the BindCall event to the registration
window id is correct by construction: each bind() registers its own closure
on one specific window, and that id is exactly the key the runtime-side
windowBindCallbacks lookup uses, so a drifting per-call id from the backend
can no longer make the lookup miss. Multi-window apps keep strict per-window
routing.
Could you rewrite the PR body before landing? It describes a different,
earlier approach (a JS-side unambiguous-name fallback in DESKTOP_JS) and a
test (desktop_js_bind_call_falls_back_on_window_id_mismatch) that aren't in
the diff. Worth also mentioning that the backend reporting an inconsistent
per-call window id is the underlying bug to fix upstream in laufey.
Fixes #35647.
Problem
deno desktopbindings (win.bind(name, fn)) stop working when the entrypointhas a
jsr:/remote import that is actually used, e.g.:Calling
bindings.ping()in the renderer rejects withUncaught (in promise) Error: No callback bound for: ping, even though thebinding was registered.
Root cause
The runtime keeps a per-window callback registry (
windowBindCallbacksinDESKTOP_JS) keyed by window id, and on abindCallevent looks the callbackup by
ev.windowId. Every other window event (keyboardEvent, mouse events,focusChanged,windowResize,closeRequested, menu clicks) sources its idfrom laufey's per-window event struct (
ev.window_id, wired up insetup_window_events), so those ids always match the idcreate_windowreturned.
bindCallwas the one exception.WefDesktopApi::bindregisters the nativebinding on
Window::from_id(window_id), but the callback it installs emittedDesktopEvent::BindCall { window_id: js_call.window_id, .. }— the backend'sper-call renderer id, not the id the binding was registered under. On the
CEF/Windows backend that per-call id can drift from the registration id (a
larger module graph from a used
jsr:import delays startup and widens thewindow), so the runtime-side lookup misses and the call is rejected.
This is why the error is the Deno-side
No callback bound for, not thebackend's
No binding for: the native binding did fire (its closure ran andproduced the event) — it just carried a window id the runtime never registered
a callback under.
Fix
Emit the id the binding was registered on instead of
js_call.window_id. Thebind closure already captures that
window_idin scope, and it is exactly thekey
windowBindCallbacksis keyed by, so the lookup can no longer miss. Thisfixes the misrouting deterministically at the runtime↔backend boundary; the
runtime side needs no window-id heuristic and keeps its strict per-window
lookup, so multi-window apps retain per-window routing (a window can only reach
callbacks it itself registered).
The underlying defect — the backend reporting a per-call renderer window id
that is inconsistent with the id a window was created under — should still be
fixed upstream in laufey so that
js_call.window_idis trustworthy; thischange makes the runtime independent of it either way.
Test
No automated coverage: reproducing the drift requires the CEF/Windows backend,
which isn't available in CI. The fix does not depend on reproducing it — it
removes the runtime's dependency on the untrusted per-call id rather than
matching around it. Verified locally that both crates compile and
cargo test -p denort desktoppasses.