Skip to content

fix(desktop): route binding calls when window id mismatches#35654

Open
crowlKats wants to merge 2 commits into
denoland:mainfrom
crowlKats:fix/desktop-binding-window-id-mismatch
Open

fix(desktop): route binding calls when window id mismatches#35654
crowlKats wants to merge 2 commits into
denoland:mainfrom
crowlKats:fix/desktop-binding-window-id-mismatch

Conversation

@crowlKats

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.

Two things combine:

  1. Why the import matters. Deno's TS transform elides a value import that
    is never referenced, so with the extname() call commented out @std/path
    is never loaded at all. Using extname keeps the import and pulls in the
    remote module graph, which delays the entrypoint's evaluation/startup.

  2. Why bindings break. DESKTOP_JS dispatches a bindCall by looking up
    the callback in a per-window map keyed by window id (ev.windowId). On the
    CEF/Windows backend the slower startup lets the window id a call arrives
    under drift from the id bind() recorded the callback under, so the
    window-scoped lookup misses and the call is rejected — despite the binding
    being registered (the native registry still has it, which is why the
    message is the Deno-side No callback bound for, not the backend's
    No binding for).

Fix

When the window-scoped lookup misses, fall back to an unambiguous name
match: if exactly one window has a callback registered for that name, route
to it. Apps that register the same name on multiple windows keep strict
per-window matching and still fall through to the existing rejection.

This hardens the runtime↔backend boundary so an inconsistent window id from
the backend no longer drops an otherwise-registered binding call.

Test

Added desktop_js_bind_call_falls_back_on_window_id_mismatch. Verified by
simulating a mismatched BindCall.window_id in the bridge: without the
fallback the packaged app reproduces No callback bound for: ping; with it
the call resolves (pong). The normal single-window path is unaffected.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant