From 1b35c76366768a1b0dde561b8cd6e6a08ee950b2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 29 May 2026 11:50:32 +0200 Subject: [PATCH 1/6] src: add cleanup hooks to `node::ObjectWrap` Add cleanup hooks to make sure that addons making use of this legacy API can do so safely in Worker threads or embedder-controlled Node.js instances. Refs: https://github.com/nodejs/node/pull/63575 Fixes: https://github.com/nodejs/node/issues/63540 Signed-off-by: Anna Henningsen --- src/node_object_wrap.h | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/node_object_wrap.h b/src/node_object_wrap.h index cf8df3af48a12f..c8ce37fcbd8891 100644 --- a/src/node_object_wrap.h +++ b/src/node_object_wrap.h @@ -22,7 +22,7 @@ #ifndef SRC_NODE_OBJECT_WRAP_H_ #define SRC_NODE_OBJECT_WRAP_H_ -#include "v8.h" +#include "node.h" #include @@ -32,10 +32,12 @@ class ObjectWrap { public: ObjectWrap() { refs_ = 0; + AddCleanupHook(); } virtual ~ObjectWrap() { + RemoveCleanupHook(); if (persistent().IsEmpty()) return; persistent().ClearWeak(); @@ -125,6 +127,18 @@ class ObjectWrap { delete wrap; } + void AddCleanupHook() { + AddEnvironmentCleanupHook(v8::Isolate::GetCurrent(), CleanupHook, this); + } + + void RemoveCleanupHook() { + RemoveEnvironmentCleanupHook(v8::Isolate::GetCurrent(), CleanupHook, this); + } + + static void CleanupHook(void* arg) { + delete static_cast(arg); + } + // NOLINTNEXTLINE(runtime/v8_persistent) v8::Persistent handle_; }; From 5e12408a1be9f85d5e4aa9ba3e46acfa0d8cf4a2 Mon Sep 17 00:00:00 2001 From: Mohamed Akram Date: Tue, 26 May 2026 04:18:14 +0400 Subject: [PATCH 2/6] test: add regression test for using `ObjectWrap` in worker This content is taken from https://github.com/nodejs/node/pull/63575. The original commit message is preserved below: --- worker: fix premature addon unload Weak callbacks could run after addons were unloaded, leading to a crash. Signed-off-by: Mohamed Akram --- test/addons/worker-addon-exit/binding.cc | 99 +++++++++++++++++++++++ test/addons/worker-addon-exit/binding.gyp | 9 +++ test/addons/worker-addon-exit/test.js | 19 +++++ 3 files changed, 127 insertions(+) create mode 100644 test/addons/worker-addon-exit/binding.cc create mode 100644 test/addons/worker-addon-exit/binding.gyp create mode 100644 test/addons/worker-addon-exit/test.js diff --git a/test/addons/worker-addon-exit/binding.cc b/test/addons/worker-addon-exit/binding.cc new file mode 100644 index 00000000000000..312534fa4c7440 --- /dev/null +++ b/test/addons/worker-addon-exit/binding.cc @@ -0,0 +1,99 @@ +#include +#include + +using v8::Context; +using v8::Function; +using v8::FunctionCallbackInfo; +using v8::FunctionTemplate; +using v8::Isolate; +using v8::Local; +using v8::Number; +using v8::Object; +using v8::ObjectTemplate; +using v8::String; +using v8::Value; + +class MyObject : public node::ObjectWrap { + public: + static void Init(v8::Local exports); + ~MyObject(); + + private: + explicit MyObject(double value = 0); + + static void New(const v8::FunctionCallbackInfo& args); + static void PlusOne(const v8::FunctionCallbackInfo& args); + + double value_; +}; + +MyObject::MyObject(double value) : value_(value) {} + +MyObject::~MyObject() {} + +void MyObject::Init(Local exports) { + Isolate* isolate = Isolate::GetCurrent(); + Local context = isolate->GetCurrentContext(); + + Local addon_data_tpl = ObjectTemplate::New(isolate); + addon_data_tpl->SetInternalFieldCount(1); // 1 field for the MyObject::New() + Local addon_data = + addon_data_tpl->NewInstance(context).ToLocalChecked(); + + // Prepare constructor template + Local tpl = FunctionTemplate::New(isolate, New, addon_data); + tpl->SetClassName(String::NewFromUtf8(isolate, "MyObject").ToLocalChecked()); + tpl->InstanceTemplate()->SetInternalFieldCount(1); + + // Prototype + NODE_SET_PROTOTYPE_METHOD(tpl, "plusOne", PlusOne); + + Local constructor = tpl->GetFunction(context).ToLocalChecked(); + addon_data->SetInternalField(0, constructor); + exports + ->Set(context, + String::NewFromUtf8(isolate, "MyObject").ToLocalChecked(), + constructor) + .FromJust(); +} + +void MyObject::New(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + Local context = isolate->GetCurrentContext(); + + if (args.IsConstructCall()) { + // Invoked as constructor: `new MyObject(...)` + double value = + args[0]->IsUndefined() ? 0 : args[0]->NumberValue(context).FromMaybe(0); + MyObject* obj = new MyObject(value); + obj->Wrap(args.This()); + args.GetReturnValue().Set(args.This()); + } else { + // Invoked as plain function `MyObject(...)`, turn into construct call. + const int argc = 1; + Local argv[argc] = {args[0]}; + Local cons = args.Data() + .As() + ->GetInternalField(0) + .As() + .As(); + Local result = + cons->NewInstance(context, argc, argv).ToLocalChecked(); + args.GetReturnValue().Set(result); + } +} + +void MyObject::PlusOne(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + + MyObject* obj = ObjectWrap::Unwrap(args.This()); + obj->value_ += 1; + + args.GetReturnValue().Set(Number::New(isolate, obj->value_)); +} + +void InitAll(Local exports) { + MyObject::Init(exports); +} + +NODE_MODULE(NODE_GYP_MODULE_NAME, InitAll) diff --git a/test/addons/worker-addon-exit/binding.gyp b/test/addons/worker-addon-exit/binding.gyp new file mode 100644 index 00000000000000..55fbe7050f18e4 --- /dev/null +++ b/test/addons/worker-addon-exit/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'sources': [ 'binding.cc' ], + 'includes': ['../common.gypi'], + } + ] +} diff --git a/test/addons/worker-addon-exit/test.js b/test/addons/worker-addon-exit/test.js new file mode 100644 index 00000000000000..6c1acfb84ced89 --- /dev/null +++ b/test/addons/worker-addon-exit/test.js @@ -0,0 +1,19 @@ +'use strict'; +const common = require('../../common'); +const { isMainThread, Worker } = require('worker_threads'); + +if (!isMainThread) { + const addon = require(`./build/${common.buildType}/binding`); + + // Create some garbage + const arr = []; + for (let i = 0; i < 1e5; i++) arr.push(`${i}`.repeat(100)); + + new addon.MyObject(10); + + // Should not segfault + throw new Error('exit'); +} else { + const worker = new Worker(__filename); + worker.on('error', () => {}); +} From 53750f57bc8bdd6badfb63aebe9f37db902f9d92 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 29 May 2026 19:50:28 +0200 Subject: [PATCH 3/6] fixup! src: add cleanup hooks to `node::ObjectWrap` --- src/node_object_wrap.h | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/node_object_wrap.h b/src/node_object_wrap.h index c8ce37fcbd8891..03c986689fd294 100644 --- a/src/node_object_wrap.h +++ b/src/node_object_wrap.h @@ -22,9 +22,8 @@ #ifndef SRC_NODE_OBJECT_WRAP_H_ #define SRC_NODE_OBJECT_WRAP_H_ -#include "node.h" #include - +#include "node.h" namespace node { @@ -135,9 +134,7 @@ class ObjectWrap { RemoveEnvironmentCleanupHook(v8::Isolate::GetCurrent(), CleanupHook, this); } - static void CleanupHook(void* arg) { - delete static_cast(arg); - } + static void CleanupHook(void* arg) { delete static_cast(arg); } // NOLINTNEXTLINE(runtime/v8_persistent) v8::Persistent handle_; From fcb0a4dc17fa6b91cccf2f5ea4b3f8c0081532d5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 29 May 2026 20:38:18 +0200 Subject: [PATCH 4/6] fixup! test: add regression test for using `ObjectWrap` in worker --- test/addons/worker-addon-exit/test.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/addons/worker-addon-exit/test.js b/test/addons/worker-addon-exit/test.js index 6c1acfb84ced89..f563f06e3b4ff2 100644 --- a/test/addons/worker-addon-exit/test.js +++ b/test/addons/worker-addon-exit/test.js @@ -5,10 +5,6 @@ const { isMainThread, Worker } = require('worker_threads'); if (!isMainThread) { const addon = require(`./build/${common.buildType}/binding`); - // Create some garbage - const arr = []; - for (let i = 0; i < 1e5; i++) arr.push(`${i}`.repeat(100)); - new addon.MyObject(10); // Should not segfault From 0cede8652c187d98c04a4300f0dd91ecd11b0435 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 30 May 2026 12:36:34 +0200 Subject: [PATCH 5/6] fixup! test: add regression test for using `ObjectWrap` in worker --- .../test-with-triggered-gc.js | 20 +++++++++++++++++++ test/addons/worker-addon-exit/test.js | 2 +- 2 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 test/addons/worker-addon-exit/test-with-triggered-gc.js diff --git a/test/addons/worker-addon-exit/test-with-triggered-gc.js b/test/addons/worker-addon-exit/test-with-triggered-gc.js new file mode 100644 index 00000000000000..839ad57f3bfb4d --- /dev/null +++ b/test/addons/worker-addon-exit/test-with-triggered-gc.js @@ -0,0 +1,20 @@ +'use strict'; +const common = require('../../common'); +const { isMainThread, Worker } = require('worker_threads'); + +if (!isMainThread) { + const addon = require(`./build/${common.buildType}/binding`); + + + // Create some garbage + const arr = []; + for (let i = 0; i < 1e5; i++) arr.push(`${i}`.repeat(100)); + + new addon.MyObject(10); + + // Should not segfault + throw new Error('exit'); +} else { + const worker = new Worker(__filename); + worker.on('error', common.mustCall(() => {})); +} diff --git a/test/addons/worker-addon-exit/test.js b/test/addons/worker-addon-exit/test.js index f563f06e3b4ff2..851eca0e980084 100644 --- a/test/addons/worker-addon-exit/test.js +++ b/test/addons/worker-addon-exit/test.js @@ -11,5 +11,5 @@ if (!isMainThread) { throw new Error('exit'); } else { const worker = new Worker(__filename); - worker.on('error', () => {}); + worker.on('error', common.mustCall(() => {})); } From fa40d0405844dc4c8a0aa6376a9bb2fda62187c1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 30 May 2026 12:41:52 +0200 Subject: [PATCH 6/6] fixup! fixup! test: add regression test for using `ObjectWrap` in worker --- test/addons/worker-addon-exit/test-with-triggered-gc.js | 2 +- test/addons/worker-addon-exit/test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/addons/worker-addon-exit/test-with-triggered-gc.js b/test/addons/worker-addon-exit/test-with-triggered-gc.js index 839ad57f3bfb4d..6639038afe04da 100644 --- a/test/addons/worker-addon-exit/test-with-triggered-gc.js +++ b/test/addons/worker-addon-exit/test-with-triggered-gc.js @@ -16,5 +16,5 @@ if (!isMainThread) { throw new Error('exit'); } else { const worker = new Worker(__filename); - worker.on('error', common.mustCall(() => {})); + worker.on('error', common.mustCall()); } diff --git a/test/addons/worker-addon-exit/test.js b/test/addons/worker-addon-exit/test.js index 851eca0e980084..18c37afb364b69 100644 --- a/test/addons/worker-addon-exit/test.js +++ b/test/addons/worker-addon-exit/test.js @@ -11,5 +11,5 @@ if (!isMainThread) { throw new Error('exit'); } else { const worker = new Worker(__filename); - worker.on('error', common.mustCall(() => {})); + worker.on('error', common.mustCall()); }