Skip to content

Commit cf36bd1

Browse files
fix(runtime): keep clone-only message reads read-only, and see an uncloneable 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.
1 parent 42a8bcf commit cf36bd1

3 files changed

Lines changed: 48 additions & 4 deletions

File tree

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// A fresh isolate: the markAsUncloneable call in the getter below is the first
2+
// one this isolate has seen, and it happens while the clone that reaches the
3+
// marked object is already being written.
4+
var markAsUncloneable = require("node:worker_threads").markAsUncloneable;
5+
var graph = {
6+
get inner() {
7+
var marked = { a: 1 };
8+
markAsUncloneable(marked);
9+
return marked;
10+
},
11+
};
12+
var result;
13+
try {
14+
structuredClone(graph);
15+
result = { threw: false };
16+
} catch (e) {
17+
result = { threw: true, name: e && e.name };
18+
}
19+
postMessage(result);

‎test-app/app/src/main/assets/app/tests/testMessaging.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,22 @@ describe("Messaging runtime edges", function () {
204204
});
205205
});
206206

207+
describe("markAsUncloneable", function () {
208+
it("rejects an object marked while the clone that reaches it is written", function (done) {
209+
var worker = new Worker("./messaging/uncloneableInGetterWorker.js");
210+
worker.onmessage = function (event) {
211+
expect(event.data).toEqual({ threw: true, name: "DataCloneError" });
212+
worker.terminate();
213+
done();
214+
};
215+
worker.onerror = function (error) {
216+
fail("worker error: " + error.message);
217+
worker.terminate();
218+
done();
219+
};
220+
});
221+
});
222+
207223
describe("worker error reporting", function () {
208224
// A worker boots on its own thread, so the first error arrives whenever
209225
// the runner gets to it; specs wait for it and only then settle for

‎test-app/runtime/src/main/cpp/StructuredSerialization.cpp‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,11 @@ class SerializerDelegate : public ValueSerializer::Delegate {
172172
if (object->InternalFieldCount() > 0) {
173173
return Just(true);
174174
}
175+
if (uncloneableBrand_.IsEmpty()) {
176+
// markAsUncloneable creates the brand on its first call in an
177+
// isolate, which a getter in the graph being written can make.
178+
uncloneableBrand_ = messaging::UncloneableBrandIfAny(isolate);
179+
}
175180
if (!uncloneableBrand_.IsEmpty()) {
176181
bool uncloneable = false;
177182
if (!object->HasPrivate(isolate->GetCurrentContext(), uncloneableBrand_)
@@ -635,7 +640,8 @@ MaybeLocal<Value> SerializedValue::Deserialize(Isolate* isolate, Local<Context>
635640
"A message carrying transferred objects can only be read once.");
636641
return MaybeLocal<Value>();
637642
}
638-
if (HasTransferables()) {
643+
const bool singleReceiver = HasTransferables();
644+
if (singleReceiver) {
639645
consumed_ = true;
640646
}
641647

@@ -728,9 +734,12 @@ MaybeLocal<Value> SerializedValue::Deserialize(Isolate* isolate, Local<Context>
728734
ArrayBuffer::New(isolate, std::move(transferredBuffers_[i])));
729735
}
730736
// Handed over above; the vectors would otherwise keep reporting
731-
// transferables that are no longer here.
732-
transferredBuffers_.clear();
733-
transferredPorts_.clear();
737+
// transferables that are no longer here. Only the single receiver writes
738+
// them: the other readers share this value with no lock.
739+
if (singleReceiver) {
740+
transferredBuffers_.clear();
741+
transferredPorts_.clear();
742+
}
734743

735744
Local<Value> result;
736745
{

0 commit comments

Comments
 (0)