From 0f46eaa0a7a4537d6ff5eede0b93ccac1e5daa32 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Fri, 23 Oct 2020 13:36:54 -0400 Subject: [PATCH 1/2] n-api: revert change to finalization Fixes: https://github.com/nodejs/node/issues/35620 This reverts commit a6b655614f03e073b9c60f3d71ed884c5af32ffc which changed finalization behavior related to N-API. We will investigate the original issue with the test separately. --- src/js_native_api_v8.cc | 6 ++---- test/node-api/test_worker_terminate_finalization/test.js | 4 ++++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 1feabfd879476c..3b8a5ff51b54ab 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -228,10 +228,9 @@ 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 there is no finalizer or the finalizer has already - // run or cannot have been queued to run (i.e. the reference count is > 0), + // want to do the delete if the finalizer has already run or + // cannot have been queued to run (ie 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 @@ -243,7 +242,6 @@ 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 7240520080e66c..d58324d5e5f696 100644 --- a/test/node-api/test_worker_terminate_finalization/test.js +++ b/test/node-api/test_worker_terminate_finalization/test.js @@ -1,6 +1,10 @@ '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) { From 3769505233a578e0429696314a6050d87b829b29 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Mon, 26 Oct 2020 15:04:32 -0400 Subject: [PATCH 2/2] Update test/node-api/test_worker_terminate_finalization/test.js --- test/node-api/test_worker_terminate_finalization/test.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/node-api/test_worker_terminate_finalization/test.js b/test/node-api/test_worker_terminate_finalization/test.js index d58324d5e5f696..171a32b812334f 100644 --- a/test/node-api/test_worker_terminate_finalization/test.js +++ b/test/node-api/test_worker_terminate_finalization/test.js @@ -3,6 +3,8 @@ const common = require('../../common'); // TODO(addaleax): Run this test once it stops failing under ASAN/valgrind. // Refs: https://github.com/nodejs/node/issues/34731 +// Refs: https://github.com/nodejs/node/pull/35777 +// Refs: https://github.com/nodejs/node/issues/35778 common.skip('Reference management in N-API leaks memory'); const { Worker, isMainThread } = require('worker_threads');