fix(desktop): route binding calls when window id mismatches#35654
Open
crowlKats wants to merge 2 commits into
Open
fix(desktop): route binding calls when window id mismatches#35654crowlKats wants to merge 2 commits into
crowlKats wants to merge 2 commits into
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.
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.
Fixes #35647.
Problem
deno desktopbindings (win.bind(name, fn)) stop working when theentrypoint has 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.
Two things combine:
Why the import matters. Deno's TS transform elides a value import that
is never referenced, so with the
extname()call commented out@std/pathis never loaded at all. Using
extnamekeeps the import and pulls in theremote module graph, which delays the entrypoint's evaluation/startup.
Why bindings break.
DESKTOP_JSdispatches abindCallby looking upthe callback in a per-window map keyed by window id (
ev.windowId). On theCEF/Windows backend the slower startup lets the window id a call arrives
under drift from the id
bind()recorded the callback under, so thewindow-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'sNo 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 bysimulating a mismatched
BindCall.window_idin the bridge: without thefallback the packaged app reproduces
No callback bound for: ping; with itthe call resolves (
pong). The normal single-window path is unaffected.