Skip to content

Fix ArrayBuffer transfer after failed structured clone - #5612

Open
mansiverma897993 wants to merge 2 commits into
boa-dev:mainfrom
mansiverma897993:codex/fix-structured-clone-transfer
Open

mansiverma897993 wants to merge 2 commits into
boa-dev:mainfrom
mansiverma897993:codex/fix-structured-clone-transfer

Conversation

@mansiverma897993

Copy link
Copy Markdown
Contributor

Fixes #5609.

Stage ArrayBuffer transfers until serialization succeeds, so a failed clone leaves buffers attached. Reject duplicate and detached transfer entries, and detach listed buffers even when they are unused.

Tests: cargo test -p boa_wintertc --lib (60 passed); cargo test -p boa_runtime --lib (108 passed); cargo fmt --all --check; cargo clippy -p boa_wintertc --all-targets -- -D warnings.

@mansiverma897993
mansiverma897993 requested a review from a team as a code owner October 24, 2026 09:12
@github-actions github-actions Bot added the Waiting On Review Waiting on reviews from the maintainers label Oct 24, 2026
@github-actions github-actions Bot added this to the v0.23 milestone Oct 24, 2026
@github-actions github-actions Bot added the C-Tests Issues and PRs related to the tests. label Oct 24, 2026
@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Test262 conformance changes

Test result main count PR count difference
Total 53,578 53,578 0
Passed 51,450 51,450 0
Ignored 1,648 1,648 0
Failed 480 480 0
Panics 0 0 0
Conformance 96.03% 96.03% 0.00%

Tested main commit: 9871775a4999591e27bbb026b92530bf43caef52
Tested PR commit: 89adadff43a86ae9ee8fe80d5763fcff61cece9a
Compare commits: 9871775...89adadf

@codecov

codecov Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.11%. Comparing base (6ddc2b4) to head (89adadf).
⚠️ Report is 1078 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5612       +/-   ##
===========================================
+ Coverage   47.24%   63.11%   +15.86%     
===========================================
  Files         476      536       +60     
  Lines       46892    60538    +13646     
===========================================
+ Hits        22154    38207    +16053     
+ Misses      24738    22331     -2407     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@shreyashsri79

Copy link
Copy Markdown

AI-assisted cloud probes confirm the original fix: when serialization fails on a function, the 8-byte buffer (first byte 42) is detached on main 9871775, but remains intact on PR 998c5bd.

One question about the all-buffer precheck: is keeping every other buffer attached an intentional guarantee when a getter detaches a transfer entry?

const a = new ArrayBuffer(8), b = new ArrayBuffer(8);
let error = null;
try {
  structuredClone({
    a,
    get value() {
      structuredClone(b, { transfer: [b] });
      return true;
    }
  }, { transfer: [a, b] });
} catch (e) { error = e.name; }
console.log(error, a.byteLength, b.byteLength);

Separate builds of main and the PR gave:

Runtime Result
Boa main 9871775 null 0 0
PR 998c5bd TypeError 8 0
Node 24.19.0 null 0 0

Reversing the list to [b, a] produced the same result in each runtime. My reading of HTML StructuredSerializeWithTransfer step 5 is that detached checks and transfers happen per entry, so [a, b] would detach a before throwing on b, while [b, a] would leave a attached. Node disagrees with that reading too, so I would not use it as the oracle here.

Which behavior should the getter-detachment regression test codify? These are focused observations, not a full-suite run.

@mansiverma897993

Copy link
Copy Markdown
Contributor Author

Thanks for the probe. The all-buffer precheck went beyond the HTML algorithm: step 5 checks and detaches each transfer entry in list order. I removed that precheck in 89adadf and updated the regression test. With [a, b], a detaches before the already-detached b causes an error; with [b, a], a stays attached. The original serialization-failure test still passes.

@mansiverma897993

Copy link
Copy Markdown
Contributor Author

@jedel1043, could you please review this PR when you have a chance? Thanks!

@shreyashsri79

Copy link
Copy Markdown

Thanks for the correction in 89adadf. AI-assisted source review confirms the transfer loop now runs in list order after successful serialization. The regression asserts both outcomes from the earlier question: [a, b] throws with lengths 0, 0, while [b, a] throws with 8, 0. The original serialization-failure test still asserts that the 8-byte buffer and first byte 42 remain intact.

This addresses my transfer-order concern. This acknowledgment is based on source inspection; I have not rerun tests for this follow-up.

@shreyashsri79

Copy link
Copy Markdown

I prepared a test-only patch extending your duplicate-transfer regression to five invalid-transfer inputs, including a valid buffer followed by an invalid entry. It checks rejection before getter access and preservation of buffer length and bytes. On head 89adadf 61cece9a, all six clone::tests:: tests passed natively on Linux, offline and locked, with 54 unrelated tests filtered out. This was a focused run, not the full suite. Would you like the patch to fold into this PR?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-Tests Issues and PRs related to the tests. Waiting On Review Waiting on reviews from the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

structuredClone detaches transferred ArrayBuffer when serialization later fails

2 participants