Skip to content

fix(desktop): attribute bind calls to the registering window id#35654

Merged
crowlKats merged 4 commits into
denoland:mainfrom
crowlKats:fix/desktop-binding-window-id-mismatch
Jul 7, 2026
Merged

fix(desktop): attribute bind calls to the registering window id#35654
crowlKats merged 4 commits into
denoland:mainfrom
crowlKats:fix/desktop-binding-window-id-mismatch

Conversation

@crowlKats

@crowlKats crowlKats commented Jun 30, 2026

Copy link
Copy Markdown
Member

Fixes #35647.

Problem

deno desktop bindings (win.bind(name, fn)) stop working when the entrypoint
has a jsr:/remote import that is actually used, e.g.:

import { extname } from "jsr:@std/path";
const win = new Deno.BrowserWindow({ title: "My App" });
win.bind("ping", async () => "pong");
async function unused() { return extname(); } // remove this call -> bindings work
Deno.serve((_) => new Response("hi", { headers: { "content-type": "text/html" } }));

Calling bindings.ping() in the renderer rejects with
Uncaught (in promise) Error: No callback bound for: ping, even though the
binding was registered.

Root cause

The runtime keeps a per-window callback registry (windowBindCallbacks in
DESKTOP_JS) keyed by window id, and on a bindCall event looks the callback
up by ev.windowId. Every other window event (keyboardEvent, mouse events,
focusChanged, windowResize, closeRequested, menu clicks) sources its id
from laufey's per-window event struct (ev.window_id, wired up in
setup_window_events), so those ids always match the id create_window
returned.

bindCall was the one exception. WefDesktopApi::bind registers the native
binding on Window::from_id(window_id), but the callback it installs emitted
DesktopEvent::BindCall { window_id: js_call.window_id, .. } — the backend's
per-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 the
window), 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 the
backend's No binding for: the native binding did fire (its closure ran and
produced 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. The
bind closure already captures that window_id in scope, and it is exactly the
key windowBindCallbacks is keyed by, so the lookup can no longer miss. This
fixes 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_id is trustworthy; this
change 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 desktop passes.

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 bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. bind() reads this.windowId synchronously at registration; if the native constructor returns before the backend finalizes the id (the larger module graph from the jsr: import would widen that race), the callback gets registered under a stale/placeholder id.
  2. 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.

Comment thread cli/rt/desktop.rs Outdated
Comment on lines +225 to +227
const callbacks = registry.get(windowId);
const scoped = callbacks?.get(name);
if (scoped) return scoped;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cli/rt/desktop.rs Outdated
// 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]>
@crowlKats

Copy link
Copy Markdown
Member Author

Pushed 9549d43 addressing the two inline points (security-boundary scoping + dropping the marker scraping in favour of a concat!-spliced macro).

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 keyboardEvent/mouse/menu dispatches), so this no longer diverges from how the other windows.get(ev.windowId) paths behave — it just avoids dropping an otherwise-registered call when the id is one nobody has seen.

I don't have the CEF/Windows backend set up to confirm which of your two candidates (stale this.windowId at bind() time vs. per-renderer-order id assignment) is the source, so I'd rather not guess at a bridge fix blind. Happy to land this as belt-and-suspenders and file a follow-up to diagnose the drift at the backend so the event-routing paths get fixed at the source too — does that split work for you?

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 off this.windowId too, 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_id mismatch 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.

Comment thread cli/rt/desktop.rs Outdated
// 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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cli/rt/desktop.rs Outdated
case "bindCall": {
const callbacks = windowBindCallbacks.get(ev.windowId);
const fn_ = callbacks?.get(ev.name);
const fn_ = resolveBindCallback(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cli/rt/desktop.rs Outdated
// 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");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]>
@crowlKats

Copy link
Copy Markdown
Member Author

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 js_call.window_id. Every window event (keyboardEvent, mouse, focus, resize, close, menu clicks) sources its id from laufey's per-window event struct (ev.window_id) wired up in setup_window_events, so those all agree with the id create_window returned. bindCall is the one consumer that didn't: WefDesktopApi::bind registered the native binding on Window::from_id(window_id) but then emitted DesktopEvent::BindCall { window_id: js_call.window_id, .. } — the backend's per-call renderer id. That's exactly why the failure was the Deno-side No callback bound for and not the backend's No binding for: the native binding fired (the closure ran), it just carried a renderer id that didn't match the id windowBindCallbacks was keyed under. This also matches your read that nativeBind/event delivery agree on an id — they do; it was the reported id on the call that diverged, not this.windowId at bind() time.

Fix. The bind closure already captures the window_id the binding was registered on, and that id always equals the runtime's map key. So it now emits that instead of js_call.window_id — one line plus a comment. Because the id the call arrives under is now the registration id by construction, resolveBindCallback is unnecessary: I reverted DESKTOP_JS to the strict per-window lookup, which drops the name-based fallback (and its cross-window reach — your line-38/1379 concern) and the resolver macro + isolate test along with it.

On the "1 of 16 consumers" point: this was never a shared-drift problem across all sixteen dispatch sites. The other fifteen route on ev.window_id from the window that generated the event and were never affected; bindCall was the sole site pulling a different id source. unbind() is likewise fine now — it keys off this.windowId, which matches the id calls arrive under.

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. cargo test -p denort desktop is green and both crates compile.

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@crowlKats crowlKats changed the title fix(desktop): route binding calls when window id mismatches fix(desktop): attribute bind calls to the registering window id Jul 7, 2026
@crowlKats
crowlKats merged commit 50118c2 into denoland:main Jul 7, 2026
139 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

desktop: bindings not working in code that has a jsr:@std/path import

2 participants