n-api: handle weak no-finalizer refs correctly

Gabriel Schulhof · addaleax · commit a6b655614f03 · 2020-09-22T19:55:59.000+02:00
When deleting a weak reference that has no finalizer we must not defer deletion until the non-existent finalizer gets called. Fixes: #34731 Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> PR-URL: #34839 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
@@ -225,9 +225,10 @@ class RefBase : protected Finalizer, RefTracker {
   // from one of Unwrap or napi_delete_reference.
   //
   // When it is called from Unwrap or napi_delete_reference we only
-  // want to do the delete if the finalizer has already run or
-  // cannot have been queued to run (ie the reference count is > 0),
+  // want to do the delete if there is no finalizer or the finalizer has already
+  // run or cannot have been queued to run (i.e. the reference count is > 0),
   // otherwise we may crash when the finalizer does run.
+  //
   // If the finalizer may have been queued and has not already run
   // delay the delete until the finalizer runs by not doing the delete
   // and setting _delete_self to true so that the finalizer will
@@ -239,6 +240,7 @@ class RefBase : protected Finalizer, RefTracker {
   static inline void Delete(RefBase* reference) {
     reference->Unlink();
     if ((reference->RefCount() != 0) ||
+        (reference->_finalize_callback == nullptr) ||
         (reference->_delete_self) ||
         (reference->_finalize_ran)) {
       delete reference;
diff --git a/test/node-api/test_worker_terminate_finalization/test.js b/test/node-api/test_worker_terminate_finalization/test.js
@@ -1,10 +1,6 @@
 'use strict';
 const common = require('../../common');
 
-// TODO(addaleax): Run this test once it stops failing under ASAN/valgrind.
-// Refs: https://github.com/nodejs/node/issues/34731
-common.skip('Reference management in N-API leaks memory');
-
 const { Worker, isMainThread } = require('worker_threads');
 
 if (isMainThread) {
