Skip to content

Commit

Permalink
n-api: handle weak no-finalizer refs correctly
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
Gabriel Schulhof authored and Trott committed Aug 21, 2020
1 parent f862bcb commit acd423b
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 6 deletions.
6 changes: 4 additions & 2 deletions src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down
4 changes: 0 additions & 4 deletions test/node-api/test_worker_terminate_finalization/test.js
Original file line number Diff line number Diff line change
@@ -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) {
Expand Down

0 comments on commit acd423b

Please sign in to comment.