Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 78a1570

Browse filesBrowse files
legendecaslegendecas
authored
src: avoid making JSTransferable wrapper object weak
JSTransferable wrapper object is a short-lived wrapper in the scope of the serialization or the deserialization. Make the JSTransferable wrapper object pointer as a strongly-referenced detached BaseObjectPtr so that a JSTransferable wrapper object and its target object will never be garbage-collected during a ser-des process, and the wrapper object will be immediately destroyed when the process is completed. PR-URL: #50026 Fixes: #49852 Fixes: #49844 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent 6b76b77 commit 78a1570
Copy full SHA for 78a1570

File tree

Expand file treeCollapse file tree

3 files changed

+37
-19
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

3 files changed

+37
-19
lines changed
Open diff view settings
Collapse file

‎src/node_messaging.cc‎

Copy file name to clipboardExpand all lines: src/node_messaging.cc
+22-14Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -321,8 +321,9 @@ class SerializerDelegate : public ValueSerializer::Delegate {
321321
}
322322

323323
if (JSTransferable::IsJSTransferable(env_, context_, object)) {
324-
JSTransferable* js_transferable = JSTransferable::Wrap(env_, object);
325-
return WriteHostObject(BaseObjectPtr<BaseObject>{js_transferable});
324+
BaseObjectPtr<JSTransferable> js_transferable =
325+
JSTransferable::Wrap(env_, object);
326+
return WriteHostObject(js_transferable);
326327
}
327328

328329
// Convert process.env to a regular object.
@@ -536,8 +537,7 @@ Maybe<bool> Message::Serialize(Environment* env,
536537
ThrowDataCloneException(context, env->clone_untransferable_str());
537538
return Nothing<bool>();
538539
}
539-
JSTransferable* js_transferable = JSTransferable::Wrap(env, entry);
540-
host_object = BaseObjectPtr<BaseObject>{js_transferable};
540+
host_object = JSTransferable::Wrap(env, entry);
541541
}
542542

543543
if (env->message_port_constructor_template()->HasInstance(entry) &&
@@ -1190,22 +1190,26 @@ Local<FunctionTemplate> GetMessagePortConstructorTemplate(Environment* env) {
11901190
}
11911191

11921192
// static
1193-
JSTransferable* JSTransferable::Wrap(Environment* env, Local<Object> target) {
1193+
BaseObjectPtr<JSTransferable> JSTransferable::Wrap(Environment* env,
1194+
Local<Object> target) {
11941195
Local<Context> context = env->context();
11951196
Local<Value> wrapper_val =
11961197
target->GetPrivate(context, env->js_transferable_wrapper_private_symbol())
11971198
.ToLocalChecked();
11981199
DCHECK(wrapper_val->IsObject() || wrapper_val->IsUndefined());
1199-
JSTransferable* wrapper;
1200+
BaseObjectPtr<JSTransferable> wrapper;
12001201
if (wrapper_val->IsObject()) {
1201-
wrapper = Unwrap<JSTransferable>(wrapper_val);
1202+
wrapper =
1203+
BaseObjectPtr<JSTransferable>{Unwrap<JSTransferable>(wrapper_val)};
12021204
} else {
12031205
Local<Object> wrapper_obj = env->js_transferable_constructor_template()
12041206
->GetFunction(context)
12051207
.ToLocalChecked()
12061208
->NewInstance(context)
12071209
.ToLocalChecked();
1208-
wrapper = new JSTransferable(env, wrapper_obj, target);
1210+
// Make sure the JSTransferable wrapper object is not garbage collected
1211+
// until the strong BaseObjectPtr's reference count is decreased to 0.
1212+
wrapper = MakeDetachedBaseObject<JSTransferable>(env, wrapper_obj, target);
12091213
target
12101214
->SetPrivate(
12111215
context, env->js_transferable_wrapper_private_symbol(), wrapper_obj)
@@ -1226,12 +1230,18 @@ JSTransferable::JSTransferable(Environment* env,
12261230
Local<Object> obj,
12271231
Local<Object> target)
12281232
: BaseObject(env, obj) {
1229-
MakeWeak();
12301233
target_.Reset(env->isolate(), target);
1231-
target_.SetWeak();
1234+
}
1235+
1236+
JSTransferable::~JSTransferable() {
1237+
HandleScope scope(env()->isolate());
1238+
target_.Get(env()->isolate())
1239+
->DeletePrivate(env()->context(),
1240+
env()->js_transferable_wrapper_private_symbol());
12321241
}
12331242

12341243
Local<Object> JSTransferable::target() const {
1244+
DCHECK(!target_.IsEmpty());
12351245
return target_.Get(env()->isolate());
12361246
}
12371247

@@ -1335,8 +1345,7 @@ JSTransferable::NestedTransferables() const {
13351345
if (!JSTransferable::IsJSTransferable(env(), context, obj)) {
13361346
continue;
13371347
}
1338-
JSTransferable* js_transferable = JSTransferable::Wrap(env(), obj);
1339-
ret.emplace_back(js_transferable);
1348+
ret.emplace_back(JSTransferable::Wrap(env(), obj));
13401349
}
13411350
return Just(ret);
13421351
}
@@ -1397,8 +1406,7 @@ BaseObjectPtr<BaseObject> JSTransferable::Data::Deserialize(
13971406
if (!JSTransferable::IsJSTransferable(env, context, ret.As<Object>())) {
13981407
return {};
13991408
}
1400-
JSTransferable* js_transferable = JSTransferable::Wrap(env, ret.As<Object>());
1401-
return BaseObjectPtr<BaseObject>{js_transferable};
1409+
return JSTransferable::Wrap(env, ret.As<Object>());
14021410
}
14031411

14041412
Maybe<bool> JSTransferable::Data::FinalizeTransferWrite(
Collapse file

‎src/node_messaging.h‎

Copy file name to clipboardExpand all lines: src/node_messaging.h
+7-5Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -324,11 +324,17 @@ class MessagePort : public HandleWrap {
324324
// See e.g. FileHandle in internal/fs/promises.js for an example.
325325
class JSTransferable : public BaseObject {
326326
public:
327-
static JSTransferable* Wrap(Environment* env, v8::Local<v8::Object> target);
327+
static BaseObjectPtr<JSTransferable> Wrap(Environment* env,
328+
v8::Local<v8::Object> target);
328329
static bool IsJSTransferable(Environment* env,
329330
v8::Local<v8::Context> context,
330331
v8::Local<v8::Object> object);
331332

333+
JSTransferable(Environment* env,
334+
v8::Local<v8::Object> obj,
335+
v8::Local<v8::Object> target);
336+
~JSTransferable();
337+
332338
BaseObject::TransferMode GetTransferMode() const override;
333339
std::unique_ptr<TransferData> TransferForMessaging() override;
334340
std::unique_ptr<TransferData> CloneForMessaging() const override;
@@ -345,10 +351,6 @@ class JSTransferable : public BaseObject {
345351
v8::Local<v8::Object> target() const;
346352

347353
private:
348-
JSTransferable(Environment* env,
349-
v8::Local<v8::Object> obj,
350-
v8::Local<v8::Object> target);
351-
352354
template <TransferMode mode>
353355
std::unique_ptr<TransferData> TransferOrClone() const;
354356

Collapse file

‎test/pummel/test-structuredclone-jstransferable.js‎

Copy file name to clipboard
+8Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
// Flags: --max-old-space-size=10
2+
'use strict';
3+
require('../common');
4+
const { createHistogram } = require('perf_hooks');
5+
6+
for (let i = 0; i < 1e4; i++) {
7+
structuredClone(createHistogram());
8+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.