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.