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

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.

Author: adrian-niculescu

NativeScript/runtime/StructuredSerialization.cpp

@@ -161,6 +161,11 @@ class SerializerDelegate : public ValueSerializer::Delegate {
     if (object->InternalFieldCount() > 0) {
       return Just(true);
     }
+    if (uncloneableBrand_.IsEmpty()) {
+      // markAsUncloneable creates the brand on its first call in an isolate,
+      // which a getter in the graph being written can make.
+      uncloneableBrand_ = messaging::UncloneableBrandIfAny(isolate);
+    }
     if (!uncloneableBrand_.IsEmpty()) {
       bool uncloneable = false;
       if (!object->HasPrivate(isolate->GetCurrentContext(), uncloneableBrand_)
@@ -622,7 +627,8 @@ MaybeLocal<Value> SerializedValue::Deserialize(Isolate* isolate,
         "A message carrying transferred objects can only be read once.");
     return MaybeLocal<Value>();
   }
-  if (HasTransferables()) {
+  const bool singleReceiver = HasTransferables();
+  if (singleReceiver) {
     consumed_ = true;
   }
 
@@ -716,9 +722,12 @@ MaybeLocal<Value> SerializedValue::Deserialize(Isolate* isolate,
         ArrayBuffer::New(isolate, std::move(transferredBuffers_[i])));
   }
   // Handed over above; the vectors would otherwise keep reporting
-  // transferables that are no longer here.
-  transferredBuffers_.clear();
-  transferredPorts_.clear();
+  // transferables that are no longer here. Only the single receiver writes
+  // them: the other readers share this value with no lock.
+  if (singleReceiver) {
+    transferredBuffers_.clear();
+    transferredPorts_.clear();
+  }
 
   Local<Value> result;
   {

TestRunner/app/tests/MessagingTests.js

@@ -295,6 +295,24 @@ describe("Messaging runtime edges", function () {
         });
     });
 
+    describe("markAsUncloneable", function () {
+        it("rejects an object marked while the clone that reaches it is written", function (done) {
+            var worker = new Worker("./messaging/uncloneableInGetterWorker.js");
+            worker.onmessage = function (event) {
+                expect(event.data).toEqual({ threw: true, name: "DataCloneError" });
+                worker.terminate();
+                done();
+            };
+            // fail() throws in this runner, which would skip done() when called
+            // from an event handler.
+            worker.onerror = function (error) {
+                expect("worker error: " + error.message).toBeNull();
+                worker.terminate();
+                done();
+            };
+        });
+    });
+
     describe("worker error reporting", function () {
         // A worker boots on its own thread, so the first error arrives whenever
         // the runner gets to it; specs wait for it and only then settle for

TestRunner/app/tests/messaging/uncloneableInGetterWorker.js

@@ -0,0 +1,19 @@
+// A fresh isolate: the markAsUncloneable call in the getter below is the first
+// one this isolate has seen, and it happens while the clone that reaches the
+// marked object is already being written.
+var markAsUncloneable = require("node:worker_threads").markAsUncloneable;
+var graph = {
+    get inner() {
+        var marked = { a: 1 };
+        markAsUncloneable(marked);
+        return marked;
+    },
+};
+var result;
+try {
+    structuredClone(graph);
+    result = { threw: false };
+} catch (e) {
+    result = { threw: true, name: e && e.name };
+}
+postMessage(result);