From af87a67e8aeb0cd57f91403c379c673d52f569b5 Mon Sep 17 00:00:00 2001
From: Michael Dawson <mdawson@devrus.com>
Date: Fri, 16 Nov 2018 21:42:31 +0000
Subject: [PATCH] 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
---
 src/js_native_api_v8.cc | 33 ++++++++++++++++++++++++++++-----
 src/js_native_api_v8.h  |  1 +
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
index b28376afb7cedd..71f0d7ba8f50a3 100644
--- a/src/js_native_api_v8.cc
+++ b/src/js_native_api_v8.cc
@@ -226,8 +226,29 @@ class Reference : private Finalizer {
       finalize_hint);
   }
 
+  // Delete is called in 2 ways. Either from the finalizer or
+  // 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,
+  // otherwise we may crash when the finalizer does run.
+  // If the finalizer 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
+  // delete it when it runs.
+  //
+  // The second way this is called is from
+  // the finalizer and _delete_self is set. In this case we
+  // know we need to do the deletion so just do it.
   static void Delete(Reference* reference) {
-    delete reference;
+    if ((reference->_delete_self) || (reference->_finalize_ran)) {
+      delete reference;
+    } else {
+      // reduce the reference count to 0 and defer until
+      // finalizer runs
+      reference->_delete_self = true;
+      while (reference->Unref() != 0) {}
+    }
   }
 
   uint32_t Ref() {
@@ -267,9 +288,6 @@ class Reference : private Finalizer {
     Reference* reference = data.GetParameter();
     reference->_persistent.Reset();
 
-    // Check before calling the finalize callback, because the callback might
-    // delete it.
-    bool delete_self = reference->_delete_self;
     napi_env env = reference->_env;
 
     if (reference->_finalize_callback != nullptr) {
@@ -280,8 +298,13 @@ class Reference : private Finalizer {
             reference->_finalize_hint));
     }
 
-    if (delete_self) {
+    // this is safe because if a request to delete the reference
+    // is made in the finalize_callback it will defer deletion
+    // to this block and set _delete_self to true
+    if (reference->_delete_self) {
       Delete(reference);
+    } else {
+      reference->_finalize_ran = true;
     }
   }
 
diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h
index d5402845dc6af6..81b00f2aa59b8f 100644
--- a/src/js_native_api_v8.h
+++ b/src/js_native_api_v8.h
@@ -180,6 +180,7 @@ class Finalizer {
   napi_finalize _finalize_callback;
   void* _finalize_data;
   void* _finalize_hint;
+  bool _finalize_ran = false;
 };
 
 class TryCatch : public v8::TryCatch {