From af5c489f3920842fef703897559b93aee34c799b Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Mon, 30 Dec 2019 21:26:35 +0100
Subject: [PATCH] n-api: keep napi_env alive while it has finalizers

Manage the napi_env refcount from Finalizer instances, as the
finalizer may refer to the napi_env until it is deleted.

Fixes: https://github.com/nodejs/node/issues/31134
Fixes: https://github.com/node-ffi-napi/node-ffi-napi/issues/48
PR-URL: https://github.com/nodejs/node/pull/31140
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
---
 src/js_native_api_v8.h                        | 28 +++++++++++++++----
 src/node_api.cc                               |  3 +-
 .../test_buffer/test-external-buffer.js       | 14 ++++++++++
 3 files changed, 39 insertions(+), 6 deletions(-)
 create mode 100644 test/node-api/test_buffer/test-external-buffer.js

diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h
index 2e0a7a1d6add20..8eed25bc9e6f77 100644
--- a/src/js_native_api_v8.h
+++ b/src/js_native_api_v8.h
@@ -182,26 +182,43 @@ inline v8::Local<v8::Value> V8LocalValueFromJsValue(napi_value v) {
 
 // Adapter for napi_finalize callbacks.
 class Finalizer {
+ public:
+  // Some Finalizers are run during shutdown when the napi_env is destroyed,
+  // and some need to keep an explicit reference to the napi_env because they
+  // are run independently.
+  enum EnvReferenceMode {
+    kNoEnvReference,
+    kKeepEnvReference
+  };
+
  protected:
   Finalizer(napi_env env,
             napi_finalize finalize_callback,
             void* finalize_data,
-            void* finalize_hint)
+            void* finalize_hint,
+            EnvReferenceMode refmode = kNoEnvReference)
     : _env(env),
       _finalize_callback(finalize_callback),
       _finalize_data(finalize_data),
-      _finalize_hint(finalize_hint) {
+      _finalize_hint(finalize_hint),
+      _has_env_reference(refmode == kKeepEnvReference) {
+    if (_has_env_reference)
+      _env->Ref();
   }
 
-  ~Finalizer() = default;
+  ~Finalizer() {
+    if (_has_env_reference)
+      _env->Unref();
+  }
 
  public:
   static Finalizer* New(napi_env env,
                         napi_finalize finalize_callback = nullptr,
                         void* finalize_data = nullptr,
-                        void* finalize_hint = nullptr) {
+                        void* finalize_hint = nullptr,
+                        EnvReferenceMode refmode = kNoEnvReference) {
     return new Finalizer(
-      env, finalize_callback, finalize_data, finalize_hint);
+        env, finalize_callback, finalize_data, finalize_hint, refmode);
   }
 
   static void Delete(Finalizer* finalizer) {
@@ -214,6 +231,7 @@ class Finalizer {
   void* _finalize_data;
   void* _finalize_hint;
   bool _finalize_ran = false;
+  bool _has_env_reference = false;
 };
 
 class TryCatch : public v8::TryCatch {
diff --git a/src/node_api.cc b/src/node_api.cc
index 95664e9c7ace27..20b591747de2d0 100644
--- a/src/node_api.cc
+++ b/src/node_api.cc
@@ -724,7 +724,8 @@ napi_status napi_create_external_buffer(napi_env env,
 
   // The finalizer object will delete itself after invoking the callback.
   v8impl::Finalizer* finalizer = v8impl::Finalizer::New(
-    env, finalize_cb, nullptr, finalize_hint);
+      env, finalize_cb, nullptr, finalize_hint,
+      v8impl::Finalizer::kKeepEnvReference);
 
   auto maybe = node::Buffer::New(isolate,
                                 static_cast<char*>(data),
diff --git a/test/node-api/test_buffer/test-external-buffer.js b/test/node-api/test_buffer/test-external-buffer.js
new file mode 100644
index 00000000000000..4f220f1d21e9ce
--- /dev/null
+++ b/test/node-api/test_buffer/test-external-buffer.js
@@ -0,0 +1,14 @@
+'use strict';
+
+const common = require('../../common');
+const binding = require(`./build/${common.buildType}/test_buffer`);
+const assert = require('assert');
+
+// Regression test for https://github.com/nodejs/node/issues/31134
+// Buffers with finalizers are allowed to remain alive until
+// Environment cleanup without crashing the process.
+// The test stores the buffer on `process` to make sure it lives as long as
+// the Context does.
+
+process.externalBuffer = binding.newExternalBuffer();
+assert.strictEqual(process.externalBuffer.toString(), binding.theText);