GeezerVison is showing github.com — n-api: handle reference delete before finalize · nodejs/node@fe0e119
File treeExpand file treeCollapse file tree Open diff view settings test/addons-napi/test_reference
Expand file treeCollapse file tree Open diff view settings Collapse file Copy file name to clipboardExpand all lines: src/node_api.cc +29-5Lines changed: 29 additions & 5 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -369,6 +369,7 @@ class Finalizer { | 369 | 369 | napi_finalize _finalize_callback; | 370 | 370 | void* _finalize_data; | 371 | 371 | void* _finalize_hint; | | 372 | + bool _finalize_ran = false; | 372 | 373 | }; | 373 | 374 |
| 374 | 375 | // Wrapper around node::Persistent that implements reference counting. | @@ -412,8 +413,29 @@ class Reference : private Finalizer { | 412 | 413 | finalize_hint); | 413 | 414 | } | 414 | 415 |
| | 416 | + // Delete is called in 2 ways. Either from the finalizer or | | 417 | + // from one of Unwrap or napi_delete_reference. | | 418 | + // | | 419 | + // When it is called from Unwrap or napi_delete_reference we only | | 420 | + // want to do the delete if the finalizer has already run, | | 421 | + // otherwise we may crash when the finalizer does run. | | 422 | + // If the finalizer has not already run delay the delete until | | 423 | + // the finalizer runs by not doing the delete | | 424 | + // and setting _delete_self to true so that the finalizer will | | 425 | + // delete it when it runs. | | 426 | + // | | 427 | + // The second way this is called is from | | 428 | + // the finalizer and _delete_self is set. In this case we | | 429 | + // know we need to do the deletion so just do it. | 415 | 430 | static void Delete(Reference* reference) { | 416 | | - delete reference; | | 431 | + if ((reference->_delete_self) || (reference->_finalize_ran)) { | | 432 | + delete reference; | | 433 | + } else { | | 434 | + // reduce the reference count to 0 and defer until | | 435 | + // finalizer runs | | 436 | + reference->_delete_self = true; | | 437 | + while (reference->Unref() != 0) {} | | 438 | + } | 417 | 439 | } | 418 | 440 |
| 419 | 441 | uint32_t Ref() { | @@ -453,9 +475,6 @@ class Reference : private Finalizer { | 453 | 475 | Reference* reference = data.GetParameter(); | 454 | 476 | reference->_persistent.Reset(); | 455 | 477 |
| 456 | | - // Check before calling the finalize callback, because the callback might | 457 | | - // delete it. | 458 | | - bool delete_self = reference->_delete_self; | 459 | 478 | napi_env env = reference->_env; | 460 | 479 |
| 461 | 480 | if (reference->_finalize_callback != nullptr) { | @@ -466,8 +485,13 @@ class Reference : private Finalizer { | 466 | 485 | reference->_finalize_hint)); | 467 | 486 | } | 468 | 487 |
| 469 | | - if (delete_self) { | | 488 | + // this is safe because if a request to delete the reference | | 489 | + // is made in the finalize_callback it will defer deletion | | 490 | + // to this block and set _delete_self to true | | 491 | + if (reference->_delete_self) { | 470 | 492 | Delete(reference); | | 493 | + } else { | | 494 | + reference->_finalize_ran = true; | 471 | 495 | } | 472 | 496 | } | 473 | 497 |
| |
Collapse file Copy file name to clipboardExpand all lines: test/addons-napi/test_reference/test.js +26Lines changed: 26 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -118,3 +118,29 @@ runTests(0, undefined, [ | 118 | 118 | assert.strictEqual(test_reference.finalizeCount, 1); | 119 | 119 | }, | 120 | 120 | ]); | | 121 | + | | 122 | +// This test creates a napi_ref on an object that has | | 123 | +// been wrapped by napi_wrap and for which the finalizer | | 124 | +// for the wrap calls napi_delete_ref on that napi_ref. | | 125 | +// | | 126 | +// Since both the wrap and the reference use the same | | 127 | +// object the finalizer for the wrap and reference | | 128 | +// may run in the same gc and in any order. | | 129 | +// | | 130 | +// It does that to validate that napi_delete_ref can be | | 131 | +// called before the finalizer has been run for the | | 132 | +// reference (there is a finalizer behind the scenes even | | 133 | +// though it cannot be passed to napi_create_reference). | | 134 | +// | | 135 | +// Since the order is not guarranteed, run the | | 136 | +// test a number of times maximize the chance that we | | 137 | +// get a run with the desired order for the test. | | 138 | +// | | 139 | +// 1000 reliably recreated the problem without the fix | | 140 | +// required to ensure delete could be called before | | 141 | +// the finalizer in manual testing. | | 142 | +for (let i = 0; i < 1000; i++) { | | 143 | + const wrapObject = new Object(); | | 144 | + test_reference.validateDeleteBeforeFinalize(wrapObject); | | 145 | + global.gc(); | | 146 | +} |
Collapse file Copy file name to clipboardExpand all lines: test/addons-napi/test_reference/test_reference.c +36Lines changed: 36 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
| | 1 | +#include <stdlib.h> | 1 | 2 | #include <node_api.h> | 2 | 3 | #include "../common.h" | 3 | 4 |
| @@ -131,6 +132,39 @@ static napi_value GetReferenceValue(napi_env env, napi_callback_info info) { | 131 | 132 | return result; | 132 | 133 | } | 133 | 134 |
| | 135 | +static void DeleteBeforeFinalizeFinalizer( | | 136 | + napi_env env, void* finalize_data, void* finalize_hint) { | | 137 | + napi_ref* ref = (napi_ref*)finalize_data; | | 138 | + napi_delete_reference(env, *ref); | | 139 | + free(ref); | | 140 | +} | | 141 | + | | 142 | +static napi_value ValidateDeleteBeforeFinalize(napi_env env, napi_callback_info info) { | | 143 | + napi_value wrapObject; | | 144 | + size_t argc = 1; | | 145 | + NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &wrapObject, NULL, NULL)); | | 146 | + | | 147 | + napi_ref* ref_t = malloc(sizeof(napi_ref)); | | 148 | + NAPI_CALL(env, napi_wrap(env, | | 149 | + wrapObject, | | 150 | + ref_t, | | 151 | + DeleteBeforeFinalizeFinalizer, | | 152 | + NULL, | | 153 | + NULL)); | | 154 | + | | 155 | + // Create a reference that will be eligible for collection at the same | | 156 | + // time as the wrapped object by passing in the same wrapObject. | | 157 | + // This means that the FinalizeOrderValidation callback may be run | | 158 | + // before the finalizer for the newly created reference (there is a finalizer | | 159 | + // behind the scenes even though it cannot be passed to napi_create_reference) | | 160 | + // The Finalizer for the wrap (which is different than the finalizer | | 161 | + // for the reference) calls napi_delete_reference validating that | | 162 | + // napi_delete_reference can be called before the finalizer for the | | 163 | + // reference runs. | | 164 | + NAPI_CALL(env, napi_create_reference(env, wrapObject, 0, ref_t)); | | 165 | + return wrapObject; | | 166 | +} | | 167 | + | 134 | 168 | static napi_value Init(napi_env env, napi_value exports) { | 135 | 169 | napi_property_descriptor descriptors[] = { | 136 | 170 | DECLARE_NAPI_GETTER("finalizeCount", GetFinalizeCount), | @@ -143,6 +177,8 @@ static napi_value Init(napi_env env, napi_value exports) { | 143 | 177 | DECLARE_NAPI_PROPERTY("incrementRefcount", IncrementRefcount), | 144 | 178 | DECLARE_NAPI_PROPERTY("decrementRefcount", DecrementRefcount), | 145 | 179 | DECLARE_NAPI_GETTER("referenceValue", GetReferenceValue), | | 180 | + DECLARE_NAPI_PROPERTY("validateDeleteBeforeFinalize", | | 181 | + ValidateDeleteBeforeFinalize), | 146 | 182 | }; | 147 | 183 |
| 148 | 184 | NAPI_CALL(env, napi_define_properties( | |
|