fix(runtime): keep clone-only message reads read-only, and see an uncloneable brand created mid-clone - #490
Conversation
Sorry, something went wrong.
…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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configuration
Reviewing files that changed from the base of the PR and between 4ae32eb and 7d20c28. 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 Walkthrough WalkthroughStructured serialization now checks an uncloneable brand on demand during graph writing. Deserialization uses transferables state to control consumed-state handling and transferred-buffer and transferred-port cleanup. A worker regression test covers an object marked uncloneable by a getter during cloning. ChangesUncloneable objects during cloning
Transferable cleanup during deserialization
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: edusperoni Merge Risk: ⚪ Minimal · up to 7d20c The change makes clone-only message reads read-only and detects an object marked uncloneable during cloning. No actionable merge risk was found. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
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. ❤️ ShareA rabbit checks each cloned leaf, Comment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
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.
The same fix for Android is NativeScript/android#2064. The new markAsUncloneable spec fails on main and passes here, and the full TestRunner suite passes. The deserialization race has no deterministic spec.
Summary by CodeRabbit