worker: fix premature addon unload

[mohd-akram]

Weak callbacks could run after addons were unloaded, leading to a crash.

Fixes #63540

[nodejs-github-bot]

Review requested:

• @nodejs/gyp

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.33%. Comparing base (8c495c8) to head (50023d9).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #63575    +/-   ##
========================================
  Coverage   90.33%   90.33%            
========================================
  Files         730      730            
  Lines      234362   234464   +102     
  Branches    43908    43909     +1     
========================================
+ Hits       211708   211810   +102     
- Misses      14376    14378     +2     
+ Partials     8278     8276     -2     

Files with missing lines	Coverage Δ
src/env-inl.h	95.01% <100.00%> (+0.02%)	⬆️
src/env.cc	85.35% <ø> (+0.18%)	⬆️
src/env.h	98.21% <ø> (ø)
src/node_worker.cc	82.17% <100.00%> (+0.05%)	⬆️

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[addaleax]

This seems like a valid bug, but it doesn't seem like an issue in the workers implementation, and rather one in node::ObjectWrap; the class should add an explicit cleanup hook via AddEnvironmentCleanupHook() along its weak callback.

node::ObjectWrap in general has been unmaintained and really should have been deprecated a long time ago, but yeah, that's the place to fix this.

[mohd-akram]

@addaleax But this problem doesn't happen in the main thread. I had first thought about solving it in ObjectWrap as well, but that will not fix the case where addons use WeakCallback directly, and potentially other situations where a callback lingers. Also, as you mentioned, the ObjectWrap code has not been modified for many years and I would be hesitant to make any change there lest it break some other thing. It is exceptionally difficult to debug this type of bug because the code is gone so it doesn't appear in the stack trace. Per the comment that's there in the code, this was disabled for the main thread presumably because of certain issues, and the same ones will ostensibly happen if an add-on is only loaded in the worker thread, so IMO it would be good to align the main and worker thread behaviors. It also makes sense from a pure conceptual perspective that unloading should happen after all code has stopped running, and I'm not sure if there's some benefit for doing it earlier.

[addaleax]

and potentially other situations where a callback lingers

That's part of the problem – this isn't specific to WeakCallbacks per se, and you can run into it with any addon that manually schedules callbacks.

Per the comment that's there in the code, this was disabled for the main thread presumably because of certain issues, and the same ones will ostensibly happen if an add-on is only loaded in the worker thread, so IMO it would be good to align the main and worker thread behaviors.

It's disabled in the main thread because legacy addons that are not aware of Environment cleanup hooks are expected to keep working as they are, that's all. This is also why we call out in our docs that addons need to explicitly opt into supporting worker threads: https://nodejs.org/api/addons.html#worker-support

Also, as you mentioned, the ObjectWrap code has not been modified for many years and I would be hesitant to make any change there lest it break some other thing.

That's not a good reason to keep it buggy tbh.

It also makes sense from a pure conceptual perspective that unloading should happen after all code has stopped running, and I'm not sure if there's some benefit for doing it earlier.

At the very least, there needs to be something that ensures that embedder-created Environment* instances still have their addons closed, which this PR currently breaks. But really, the Environment instance is conceptually what owns the addons, and if one has objects or callbacks that outlive the Enviroment, that's a bug (again, AddEnvironmentCleanupHook() is there for exactly this).