From 50023d93b701a7a37f43e9ab53011a27ce28c20b Mon Sep 17 00:00:00 2001 From: Mohamed Akram Date: Tue, 26 May 2026 04:18:14 +0400 Subject: [PATCH] worker: fix premature addon unload Weak callbacks could run after addons were unloaded, leading to a crash. Signed-off-by: Mohamed Akram --- src/env-inl.h | 4 + src/env.cc | 12 --- src/env.h | 1 + src/node_worker.cc | 8 ++ test/addons/worker-addon-exit/binding.cc | 99 +++++++++++++++++++++++ test/addons/worker-addon-exit/binding.gyp | 9 +++ test/addons/worker-addon-exit/test.js | 19 +++++ 7 files changed, 140 insertions(+), 12 deletions(-) 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/src/env-inl.h b/src/env-inl.h index 761a7bfc995528..bbcff43b8c899a 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -280,6 +280,10 @@ inline uv_loop_t* Environment::event_loop() const { return isolate_data()->event_loop(); } +inline std::list& Environment::loaded_addons() { + return loaded_addons_; +} + #if HAVE_INSPECTOR inline bool Environment::is_in_inspector_console_call() const { return is_in_inspector_console_call_; diff --git a/src/env.cc b/src/env.cc index df092506ab3e21..05a8191a29a5ad 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1073,18 +1073,6 @@ Environment::~Environment() { TRACE_EVENT_NESTABLE_ASYNC_END0( TRACING_CATEGORY_NODE1(environment), "Environment", this); - // Do not unload addons on the main thread. Some addons need to retain memory - // beyond the Environment's lifetime, and unloading them early would break - // them; with Worker threads, we have the opportunity to be stricter. - // Also, since the main thread usually stops just before the process exits, - // this is far less relevant here. - if (!is_main_thread()) { - // Dereference all addons that were loaded into this environment. - for (binding::DLib& addon : loaded_addons_) { - addon.Close(); - } - } - delete external_memory_accounter_; if (cpu_profiler_) { for (auto& it : pending_profiles_) { diff --git a/src/env.h b/src/env.h index 61512d9494ff61..92815fab10a31a 100644 --- a/src/env.h +++ b/src/env.h @@ -703,6 +703,7 @@ class Environment final : public MemoryRetainer { inline cppgc::AllocationHandle& cppgc_allocation_handle() const; inline v8::ExternalMemoryAccounter* external_memory_accounter() const; inline uv_loop_t* event_loop() const; + inline std::list& loaded_addons(); void TryLoadAddon(const char* filename, int flags, const std::function& was_loaded); diff --git a/src/node_worker.cc b/src/node_worker.cc index edc21e7e556157..caa905304e3347 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -258,6 +258,11 @@ class WorkerThreadData { if (!loop_init_failed_) { CheckedUvLoopClose(&loop_); } + + // Dereference all addons that were loaded into this environment. + for (binding::DLib& addon : loaded_addons_) { + addon.Close(); + } } bool loop_is_usable() const { return !loop_init_failed_; } @@ -266,6 +271,7 @@ class WorkerThreadData { Worker* const w_; uv_loop_t loop_; bool loop_init_failed_ = true; + std::list loaded_addons_; DeleteFnPtr isolate_data_; friend class Worker; }; @@ -322,6 +328,8 @@ void Worker::Run() { if (!env_) return; env_->set_can_call_into_js(false); + std::swap(data.loaded_addons_, env_->loaded_addons()); + { Mutex::ScopedLock lock(mutex_); stopped_ = true; 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', () => {}); +}