From 78a15702dd1bc03505ee130b0ad999ee292b6581 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Tue, 10 Oct 2023 02:42:30 -0500 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/50026 Fixes: https://github.com/nodejs/node/issues/49852 Fixes: https://github.com/nodejs/node/issues/49844 Reviewed-By: Yagiz Nizipli Reviewed-By: Matteo Collina --- src/node_messaging.cc | 36 +++++++++++-------- src/node_messaging.h | 12 ++++--- .../test-structuredclone-jstransferable.js | 8 +++++ 3 files changed, 37 insertions(+), 19 deletions(-) create mode 100644 test/pummel/test-structuredclone-jstransferable.js diff --git a/src/node_messaging.cc b/src/node_messaging.cc index 920d1f60843b3c..6d2cbf8726c4e4 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -321,8 +321,9 @@ class SerializerDelegate : public ValueSerializer::Delegate { } if (JSTransferable::IsJSTransferable(env_, context_, object)) { - JSTransferable* js_transferable = JSTransferable::Wrap(env_, object); - return WriteHostObject(BaseObjectPtr{js_transferable}); + BaseObjectPtr js_transferable = + JSTransferable::Wrap(env_, object); + return WriteHostObject(js_transferable); } // Convert process.env to a regular object. @@ -536,8 +537,7 @@ Maybe Message::Serialize(Environment* env, ThrowDataCloneException(context, env->clone_untransferable_str()); return Nothing(); } - JSTransferable* js_transferable = JSTransferable::Wrap(env, entry); - host_object = BaseObjectPtr{js_transferable}; + host_object = JSTransferable::Wrap(env, entry); } if (env->message_port_constructor_template()->HasInstance(entry) && @@ -1190,22 +1190,26 @@ Local GetMessagePortConstructorTemplate(Environment* env) { } // static -JSTransferable* JSTransferable::Wrap(Environment* env, Local target) { +BaseObjectPtr JSTransferable::Wrap(Environment* env, + Local target) { Local context = env->context(); Local wrapper_val = target->GetPrivate(context, env->js_transferable_wrapper_private_symbol()) .ToLocalChecked(); DCHECK(wrapper_val->IsObject() || wrapper_val->IsUndefined()); - JSTransferable* wrapper; + BaseObjectPtr wrapper; if (wrapper_val->IsObject()) { - wrapper = Unwrap(wrapper_val); + wrapper = + BaseObjectPtr{Unwrap(wrapper_val)}; } else { Local wrapper_obj = env->js_transferable_constructor_template() ->GetFunction(context) .ToLocalChecked() ->NewInstance(context) .ToLocalChecked(); - wrapper = new JSTransferable(env, wrapper_obj, target); + // Make sure the JSTransferable wrapper object is not garbage collected + // until the strong BaseObjectPtr's reference count is decreased to 0. + wrapper = MakeDetachedBaseObject(env, wrapper_obj, target); target ->SetPrivate( context, env->js_transferable_wrapper_private_symbol(), wrapper_obj) @@ -1226,12 +1230,18 @@ JSTransferable::JSTransferable(Environment* env, Local obj, Local target) : BaseObject(env, obj) { - MakeWeak(); target_.Reset(env->isolate(), target); - target_.SetWeak(); +} + +JSTransferable::~JSTransferable() { + HandleScope scope(env()->isolate()); + target_.Get(env()->isolate()) + ->DeletePrivate(env()->context(), + env()->js_transferable_wrapper_private_symbol()); } Local JSTransferable::target() const { + DCHECK(!target_.IsEmpty()); return target_.Get(env()->isolate()); } @@ -1335,8 +1345,7 @@ JSTransferable::NestedTransferables() const { if (!JSTransferable::IsJSTransferable(env(), context, obj)) { continue; } - JSTransferable* js_transferable = JSTransferable::Wrap(env(), obj); - ret.emplace_back(js_transferable); + ret.emplace_back(JSTransferable::Wrap(env(), obj)); } return Just(ret); } @@ -1397,8 +1406,7 @@ BaseObjectPtr JSTransferable::Data::Deserialize( if (!JSTransferable::IsJSTransferable(env, context, ret.As())) { return {}; } - JSTransferable* js_transferable = JSTransferable::Wrap(env, ret.As()); - return BaseObjectPtr{js_transferable}; + return JSTransferable::Wrap(env, ret.As()); } Maybe JSTransferable::Data::FinalizeTransferWrite( diff --git a/src/node_messaging.h b/src/node_messaging.h index d1c16bc1c03cb7..a32d477c2e2f79 100644 --- a/src/node_messaging.h +++ b/src/node_messaging.h @@ -324,11 +324,17 @@ class MessagePort : public HandleWrap { // See e.g. FileHandle in internal/fs/promises.js for an example. class JSTransferable : public BaseObject { public: - static JSTransferable* Wrap(Environment* env, v8::Local target); + static BaseObjectPtr Wrap(Environment* env, + v8::Local target); static bool IsJSTransferable(Environment* env, v8::Local context, v8::Local object); + JSTransferable(Environment* env, + v8::Local obj, + v8::Local target); + ~JSTransferable(); + BaseObject::TransferMode GetTransferMode() const override; std::unique_ptr TransferForMessaging() override; std::unique_ptr CloneForMessaging() const override; @@ -345,10 +351,6 @@ class JSTransferable : public BaseObject { v8::Local target() const; private: - JSTransferable(Environment* env, - v8::Local obj, - v8::Local target); - template std::unique_ptr TransferOrClone() const; diff --git a/test/pummel/test-structuredclone-jstransferable.js b/test/pummel/test-structuredclone-jstransferable.js new file mode 100644 index 00000000000000..e537b8c3458708 --- /dev/null +++ b/test/pummel/test-structuredclone-jstransferable.js @@ -0,0 +1,8 @@ +// Flags: --max-old-space-size=10 +'use strict'; +require('../common'); +const { createHistogram } = require('perf_hooks'); + +for (let i = 0; i < 1e4; i++) { + structuredClone(createHistogram()); +}