Skip to content

structuredClone: throw DataCloneError for a detached ArrayBuffer - #32799

Merged
Jarred-Sumner merged 1 commit into
mainfrom
farm/4e69f772/detached-arraybuffer-datacloneerror
Jun 27, 2026
Merged

Jarred-Sumner merged 1 commit into
mainfrom
farm/4e69f772/detached-arraybuffer-datacloneerror

Conversation

@robobun

@robobun robobun commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

Serializing a graph that contains a detached ArrayBuffer threw a plain TypeError instead of a "DataCloneError" DOMException. The HTML structured serialize algorithm requires DataCloneError here, and Node, Chrome, Firefox, and Safari all throw it. Code that branches on DataCloneError (the standard "unclonable value" signal, e.g. retry-without-transfer fallbacks) takes the wrong branch on Bun.

Reproduction

const ab = new ArrayBuffer(8);
structuredClone(ab, { transfer: [ab] }); // detach it
try { structuredClone({ ab }); } catch (e) { console.log(e.name); }
// Bun: "TypeError"   Node / browsers: "DataCloneError"

Before the fix:

TypeError: Unable to deserialize data.

Cause

The detached check in CloneSerializer::dumpIfTerminal set SerializationReturnCode::ValidationError, which maybeThrowExceptionIfSerializationFailed maps to a TypeError:

if (auto* arrayBuffer = toPossiblySharedArrayBuffer(vm, obj)) {
    if (arrayBuffer->isDetached()) {
        code = SerializationReturnCode::ValidationError;  // -> TypeError

Upstream WebKit uses DataCloneError at this exact site. Bun's port of SerializedScriptValue.cpp (#3637, 2023) predates that upstream change and kept the old code.

Fix

Set SerializationReturnCode::DataCloneError instead. One line; no wire format change.

This covers every entry point that shares the serializer: structuredClone, Worker/MessagePort postMessage, bun:jsc serialize, and node:v8 serialize.

Detached ArrayBuffers reached through a typed array or DataView already threw DataCloneError (they hit the isOutOfBounds check first), as did a detached buffer passed in the transfer list; only a direct reference to a detached ArrayBuffer in the serialized graph was wrong.

Not changed: a MessagePort that appears in the graph but not in the transfer list still throws TypeError. That path also uses ValidationError, but it matches current upstream WebKit, so it is out of scope here.

Verification

The new test in test/js/web/workers/structured-clone.test.ts serializes a detached ArrayBuffer bare, inside an object, inside an array, and inside a Map, and asserts the thrown value is a DOMException named DataCloneError with code === DOMException.DATA_CLONE_ERR.

Before (unfixed) and after
# USE_SYSTEM_BUN=1 bun test structured-clone.test.ts -t "Serializing a detached ArrayBuffer"
error: expect(received).toBeInstanceOf(expected)
Expected constructor: [class DOMException]
TypeError: Unable to deserialize data.
(fail) structuredClone > transferables > Serializing a detached ArrayBuffer throws DataCloneError

# bun bd test structured-clone.test.ts
162 pass, 0 fail

Serializing a detached ArrayBuffer through structuredClone, postMessage, or
bun:jsc's serialize threw a plain TypeError ("Unable to deserialize data.").
Per the HTML structured serialize algorithm it must be a DataCloneError
DOMException, and Node, Chrome, Firefox, and Safari all agree.

The detached check in CloneSerializer::dumpIfTerminal set
SerializationReturnCode::ValidationError, which maps to TypeError. Upstream
WebKit uses DataCloneError at this site; Bun's 2023 port of the file predates
that change. Set DataCloneError instead.

Detached ArrayBuffers reached through a typed array or DataView already took
the isOutOfBounds path and threw DataCloneError, as did a detached buffer in
the transfer list; only the direct ArrayBuffer reference was wrong.
@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 11 minutes and 55 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 60de4a2d-bbc1-46c0-9bb0-95f67f3c187a

📥 Commits

Reviewing files that changed from the base of the PR and between e384998 and cf36dcf.

📒 Files selected for processing (2)
  • src/jsc/bindings/webcore/SerializedScriptValue.cpp
  • test/js/web/workers/structured-clone.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jun 27, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 7:50 PM PT - Jun 26th, 2026

❌ @robobun, your commit cf36dcf has some failures in Build #65217 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 32799

That installs a local version of the PR into your bun-32799 executable, so you can run:

bun-32799 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — one-line enum swap aligning the detached-ArrayBuffer error with the HTML spec and upstream WebKit, with a focused regression test.

Extended reasoning...

Overview

This PR changes a single enum value in CloneSerializer::dumpIfTerminal (src/jsc/bindings/webcore/SerializedScriptValue.cpp): when a detached ArrayBuffer is encountered during serialization, it now sets SerializationReturnCode::DataCloneError instead of ValidationError. I confirmed in maybeThrowExceptionIfSerializationFailed that DataCloneError maps to throwDataCloneError (a DOMException with name === "DataCloneError") while ValidationError mapped to a generic TypeError. A regression test in test/js/web/workers/structured-clone.test.ts exercises bare, object-wrapped, array-wrapped, and Map-wrapped detached buffers and asserts the exception class, name, and code.

Security risks

None. This only changes which exception subclass is thrown on an existing error path; no new data is exposed, no auth/crypto/permission logic is touched, and the serialization wire format is unchanged.

Level of scrutiny

Low. The functional change is literally one identifier on one line, plus a comment citing the relevant WHATWG spec step. The PR description correctly notes that upstream WebKit, Node, Chrome, Firefox, and Safari all throw DataCloneError here, so this is a straightforward spec/compat alignment rather than a design decision. The new test sits inside the existing structuredClone-only transferables block and follows the established style of adjacent tests.

Other factors

No bugs were flagged by the bug-hunting system, there are no outstanding human review comments, and the PR author already verified locally that the full structured-clone.test.ts suite passes (162 pass, 0 fail). The change is self-contained and easy to revert if anything unexpected surfaces in CI.

@robobun

robobun commented Jun 27, 2026 •

Copy link
Copy Markdown
Collaborator Author

CI status on this PR (build 65217, now complete): the diff is green everywhere it is exercised, and every red lane is unrelated to it. I am not going to push retrigger commits, so here is the breakdown for whoever looks at the checks.

Exercising this diff (all green)

  • test/js/web/workers/structured-clone.test.ts: 162 pass, 0 fail on the Debian 13 x64 ASAN test lane (job), and it appears in no failing shard.
  • 277 test/build jobs passed. Format, lint, and the review bot are green.
  • CI's own failure annotations contain no [new] failures for this PR; the only test-failure annotation is the flaky list, all of which passed on runner retry.
  • Fail-before is proven: on the stock/system Bun the new test throws TypeError: Unable to deserialize data. and fails; with the fix it passes.

Red lanes (5 jobs, none touched by this diff)

  1. :windows: 2019 x64, :windows: 2019 x64-baseline, :windows: 11 aarch64 - test-bun: all three fail the same way, test/js/bun/sourcemap/internal-sourcemap-roundtrip.test.ts (6 fail) asserting sources: ["../in.js"] but getting "..\in.js". This is broken on main: Don't panic generating a sourcemap for a source ending in a truncated UTF-8 sequence #32774 (990be52) added a test that pins the exact sources value, which has never been correct on Windows (Generated JS sourcemap includes unwanted relative difference between input and output folders #14769). The fix is open as Use forward slashes in source map sources on Windows #32797 and has not merged, so every Windows test lane is red on every PR until it does. That it reproduces identically on all three Windows lanes (and nowhere else) rules out flake; it is a deterministic main breakage, which is also why I am not burning CI on a retrigger.
  2. :darwin: 26 aarch64 - test-bun (x2): buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'. The darwin build step itself passed; the test job could not fetch the artifact. Infra.

The change is a one-line enum swap in SerializedScriptValue.cpp plus its test, so it has no plausible reach into sourcemaps, bun add, the REPL, or artifact downloads. This is ready to review and merge; a rebase once #32797 lands should turn the Windows lanes green.

@Jarred-Sumner
Jarred-Sumner merged commit c2b5cef into main Jun 27, 2026
74 of 79 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/4e69f772/detached-arraybuffer-datacloneerror branch June 27, 2026 01:18
robobun added a commit that referenced this pull request Jun 27, 2026
The previous commit accidentally picked up the detached-ArrayBuffer hunk
that already landed on main as #32799. This PR does not change
SerializedScriptValue.cpp.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants