Skip to content

structured clone - #3637

Merged
dylan-conway merged 15 commits into
mainfrom
dylan/structured-clone
Jul 15, 2023
Merged

dylan-conway merged 15 commits into
mainfrom
dylan/structured-clone

Conversation

@dylan-conway

Copy link
Copy Markdown
Member

No description provided.

@github-actions

github-actions Bot commented Jul 15, 2023 •

Copy link
Copy Markdown
Contributor

✅ prettier errors have been resolved. Thank you.

#642aa2204f2ecbbff33afb07d9559d3588b21754

Comment thread src/bun.js/bindings/ZigGlobalObject.cpp Outdated
Comment thread src/bun.js/bindings/ZigGlobalObject.cpp Outdated
Comment thread src/bun.js/bindings/ZigGlobalObject.cpp Outdated
Comment thread src/bun.js/node/types.zig
Comment thread src/bun.js/webcore/blob.zig Outdated
Comment thread src/bun.js/webcore/blob.zig
Comment thread src/bun.js/webcore/blob.zig
Comment thread src/bun.js/webcore/blob.zig Outdated
@github-actions

Copy link
Copy Markdown
Contributor

❌ @dylan-conway 7 files with test failures on bun-darwin-aarch64:

  • test/cli/install/bun-link.test.ts
  • test/cli/install/bun-run.test.ts
  • test/js/bun/net/socket.test.ts
  • test/js/bun/spawn/spawn.test.ts
  • test/js/bun/test/test-test.test.ts
  • test/js/node/watch/fs.watch.test.ts
  • test/js/web/fetch/fetch-leak.test.js

View test output

#642aa2204f2ecbbff33afb07d9559d3588b21754

@github-actions

Copy link
Copy Markdown
Contributor

❌ @dylan-conway 4 files with test failures on linux-x64-baseline:

  • test/cli/install/bun-link.test.ts
  • test/cli/install/bun-run.test.ts
  • test/js/node/watch/fs.watch.test.ts
  • test/js/web/fetch/fetch-leak.test.js

View test output

#642aa2204f2ecbbff33afb07d9559d3588b21754

@github-actions

Copy link
Copy Markdown
Contributor

❌ @dylan-conway 4 files with test failures on linux-x64:

  • test/cli/install/bun-link.test.ts
  • test/cli/install/bun-run.test.ts
  • test/js/node/watch/fs.watch.test.ts
  • test/js/web/fetch/fetch-leak.test.js

View test output

#642aa2204f2ecbbff33afb07d9559d3588b21754

@github-actions

Copy link
Copy Markdown
Contributor

❌ @dylan-conway 7 files with test failures on bun-darwin-x64-baseline:

  • test/cli/install/bun-link.test.ts
  • test/cli/install/bun-run.test.ts
  • test/cli/install/bunx.test.ts
  • test/js/bun/spawn/spawn-streaming-stdin.test.ts
  • test/js/bun/sqlite/sqlite.test.js
  • test/js/third_party/webpack/webpack.test.ts
  • test/js/web/timers/setTimeout.test.js

View test output

#642aa2204f2ecbbff33afb07d9559d3588b21754

@dylan-conway
dylan-conway merged commit c39c11e into main Jul 15, 2023
@dylan-conway
dylan-conway deleted the dylan/structured-clone branch July 15, 2023 02:37
Jarred-Sumner pushed a commit that referenced this pull request Jun 27, 2026
)

## Summary

Serializing a graph that contains a detached `ArrayBuffer` threw a plain
`TypeError` instead of a `"DataCloneError"` `DOMException`. The [HTML
structured serialize
algorithm](https://html.spec.whatwg.org/multipage/structured-data.html#structuredserializeinternal)
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

```js
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`:

```cpp
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`.

<details>
<summary>Before (unfixed) and after</summary>

```
# 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
```

</details>
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.

2 participants