From 2a25930d629fc8d47909c6628bc172c3916fe99c Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Tue, 18 Aug 2020 23:00:37 -0700 Subject: [PATCH] n-api: handle weak no-finalizer refs correctly When deleting a weak reference that has no finalizer we must not defer deletion until the non-existent finalizer gets called. Fixes: https://github.com/nodejs/node/issues/34731 Signed-off-by: Gabriel Schulhof --- src/js_native_api_v8.cc | 6 ++++-- test/node-api/test_worker_terminate_finalization/test.js | 4 ---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index b8455eb3a69b3e..0a069b3ae45b97 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -228,9 +228,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 @@ -242,6 +243,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 index d58324d5e5f696..7240520080e66c 100644 --- 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) {