Skip to content

fix(runtime): keep clone-only message reads read-only, and see an uncloneable brand created mid-clone - #2064

Open
adrian-niculescu wants to merge 1 commit into
NativeScript:feat/worker-threadsfrom
adrian-niculescu:fix/serialization-shared-reads
Open

adrian-niculescu wants to merge 1 commit into
NativeScript:feat/worker-threadsfrom
adrian-niculescu:fix/serialization-shared-reads

Conversation

@adrian-niculescu

@adrian-niculescu adrian-niculescu commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

An object marked with markAsUncloneable from a getter, during the clone that reaches it, is cloned instead of throwing DataCloneError when that is the first markAsUncloneable call in the isolate. The serializer reads the brand once, when it is constructed, and at that point the brand does not exist yet. It now picks the brand up once markAsUncloneable creates it.

Separately, SerializedValue::Deserialize cleared its transfer vectors on every read. A clone-only message is read by every BroadcastChannel receiver, and every getEnvironmentData call, with no lock, so those clears were concurrent writes to shared state, against what the comment on the consumed_ flag intends. Only the single receiver of a message with transferables writes them now.

Stacked on #2043; the same fix for iOS is NativeScript/ios#490. The new markAsUncloneable spec fails on that branch and passes here, and the full device suite passes. The deserialization race has no deterministic spec.

Summary by CodeRabbit

  • Bug Fixes
    • Structured cloning now correctly reports a DataCloneError when it encounters an object marked as uncloneable during serialization.
    • Deserialized values with transferables remain single-use, while values without transferables can be read without triggering transferable cleanup.

…loneable brand created mid-clone

A clone-only message is read concurrently by every BroadcastChannel receiver and every getEnvironmentData call, and deserializing it cleared its transfer vectors, a write. Only the single receiver of a message with transferables writes them now.

The serializer cached the uncloneable brand when it was created, so the first markAsUncloneable call in an isolate, made by a getter in the graph being written, went unseen and the marked object was cloned. It now picks the brand up once it exists.
@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 251f22e8-9341-43d9-9993-1b569a175846
📥 Commits

Reviewing files that changed from the base of the PR and between 42a8bcf and cf36bd1.

📒 Files selected for processing (3)
  • test-app/app/src/main/assets/app/tests/messaging/uncloneableInGetterWorker.js
  • test-app/app/src/main/assets/app/tests/testMessaging.js
  • test-app/runtime/src/main/cpp/StructuredSerialization.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

The change updates structured serialization to detect an uncloneable brand created while a getter runs during cloning, and to apply single-consumer handling and transferable-storage cleanup only when transferables are present. It adds a worker regression test for the clone error.

Changes

Structured serialization

Layer / File(s) Summary
Detect brands created during cloning
test-app/runtime/src/main/cpp/StructuredSerialization.cpp, test-app/app/src/main/assets/app/tests/messaging/uncloneableInGetterWorker.js, test-app/app/src/main/assets/app/tests/testMessaging.js
IsHostObject refreshes an empty cached uncloneable brand. A worker regression test checks that marking an object uncloneable from a getter causes a DataCloneError.
Condition cleanup on transferables
test-app/runtime/src/main/cpp/StructuredSerialization.cpp
Deserialize uses the presence of transferables to control single-consumer marking and cleanup of transferred buffer and port storage.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Suggested reviewers: edusperoni

Merge Risk: ⚪ Minimal · up to cf36b

No merge-blocking issue is established; the change is ready to merge after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title describes both main runtime fixes: read-only clone-only message reads and detection of an uncloneable brand created during cloning.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit watched the getter run,
Then saw the clone report its error.
With transferables, storage clears;
Without them, vectors stay in place.
The rabbit hops through tests with cheer.

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

@adrian-niculescu

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant