fix(desktop): transport binding args and results as DesktopValue so Uint8Array survives

[crowlKats]

Fixes #36498.

Problem

Binding return values were funneled through serde_json::Value, which has no byte-array representation: a handler returning a Uint8Array made op_desktop_resolve_bind_call reject with invalid type: byte array, expected any valid JSON value. Arguments took the same JSON round-trip in the other direction, so a Uint8Array passed webview→Deno arrived as a plain numeric-keyed object ({"0":1,"1":2,...}), which is also pathologically slow for large buffers.

Fix

laufey::Value and both backends (webview, CEF) already transport binary natively — only the runtime-side JSON round-trip dropped it. This PR:

• gives DesktopValue (which already had a Binary variant and a ToV8 impl) manual Serialize/Deserialize impls in which Binary maps to serde bytes — serde_v8 materializes bytes as Uint8Array in both directions;
• switches DesktopEvent::BindCall.args, PendingBindResponses, and op_desktop_resolve_bind_call from serde_json::Value to DesktopValue;
• converts DesktopValue ⇄ laufey::Value directly in rt_desktop (the now-unused laufey_value_to_json / json_to_laufey_value helpers are removed).

Non-binary values keep exact JSON semantics (pinned by tests, including the int-vs-double distinction laufey's Value::Int(i32) requires).

Also fixed: silently wrapping integers

The old json_to_laufey_value did laufey::Value::Int(i as i32) on an as_i64(), a wrapping cast — 3_000_000_000 reached the renderer as -1294967296. The new visit_i64/visit_u64 degrade out-of-range integers to Double instead. Pinned by test.

Recursion is now bounded

The visitor recurses once per nesting level, so a self-referential value returned from a handler (const o = {}; o.self = o; return o) ran the runtime thread off the end of its stack. Depth is threaded through a DeserializeSeed and capped at 128, matching serde_json's own recursion limit; past that the caller gets an error. This was equally true of the serde_json::Value visitor before, but this PR rewrites exactly that code.

Limitations

The typed-array view type is not preserved. serde_v8 routes every ArrayBufferView and ArrayBuffer to visit_byte_buf, so a handler returning a Float64Array, Int32Array, DataView or bare ArrayBuffer arrives on the other side as raw bytes rather than its original view type. Callers needing the original type must carry it themselves. This is lossy only relative to a transport that never worked — before this PR any of those was a hard invalid type: byte array error.

The renderer materializes incoming binary as ArrayBuffer, not Uint8Array (both backends). That last-mile discrepancy is in laufey's renderer glue; fix upcoming there. With this PR the data itself is correct and complete in both directions on both backends.

Verification

Ran the issue's repro (extended to cover the argument direction) on macOS with the webview backend:

• before: ret=FAIL: TypeError: invalid type: byte array, expected any valid JSON value, echo arrives as plain object
• after: return carries correct bytes; bindings.echo(new Uint8Array([9,8,7,128])) arrives Deno-side as a real Uint8Array and round-trips intact.

[crowlbot]

Traced both directions against the actual serde_v8 in libs/serde_v8, since the whole PR rests on the claim that serde bytes survive the boundary. The mechanism checks out:

• Rust → JS: ser.rs:485 — serialize_bytes → slice_to_uint8array. And DesktopEvent is #[serde(tag = "kind")], whose struct variants serialize their fields straight through the target serializer, with no Content buffering — that's only the internally-tagged deserialize path, which isn't used here. So Binary reaches serialize_bytes intact.
• JS → Rust: de.rs:166 — deserialize_any maps ArrayBufferView | ArrayBuffer to visit_byte_buf, which the new visitor handles. The visitor's missing integer methods are fine too: serde's defaults forward visit_i32 → visit_i64 and visit_u32 → visit_u64, which is exactly what serde_v8 calls at de.rs:152-155.

The JS glue holds as well — cli/rt/desktop.rs:749 does Array.isArray(ev.args), still true now that args is a Vec<DesktopValue> seq rather than a JSON array.

An unadvertised bug fix worth calling out

The old json_to_laufey_value did laufey::Value::Int(i as i32) on an as_i64() — a silently wrapping cast. 3_000_000_000 arrived at the renderer as -1294967296. The new visit_i64/visit_u64 degrade out-of-range integers to Double instead, and the test pins it. That's a real correctness fix beyond the stated scope, and the description undersells it.

Three things I'd raise

1. An avoidable deep copy per call — on the exact path this PR is optimizing.

let args: Vec<DesktopValue> = js_call
  .args
  .iter()
  .cloned()
  .map(laufey_value_to_desktop_value)
  .collect();

laufey_value_to_desktop_value takes ownership (cli/rt_desktop/lib.rs:1530), so .cloned() deep-copies every argument — binary payloads included — purely to satisfy the borrow, and then the conversion moves the same data again. Since part of the motivation is that the old path was pathologically slow for large buffers, std::mem::take(&mut js_call.args) on a mut js_call binding would drop one full copy per call. (A partial move of the field won't work if resolve takes self, hence mem::take.)

2. Typed-array type is silently lost.

Because serde_v8 routes all of ArrayBufferView | ArrayBuffer to visit_byte_buf, a handler returning a Float64Array, Int32Array, DataView, or a bare ArrayBuffer now arrives at the renderer as an opaque byte blob rather than its original view type. That's clearly better than the previous hard error, but it's lossy in a way neither the PR description nor the code comments mention — worth a sentence, especially alongside the existing note about the renderer materializing ArrayBuffer rather than Uint8Array.

3. No recursion depth limit in the new visitor.

A cyclic object returned from a handler (const o = {}; o.self = o; return o) recurses to stack overflow. This is pre-existing — serde_json::Value's visitor recursed unbounded too — so it isn't a regression. But this PR is rewriting precisely that visitor, which makes it the natural moment to add a depth guard.

Minor: PendingBindCall (runtime/ops/desktop.rs:404) had its types updated here, but as far as I can find it's only defined and re-exported through cli/rt/desktop.rs:1206 — never constructed.

[crowlKats]

Thanks — thorough trace, and all three land. Fixed in 4ddd7c1, plus a merge of main (the branch had gone conflicted on a test-import collision with #36649).

1. The deep copy

Fixed exactly as you described. js_call.resolve takes self, so the field can't be moved out; mem::take on a mut js_call binding drops one full copy of every argument per call:

let args: Vec<DesktopValue> = std::mem::take(&mut js_call.args)
  .into_iter()
  .map(laufey_value_to_desktop_value)
  .collect();

Good catch that this was on the path the PR is specifically meant to make cheap for large buffers — the clone undercut the stated motivation.

2. Typed-array type loss

Documented in both places. There's now a paragraph on the serde impls spelling out that serde_v8 routes every ArrayBufferView and ArrayBuffer to visit_byte_buf, so Float64Array / Int32Array / DataView / bare ArrayBuffer all arrive as raw bytes, and a Limitations section in the description covering it alongside the existing renderer-ArrayBuffer note. Framed as lossy only relative to a transport that never worked, since before this any of them was a hard error.

3. Recursion depth

Added. Depth is threaded through a DeserializeSeed (a bare Deserialize impl has nowhere to carry it) and capped at 128 to match serde_json's own limit; visit_seq and visit_map each check before descending, and visit_some passes the current depth through.

Two tests cover it — one nesting through arrays, one through objects, since the two guards are separate. Both build the value as a serde_json::Value rather than parsing text: serde_json's parser has its own recursion limit that fires first and would have masked the guard under test. A test also pins that nesting at exactly the limit still deserializes, so the guard can't degenerate into rejecting anything structured.

You're right it's pre-existing rather than a regression, but agreed it's the moment to fix it.

The integer fix

Fair — the description undersold it. There's now a section calling out that the old Int(i as i32) wrapping cast sent 3_000_000_000 to the renderer as -1294967296, and that out-of-range integers now degrade to Double.

PendingBindCall

Confirmed dead: defined at runtime/ops/desktop.rs:404, re-exported at cli/rt/desktop.rs:1206, never constructed anywhere in the tree. I've left it alone here — CONTRIBUTING asks for minimal PRs without drive-by changes, and deleting a public re-export is its own (small) change. Happy to send a follow-up removing it if you'd rather.

Verified: deno_runtime desktop tests pass (28, including the two new depth tests), deno desktop tests pass (87), denort_desktop builds, clippy clean across all three, formatted.

[crowlbot]

Re-reviewed at 4ddd7c1.

All three are addressed, and the depth work is done properly — threading the counter through a DeserializeSeed is the only way to carry state into a nested Deserialize, and both visit_seq and visit_map increment before recursing. mem::take on js_call.args is the right call, and the comment explaining why the field can't simply be moved out (resolve consumes js_call) will save the next reader the same detour. The "Limitations" section on typed-array view types is exactly the disclosure that was missing.

One follow-on, since the depth limit is now advertised as covering this: it only guards the JS → Rust direction. A DesktopValue also arrives from the other side, and that path is unbounded:

• laufey_value_to_desktop_value recurses once per level over a renderer-supplied laufey::Value (binding arguments), before the deserializer is ever involved;
• DesktopValue::to_v8 then recurses again over whatever came out.

Cycles presumably can't exist in a laufey::Value — something upstream would have to have already resolved them to build it — but depth can: a deeply nested array or object passed as a binding argument from the renderer still walks the runtime thread's stack, with no MAX_DEPTH anywhere on that path. Worth either bounding those two conversions too, or scoping the doc comment on MAX_DEPTH to the direction it actually covers, so a reader doesn't take it as a general guarantee for DesktopValue.

Also flagging that test integration (2/2) debug macos-x86_64 is red here. The same job is red on #36006 too, so it looks unrelated to this change, but worth confirming before merge.

[crowlbot]

Following up on the CI question from my last comment — the failure is unrelated to this change:

failed tests:
    integration::repl::pty_regex_literal_with_quote

panicked at tests/integration/repl_tests.rs:67:13:
Timed out.

A PTY REPL timeout on macos-x86_64, failing identically on #36573 and #36006, neither of which touches the REPL. Flake — safe to ignore or re-run.

[crowlKats]

Bounded the other direction in a3275c9 — you're right that scoping the doc comment would have been the weaker of the two options you offered, since the hazard is real either way.

laufey_value_to_desktop_value now carries a depth and is bounded by the same MAX_DEPTH, so both entry points agree on what's too deep. That's what makes to_v8 safe rather than incidentally-safe: it only ever walks a value that arrived through one of the two bounded conversions, so it needs no guard of its own.

Too-deep binding arguments reject that one call rather than taking the app down:

Err(err) => {
  js_call.reject(laufey::Value::String(err));
  return;
}

The execute_js result path goes through the same conversion, so it's covered too — a too-deep result surfaces as a rejection instead of being taken as a successful value.

Your point about cycles is right and worth keeping explicit, so it's in the MAX_DEPTH doc: a laufey::Value can't be cyclic, because something upstream would have had to resolve the cycle to build it. Depth alone is the whole exposure on that side, unlike the JS side where o.self = o is the realistic source. The doc now names both directions and what enforces each.

Three tests: arrays past the limit, objects past the limit (separate arm, separate guard), and at-the-limit still converting.

---

On CI: integration::repl::pty_regex_literal_with_quote — a PTY test asserting on terminal escape sequences. Unrelated to this change and to #36006, as you guessed; it's failing the same way on both.

[crowlbot]

Re-reviewed at a3275c9. Fully green, and the remaining gap is closed properly.

Bounding laufey_value_to_desktop_value with the same MAX_DEPTH, exported so both directions share one constant, is the right shape — and surfacing an over-deep argument as js_call.reject(...) rather than an error means a hostile page gets a rejected binding call instead of taking the app down. Rejecting the execute_js result while falling back to the error string on the error branch is the correct asymmetry too: a too-deep error value shouldn't mask what the error was.

The doc comment on MAX_DEPTH is the part worth keeping as-is. Enumerating both entry points, noting that a laufey::Value can't be cyclic but can still be deep, and then closing with why bounding both is what makes to_v8 safe — that last line is the actual invariant, and it now lives somewhere a future edit will trip over it.

Nothing further from me on this one.

[crowlbot]

Re-reviewed at the current head — this round is a merge of main with no changes of your own, so nothing new on the code.

Since that merge pulls in #36575 and #36574, which touch the same three desktop files this branch does, I checked the resolution rather than assuming: no conflict markers anywhere, this branch's own changes are intact, and both merged PRs' changes survived in runtime/ops/desktop.rs (op_desktop_alert_async + ERROR_DIALOG_SHOWING from #36575, the .backup gate on the update sentinel from #36574). Clean.

CI is still running; nothing red so far.