n-api: handle reference delete before finalize

Crashes were reported during finalization due to
the memory for a reference being deleted and the
finalizer running after the deletion.

This change ensures the deletion of the memory for
the reference only occurs after the finalizer has run.

Fixes: https://github.com/nodejs/node-addon-api/issues/393

Backport-PR-URL: https://github.com/nodejs/node/pull/25574
PR-URL: https://github.com/nodejs/node/pull/24494
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

2 files changed, 62 insertions, 0 deletions

diff --git a/test/addons-napi/test_reference/test.js b/test/addons-napi/test_reference/test.js
index 14932a74ca7..389ee11d7e5 100644
--- a/test/addons-napi/test_reference/test.js
+++ b/test/addons-napi/test_reference/test.js
@@ -118,3 +118,29 @@ runTests(0, undefined, [
     assert.strictEqual(test_reference.finalizeCount, 1);
   },
 ]);
+
+// This test creates a napi_ref on an object that has
+// been wrapped by napi_wrap and for which the finalizer
+// for the wrap calls napi_delete_ref on that napi_ref.
+//
+// Since both the wrap and the reference use the same
+// object the finalizer for the wrap and reference
+// may run in the same gc and in any order.
+//
+// It does that to validate that napi_delete_ref can be
+// called before the finalizer has been run for the
+// reference (there is a finalizer behind the scenes even
+// though it cannot be passed to napi_create_reference).
+//
+// Since the order is not guarranteed, run the
+// test a number of times maximize the chance that we
+// get a run with the desired order for the test.
+//
+// 1000 reliably recreated the problem without the fix
+// required to ensure delete could be called before
+// the finalizer in manual testing.
+for (let i = 0; i < 1000; i++) {
+  const wrapObject = new Object();
+  test_reference.validateDeleteBeforeFinalize(wrapObject);
+  global.gc();
+}
diff --git a/test/addons-napi/test_reference/test_reference.c b/test/addons-napi/test_reference/test_reference.c
index 75abc49ad32..f3dc3644770 100644
--- a/test/addons-napi/test_reference/test_reference.c
+++ b/test/addons-napi/test_reference/test_reference.c
@@ -1,3 +1,4 @@
+#include <stdlib.h>
 #include <node_api.h>
 #include "../common.h"
 
@@ -131,6 +132,39 @@ static napi_value GetReferenceValue(napi_env env, napi_callback_info info) {
   return result;
 }
 
+static void DeleteBeforeFinalizeFinalizer(
+    napi_env env, void* finalize_data, void* finalize_hint) {
+  napi_ref* ref = (napi_ref*)finalize_data;
+  napi_delete_reference(env, *ref);
+  free(ref);
+}
+
+static napi_value ValidateDeleteBeforeFinalize(napi_env env, napi_callback_info info) {
+  napi_value wrapObject;
+  size_t argc = 1;
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &wrapObject, NULL, NULL));
+
+  napi_ref* ref_t = malloc(sizeof(napi_ref));
+  NAPI_CALL(env, napi_wrap(env,
+			   wrapObject,
+			   ref_t,
+			   DeleteBeforeFinalizeFinalizer,
+			   NULL,
+			   NULL));
+
+  // Create a reference that will be eligible for collection at the same
+  // time as the wrapped object by passing in the same wrapObject.
+  // This means that the FinalizeOrderValidation callback may be run
+  // before the finalizer for the newly created reference (there is a finalizer
+  // behind the scenes even though it cannot be passed to napi_create_reference)
+  // The Finalizer for the wrap (which is different than the finalizer
+  // for the reference) calls napi_delete_reference validating that
+  // napi_delete_reference can be called before the finalizer for the
+  // reference runs.
+  NAPI_CALL(env, napi_create_reference(env, wrapObject, 0, ref_t));
+  return wrapObject;
+}
+
 static napi_value Init(napi_env env, napi_value exports) {
   napi_property_descriptor descriptors[] = {
     DECLARE_NAPI_GETTER("finalizeCount", GetFinalizeCount),
@@ -143,6 +177,8 @@ static napi_value Init(napi_env env, napi_value exports) {
     DECLARE_NAPI_PROPERTY("incrementRefcount", IncrementRefcount),
     DECLARE_NAPI_PROPERTY("decrementRefcount", DecrementRefcount),
     DECLARE_NAPI_GETTER("referenceValue", GetReferenceValue),
+    DECLARE_NAPI_PROPERTY("validateDeleteBeforeFinalize",
+		          ValidateDeleteBeforeFinalize),
   };
 
   NAPI_CALL(env, napi_define_properties(