From dfefe12551bcba5e1423d71fa31277f25ff78617 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Tue, 9 Feb 2021 22:52:54 -0800 Subject: [PATCH 1/8] node-api: force env shutdown deferring behavior The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: https://github.com/nodejs/node/issues/37236 Co-authored-by: Chengzhong Wu PR-URL: https://github.com/nodejs/node/pull/37303 Reviewed-By: Chengzhong Wu Reviewed-By: Michael Dawson --- src/js_native_api_v8.cc | 9 +++ .../test_reference_double_free/binding.gyp | 11 ++++ .../test_reference_double_free/test.js | 11 ++++ .../test_reference_double_free.c | 55 +++++++++++++++++++ 4 files changed, 86 insertions(+) create mode 100644 test/js-native-api/test_reference_double_free/binding.gyp create mode 100644 test/js-native-api/test_reference_double_free/test.js create mode 100644 test/js-native-api/test_reference_double_free/test_reference_double_free.c diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 6a205fafcaa95f..5163503beea7c1 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -269,8 +269,17 @@ class RefBase : protected Finalizer, RefTracker { protected: inline void Finalize(bool is_env_teardown = false) override { + // During environment teardown we have to convert a strong reference to + // a weak reference to force the deferring behavior if the user's finalizer + // happens to delete this reference so that the code in this function that + // follows the call to the user's finalizer may safely access variables from + // this instance. + if (is_env_teardown && RefCount() > 0) _refcount = 0; + if (_finalize_callback != nullptr) { _env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint); + // This ensures that we never call the finalizer twice. + _finalize_callback = nullptr; } // this is safe because if a request to delete the reference diff --git a/test/js-native-api/test_reference_double_free/binding.gyp b/test/js-native-api/test_reference_double_free/binding.gyp new file mode 100644 index 00000000000000..864846765d0132 --- /dev/null +++ b/test/js-native-api/test_reference_double_free/binding.gyp @@ -0,0 +1,11 @@ +{ + "targets": [ + { + "target_name": "test_reference_double_free", + "sources": [ + "../entry_point.c", + "test_reference_double_free.c" + ] + } + ] +} diff --git a/test/js-native-api/test_reference_double_free/test.js b/test/js-native-api/test_reference_double_free/test.js new file mode 100644 index 00000000000000..242d850ba9f677 --- /dev/null +++ b/test/js-native-api/test_reference_double_free/test.js @@ -0,0 +1,11 @@ +'use strict'; + +// This test makes no assertions. It tests a fix without which it will crash +// with a double free. + +const { buildType } = require('../../common'); + +const addon = require(`./build/${buildType}/test_reference_double_free`); + +{ new addon.MyObject(true); } +{ new addon.MyObject(false); } diff --git a/test/js-native-api/test_reference_double_free/test_reference_double_free.c b/test/js-native-api/test_reference_double_free/test_reference_double_free.c new file mode 100644 index 00000000000000..45329e075954c9 --- /dev/null +++ b/test/js-native-api/test_reference_double_free/test_reference_double_free.c @@ -0,0 +1,55 @@ +#include +#include +#include "../common.h" + +static size_t g_call_count = 0; + +static void Destructor(napi_env env, void* data, void* nothing) { + napi_ref* ref = data; + NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref)); + free(ref); +} + +static void NoDeleteDestructor(napi_env env, void* data, void* hint) { + napi_ref* ref = data; + size_t* call_count = hint; + + // This destructor must be called exactly once. + if ((*call_count) > 0) abort(); + *call_count = ((*call_count) + 1); + free(ref); +} + +static napi_value New(napi_env env, napi_callback_info info) { + size_t argc = 1; + napi_value js_this, js_delete; + bool delete; + napi_ref* ref = malloc(sizeof(*ref)); + + NAPI_CALL(env, + napi_get_cb_info(env, info, &argc, &js_delete, &js_this, NULL)); + NAPI_CALL(env, napi_get_value_bool(env, js_delete, &delete)); + + if (delete) { + NAPI_CALL(env, + napi_wrap(env, js_this, ref, Destructor, NULL, ref)); + } else { + NAPI_CALL(env, + napi_wrap(env, js_this, ref, NoDeleteDestructor, &g_call_count, ref)); + } + NAPI_CALL(env, napi_reference_ref(env, *ref, NULL)); + + return js_this; +} + +EXTERN_C_START +napi_value Init(napi_env env, napi_value exports) { + napi_value myobj_ctor; + NAPI_CALL(env, + napi_define_class( + env, "MyObject", NAPI_AUTO_LENGTH, New, NULL, 0, NULL, &myobj_ctor)); + NAPI_CALL(env, + napi_set_named_property(env, exports, "MyObject", myobj_ctor)); + return exports; +} +EXTERN_C_END From 7b6b7d343edb7b4d23a75a2e3992f8284914f1dd Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Wed, 3 Mar 2021 20:37:27 -0800 Subject: [PATCH 2/8] node-api: stop ref gc during environment teardown A gc may happen during environment teardown. Thus, during finalization initiated by environment teardown we must remove the V8 finalizer before calling the Node-API finalizer. Fixes: https://github.com/nodejs/node/issues/37236 PR-URL: https://github.com/nodejs/node/pull/37616 Reviewed-By: Chengzhong Wu Reviewed-By: Michael Dawson --- src/js_native_api_v8.cc | 28 +++++++++++++- .../test_reference_double_free.c | 2 +- test/node-api/test_env_teardown_gc/binding.c | 37 +++++++++++++++++++ .../node-api/test_env_teardown_gc/binding.gyp | 8 ++++ test/node-api/test_env_teardown_gc/test.js | 14 +++++++ 5 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 test/node-api/test_env_teardown_gc/binding.c create mode 100644 test/node-api/test_env_teardown_gc/binding.gyp create mode 100644 test/node-api/test_env_teardown_gc/test.js diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 5163503beea7c1..e969adc3918ec4 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -269,6 +269,20 @@ class RefBase : protected Finalizer, RefTracker { protected: inline void Finalize(bool is_env_teardown = false) override { + // In addition to being called during environment teardown, this method is + // also the entry point for the garbage collector. During environment + // teardown we have to remove the garbage collector's reference to this + // method so that, if, as part of the user's callback, JS gets executed, + // resulting in a garbage collection pass, this method is not re-entered as + // part of that pass, because that'll cause a double free (as seen in + // https://github.com/nodejs/node/issues/37236). + // + // Since this class does not have access to the V8 persistent reference, + // this method is overridden in the `Reference` class below. Therein the + // weak callback is removed, ensuring that the garbage collector does not + // re-enter this method, and the method chains up to continue the process of + // environment-teardown-induced finalization. + // During environment teardown we have to convert a strong reference to // a weak reference to force the deferring behavior if the user's finalizer // happens to delete this reference so that the code in this function that @@ -277,9 +291,10 @@ class RefBase : protected Finalizer, RefTracker { if (is_env_teardown && RefCount() > 0) _refcount = 0; if (_finalize_callback != nullptr) { - _env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint); // This ensures that we never call the finalizer twice. + napi_finalize fini = _finalize_callback; _finalize_callback = nullptr; + _env->CallFinalizer(fini, _finalize_data, _finalize_hint); } // this is safe because if a request to delete the reference @@ -354,6 +369,17 @@ class Reference : public RefBase { } } + protected: + inline void Finalize(bool is_env_teardown = false) override { + // During env teardown, `~napi_env()` alone is responsible for finalizing. + // Thus, we don't want any stray gc passes to trigger a second call to + // `Finalize()`, so let's reset the persistent here. + if (is_env_teardown) _persistent.ClearWeak(); + + // Chain up to perform the rest of the finalization. + RefBase::Finalize(is_env_teardown); + } + private: // The N-API finalizer callback may make calls into the engine. V8's heap is // not in a consistent state during the weak callback, and therefore it does diff --git a/test/js-native-api/test_reference_double_free/test_reference_double_free.c b/test/js-native-api/test_reference_double_free/test_reference_double_free.c index 45329e075954c9..f38867d220ae68 100644 --- a/test/js-native-api/test_reference_double_free/test_reference_double_free.c +++ b/test/js-native-api/test_reference_double_free/test_reference_double_free.c @@ -6,7 +6,7 @@ static size_t g_call_count = 0; static void Destructor(napi_env env, void* data, void* nothing) { napi_ref* ref = data; - NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref)); + NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref)); free(ref); } diff --git a/test/node-api/test_env_teardown_gc/binding.c b/test/node-api/test_env_teardown_gc/binding.c new file mode 100644 index 00000000000000..f894690b2470b2 --- /dev/null +++ b/test/node-api/test_env_teardown_gc/binding.c @@ -0,0 +1,37 @@ +#include +#include +#include "../../js-native-api/common.h" + +static void MyObject_fini(napi_env env, void* data, void* hint) { + napi_ref* ref = data; + napi_value global; + napi_value cleanup; + NAPI_CALL_RETURN_VOID(env, napi_get_global(env, &global)); + NAPI_CALL_RETURN_VOID( + env, napi_get_named_property(env, global, "cleanup", &cleanup)); + napi_status status = napi_call_function(env, global, cleanup, 0, NULL, NULL); + // We may not be allowed to call into JS, in which case a pending exception + // will be returned. + NAPI_ASSERT_RETURN_VOID(env, + status == napi_ok || status == napi_pending_exception, + "Unexpected status for napi_call_function"); + NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref)); + free(ref); +} + +static napi_value MyObject(napi_env env, napi_callback_info info) { + napi_value js_this; + napi_ref* ref = malloc(sizeof(*ref)); + NAPI_CALL(env, napi_get_cb_info(env, info, NULL, NULL, &js_this, NULL)); + NAPI_CALL(env, napi_wrap(env, js_this, ref, MyObject_fini, NULL, ref)); + return NULL; +} + +NAPI_MODULE_INIT() { + napi_value ctor; + NAPI_CALL( + env, napi_define_class( + env, "MyObject", NAPI_AUTO_LENGTH, MyObject, NULL, 0, NULL, &ctor)); + NAPI_CALL(env, napi_set_named_property(env, exports, "MyObject", ctor)); + return exports; +} diff --git a/test/node-api/test_env_teardown_gc/binding.gyp b/test/node-api/test_env_teardown_gc/binding.gyp new file mode 100644 index 00000000000000..413621ade335a1 --- /dev/null +++ b/test/node-api/test_env_teardown_gc/binding.gyp @@ -0,0 +1,8 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'sources': [ 'binding.c' ] + } + ] +} diff --git a/test/node-api/test_env_teardown_gc/test.js b/test/node-api/test_env_teardown_gc/test.js new file mode 100644 index 00000000000000..8f1c4f4038fbab --- /dev/null +++ b/test/node-api/test_env_teardown_gc/test.js @@ -0,0 +1,14 @@ +'use strict'; +// Flags: --expose-gc + +process.env.NODE_TEST_KNOWN_GLOBALS = 0; + +const common = require('../../common'); +const binding = require(`./build/${common.buildType}/binding`); + +global.it = new binding.MyObject(); + +global.cleanup = () => { + delete global.it; + global.gc(); +}; From affb8b2515789099e4b510251b500c55b50c8afc Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 23 Mar 2021 11:26:18 -0400 Subject: [PATCH 3/8] node-api: fix crash in finalization Refs: https://github.com/nodejs/node-addon-api/issues/906 Refs: https://github.com/nodejs/node/pull/37616 Fix crash introduced by https://github.com/nodejs/node/pull/37616 Signed-off-by: Michael Dawson PR-URL: https://github.com/nodejs/node/pull/37876 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: Rich Trott Reviewed-By: James M Snell --- src/js_native_api_v8.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index e969adc3918ec4..5106711bd2d1c1 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -373,8 +373,11 @@ class Reference : public RefBase { inline void Finalize(bool is_env_teardown = false) override { // During env teardown, `~napi_env()` alone is responsible for finalizing. // Thus, we don't want any stray gc passes to trigger a second call to - // `Finalize()`, so let's reset the persistent here. - if (is_env_teardown) _persistent.ClearWeak(); + // `Finalize()`, so let's reset the persistent here if nothing is + // keeping it alive. + if (is_env_teardown && _persistent.IsWeak()) { + _persistent.ClearWeak(); + } // Chain up to perform the rest of the finalization. RefBase::Finalize(is_env_teardown); From b4090b97b56a963dd8c2ff529f52dc480b9d64d4 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Wed, 31 Mar 2021 14:42:11 +0800 Subject: [PATCH 4/8] node-api: make reference weak parameter an indirect link to references As the cancellation of second pass callbacks are not reachable from the current v8 API, and the second pass callbacks are scheduled with NodePlatform's task runner, we have to ensure that the weak parameter holds indirect access to the v8impl::Reference object so that the object can be destroyed on addon env teardown before the whole node env is able to shutdown. PR-URL: https://github.com/nodejs/node/pull/38000 Reviewed-By: Michael Dawson Reviewed-By: Gabriel Schulhof --- src/js_native_api_v8.cc | 98 ++++++++++++++++++++++++++++++++++------- 1 file changed, 81 insertions(+), 17 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 5106711bd2d1c1..8254dc7d6c8aa2 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -313,16 +313,16 @@ class RefBase : protected Finalizer, RefTracker { }; class Reference : public RefBase { + using SecondPassCallParameterRef = Reference*; + protected: template - Reference(napi_env env, - v8::Local value, - Args&&... args) + Reference(napi_env env, v8::Local value, Args&&... args) : RefBase(env, std::forward(args)...), - _persistent(env->isolate, value) { + _persistent(env->isolate, value), + _secondPassParameter(new SecondPassCallParameterRef(this)) { if (RefCount() == 0) { - _persistent.SetWeak( - this, FinalizeCallback, v8::WeakCallbackType::kParameter); + SetWeak(); } } @@ -343,10 +343,19 @@ class Reference : public RefBase { finalize_hint); } + virtual ~Reference() { + // If the second pass callback is scheduled, it will delete the + // parameter passed to it, otherwise it will never be scheduled + // and we need to delete it here. + if (_secondPassParameter != nullptr) { + delete _secondPassParameter; + } + } + inline uint32_t Ref() { uint32_t refcount = RefBase::Ref(); if (refcount == 1) { - _persistent.ClearWeak(); + ClearWeak(); } return refcount; } @@ -355,8 +364,7 @@ class Reference : public RefBase { uint32_t old_refcount = RefCount(); uint32_t refcount = RefBase::Unref(); if (old_refcount == 1 && refcount == 0) { - _persistent.SetWeak( - this, FinalizeCallback, v8::WeakCallbackType::kParameter); + SetWeak(); } return refcount; } @@ -373,10 +381,11 @@ class Reference : public RefBase { inline void Finalize(bool is_env_teardown = false) override { // During env teardown, `~napi_env()` alone is responsible for finalizing. // Thus, we don't want any stray gc passes to trigger a second call to - // `Finalize()`, so let's reset the persistent here if nothing is - // keeping it alive. - if (is_env_teardown && _persistent.IsWeak()) { - _persistent.ClearWeak(); + // `RefBase::Finalize()`. ClearWeak will ensure that even if the + // gc is in progress no Finalization will be run for this Reference + // by the gc. + if (is_env_teardown) { + ClearWeak(); } // Chain up to perform the rest of the finalization. @@ -384,6 +393,35 @@ class Reference : public RefBase { } private: + // ClearWeak is marking the Reference so that the gc should not + // collect it, but it is possible that a second pass callback + // may have been scheduled already if we are in shutdown. We clear + // the secondPassParameter so that even if it has been + // secheduled no Finalization will be run. + inline void ClearWeak() { + if (!_persistent.IsEmpty()) { + _persistent.ClearWeak(); + } + if (_secondPassParameter != nullptr) { + *_secondPassParameter = nullptr; + } + } + + // Mark the reference as weak and eligible for collection + // by the gc. + inline void SetWeak() { + if (_secondPassParameter == nullptr) { + // This means that the Reference has already been processed + // by the second pass calback, so its already been Finalized, do + // nothing + return; + } + _persistent.SetWeak( + _secondPassParameter, FinalizeCallback, + v8::WeakCallbackType::kParameter); + *_secondPassParameter = this; + } + // The N-API finalizer callback may make calls into the engine. V8's heap is // not in a consistent state during the weak callback, and therefore it does // not support calls back into it. However, it provides a mechanism for adding @@ -391,20 +429,46 @@ class Reference : public RefBase { // attach such a second-pass finalizer from the first pass finalizer. Thus, // we do that here to ensure that the N-API finalizer callback is free to call // into the engine. - static void FinalizeCallback(const v8::WeakCallbackInfo& data) { - Reference* reference = data.GetParameter(); + static void FinalizeCallback( + const v8::WeakCallbackInfo& data) { + SecondPassCallParameterRef* parameter = data.GetParameter(); + Reference* reference = *parameter; + if (reference == nullptr) { + return; + } // The reference must be reset during the first pass. reference->_persistent.Reset(); + // Mark the parameter not delete-able until the second pass callback is + // invoked. + reference->_secondPassParameter = nullptr; data.SetSecondPassCallback(SecondPassCallback); } - static void SecondPassCallback(const v8::WeakCallbackInfo& data) { - data.GetParameter()->Finalize(); + // Second pass callbacks are scheduled with platform tasks. At env teardown, + // the tasks may have already be scheduled and we are unable to cancel the + // second pass callback task. We have to make sure that parameter is kept + // alive until the second pass callback is been invoked. In order to do + // this and still allow our code to Finalize/delete the Reference during + // shutdown we have to use a seperately allocated parameter instead + // of a parameter within the Reference object itself. This is what + // is stored in _secondPassParameter and it is alocated in the + // constructor for the Reference. + static void SecondPassCallback( + const v8::WeakCallbackInfo& data) { + SecondPassCallParameterRef* parameter = data.GetParameter(); + Reference* reference = *parameter; + delete parameter; + if (reference == nullptr) { + // the reference itself has already been deleted so nothing to do + return; + } + reference->Finalize(); } v8impl::Persistent _persistent; + SecondPassCallParameterRef* _secondPassParameter; }; class ArrayBufferReference final : public Reference { From 2a8bdd128dd4596df8154c46ec4e8e6755674050 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 15 Apr 2021 09:05:48 -0700 Subject: [PATCH 5/8] src: fix finalization crash PR-URL: https://github.com/nodejs/node/pull/38250 Fixes: https://github.com/nodejs/node/issues/38040 Reviewed-By: Beth Griggs Reviewed-By: Antoine du Hamel --- src/js_native_api_v8.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 8254dc7d6c8aa2..81619b914153c6 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -379,6 +379,9 @@ class Reference : public RefBase { protected: inline void Finalize(bool is_env_teardown = false) override { + if (is_env_teardown) env_teardown_finalize_started_ = true; + if (!is_env_teardown && env_teardown_finalize_started_) return; + // During env teardown, `~napi_env()` alone is responsible for finalizing. // Thus, we don't want any stray gc passes to trigger a second call to // `RefBase::Finalize()`. ClearWeak will ensure that even if the @@ -467,6 +470,7 @@ class Reference : public RefBase { reference->Finalize(); } + bool env_teardown_finalize_started_ = false; v8impl::Persistent _persistent; SecondPassCallParameterRef* _secondPassParameter; }; From 3939bba723fbbbb98d0c1bfc76f37c5bf146c280 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Fri, 30 Apr 2021 17:51:11 -0400 Subject: [PATCH 6/8] node-api: fix shutdown crashes Refs: https://github.com/nodejs/node-addon-api/issues/906 Ensure that finalization is not defered during shutdown. The env for the addon is deleted immediately after iterating the list of finalizers to be run. Defering causes crashes as the finalization uses the already deleted env. Signed-off-by: Michael Dawson PR-URL: https://github.com/nodejs/node/pull/38492 Reviewed-By: Anna Henningsen Reviewed-By: Chengzhong Wu Reviewed-By: Gabriel Schulhof --- src/js_native_api_v8.h | 31 +++++++++++++++++++++++++++++++ src/node_api.cc | 9 +++++++-- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 06b8049ec46db0..95f390b58287e8 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -122,6 +122,37 @@ struct napi_env__ { void* instance_data = nullptr; }; +// This class is used to keep a napi_env live in a way that +// is exception safe versus calling Ref/Unref directly +class EnvRefHolder { + public: + explicit EnvRefHolder(napi_env env) : _env(env) { + _env->Ref(); + } + + explicit EnvRefHolder(const EnvRefHolder& other): _env(other.env()) { + _env->Ref(); + } + + EnvRefHolder(EnvRefHolder&& other) { + _env = other._env; + other._env = nullptr; + } + + ~EnvRefHolder() { + if (_env != nullptr) { + _env->Unref(); + } + } + + napi_env env(void) const { + return _env; + } + + private: + napi_env _env; +}; + static inline napi_status napi_clear_last_error(napi_env env) { env->last_error.error_code = napi_ok; diff --git a/src/node_api.cc b/src/node_api.cc index 211386ba6d502d..a153e5dfcd204d 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -35,8 +35,13 @@ struct node_napi_env__ : public napi_env__ { } void CallFinalizer(napi_finalize cb, void* data, void* hint) override { - napi_env env = static_cast(this); - node_env()->SetImmediate([=](node::Environment* node_env) { + // we need to keep the env live until the finalizer has been run + // EnvRefHolder provides an exception safe wrapper to Ref and then + // Unref once the lamba is freed + EnvRefHolder liveEnv(static_cast(this)); + node_env()->SetImmediate([=, liveEnv = std::move(liveEnv)] + (node::Environment* node_env) { + napi_env env = liveEnv.env(); v8::HandleScope handle_scope(env->isolate); v8::Context::Scope context_scope(env->context()); env->CallIntoModule([&](napi_env env) { From b9d99d7770c60ea419d823220cca5e856c4a2b23 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 1 Jun 2021 21:20:55 -0400 Subject: [PATCH 7/8] node-api: avoid SecondPassCallback crash PR https://github.com/nodejs/node/pull/38000 added indirection so that we could stop finalization in cases where it had been scheduled in a second pass callback but we were doing it in advance in environment teardown. Unforunately we missed that the code which tries to clear the second pass parameter checked if the pointer to the parameter (_secondPassParameter) was nullptr and that when the second pass callback was scheduled we set _secondPassParameter to nullptr in order to avoid it being deleted outside of the second pass callback. The net result was that we would not clear the _secondPassParameter contents and failed to avoid the Finalization in the second pass callback. This PR adds an additional boolean for deciding if the secondPassParameter should be deleted outside of the second pass callback instead of setting secondPassParameter to nullptr thus avoiding the conflict between the 2 ways it was being used. See the discussion starting at: https://github.com/nodejs/node/pull/38273#issuecomment-852403751 for how this was discovered on OSX while trying to upgrade to a new V8 version. Signed-off-by: Michael Dawson PR-URL: https://github.com/nodejs/node/pull/38899 Reviewed-By: Chengzhong Wu Reviewed-By: James M Snell --- src/js_native_api_v8.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 81619b914153c6..8401ed30a64748 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -320,7 +320,8 @@ class Reference : public RefBase { Reference(napi_env env, v8::Local value, Args&&... args) : RefBase(env, std::forward(args)...), _persistent(env->isolate, value), - _secondPassParameter(new SecondPassCallParameterRef(this)) { + _secondPassParameter(new SecondPassCallParameterRef(this)), + _secondPassScheduled(false) { if (RefCount() == 0) { SetWeak(); } @@ -347,7 +348,7 @@ class Reference : public RefBase { // If the second pass callback is scheduled, it will delete the // parameter passed to it, otherwise it will never be scheduled // and we need to delete it here. - if (_secondPassParameter != nullptr) { + if (!_secondPassScheduled) { delete _secondPassParameter; } } @@ -444,8 +445,7 @@ class Reference : public RefBase { reference->_persistent.Reset(); // Mark the parameter not delete-able until the second pass callback is // invoked. - reference->_secondPassParameter = nullptr; - + reference->_secondPassScheduled = true; data.SetSecondPassCallback(SecondPassCallback); } @@ -467,12 +467,14 @@ class Reference : public RefBase { // the reference itself has already been deleted so nothing to do return; } + reference->_secondPassParameter = nullptr; reference->Finalize(); } bool env_teardown_finalize_started_ = false; v8impl::Persistent _persistent; SecondPassCallParameterRef* _secondPassParameter; + bool _secondPassScheduled; }; class ArrayBufferReference final : public Reference { From 0fb5c42b1d278389b0d0a571a1915fd8efef7b01 Mon Sep 17 00:00:00 2001 From: legendecas Date: Tue, 8 Jun 2021 21:53:07 +0800 Subject: [PATCH 8/8] node-api: cctest on v8impl::Reference PR-URL: https://github.com/nodejs/node/pull/38970 Reviewed-By: Michael Dawson Reviewed-By: Gabriel Schulhof --- LICENSE | 2 +- node.gyp | 2 + src/gtest/LICENSE | 28 ++ src/gtest/gtest_prod.h | 61 +++ src/js_native_api_v8.cc | 556 +++++++++++++-------------- src/js_native_api_v8.h | 75 ++++ src/js_native_api_v8_internals.h | 1 + src/node_api.cc | 68 ++-- src/node_api_internals.h | 30 ++ test/cctest/test_js_native_api_v8.cc | 102 +++++ test/cctest/test_node_api.cc | 41 ++ 11 files changed, 635 insertions(+), 331 deletions(-) create mode 100644 src/gtest/LICENSE create mode 100644 src/gtest/gtest_prod.h create mode 100644 src/node_api_internals.h create mode 100644 test/cctest/test_js_native_api_v8.cc create mode 100644 test/cctest/test_node_api.cc diff --git a/LICENSE b/LICENSE index 298631ee09e2dd..e18293b3628173 100644 --- a/LICENSE +++ b/LICENSE @@ -1294,7 +1294,7 @@ The externally maintained libraries used by Node.js are: WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. """ -- gtest, located at test/cctest/gtest, is licensed as follows: +- gtest, located at src/gtest and test/cctest/gtest, is licensed as follows: """ Copyright 2008, Google Inc. All rights reserved. diff --git a/node.gyp b/node.gyp index 48fd7a124a6961..3bb6ce32fab8e5 100644 --- a/node.gyp +++ b/node.gyp @@ -1232,7 +1232,9 @@ 'test/cctest/test_base_object_ptr.cc', 'test/cctest/test_node_postmortem_metadata.cc', 'test/cctest/test_environment.cc', + 'test/cctest/test_js_native_api_v8.cc', 'test/cctest/test_linked_binding.cc', + 'test/cctest/test_node_api.cc', 'test/cctest/test_per_process.cc', 'test/cctest/test_platform.cc', 'test/cctest/test_json_utils.cc', diff --git a/src/gtest/LICENSE b/src/gtest/LICENSE new file mode 100644 index 00000000000000..1941a11f8ce943 --- /dev/null +++ b/src/gtest/LICENSE @@ -0,0 +1,28 @@ +Copyright 2008, Google Inc. +All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are +met: + + * Redistributions of source code must retain the above copyright +notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above +copyright notice, this list of conditions and the following disclaimer +in the documentation and/or other materials provided with the +distribution. + * Neither the name of Google Inc. nor the names of its +contributors may be used to endorse or promote products derived from +this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. diff --git a/src/gtest/gtest_prod.h b/src/gtest/gtest_prod.h new file mode 100644 index 00000000000000..473387f170c838 --- /dev/null +++ b/src/gtest/gtest_prod.h @@ -0,0 +1,61 @@ +// Copyright 2006, Google Inc. +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// +// Google C++ Testing and Mocking Framework definitions useful in production +// code. GOOGLETEST_CM0003 DO NOT DELETE + +#ifndef GOOGLETEST_INCLUDE_GTEST_GTEST_PROD_H_ // NOLINT +#define GOOGLETEST_INCLUDE_GTEST_GTEST_PROD_H_ + +// When you need to test the private or protected members of a class, +// use the FRIEND_TEST macro to declare your tests as friends of the +// class. For example: +// +// class MyClass { +// private: +// void PrivateMethod(); +// FRIEND_TEST(MyClassTest, PrivateMethodWorks); +// }; +// +// class MyClassTest : public testing::Test { +// // ... +// }; +// +// TEST_F(MyClassTest, PrivateMethodWorks) { +// // Can call MyClass::PrivateMethod() here. +// } +// +// Note: The test class must be in the same namespace as the class being tested. +// For example, putting MyClassTest in an anonymous namespace will not work. + +#define FRIEND_TEST(test_case_name, test_name) \ + friend class test_case_name##_##test_name##_Test + +#endif // GOOGLETEST_INCLUDE_GTEST_GTEST_PROD_H_ // NOLINT diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 8401ed30a64748..d8239fb1cb1c36 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -188,295 +188,6 @@ inline static napi_status ConcludeDeferred(napi_env env, return GET_RETURN_STATUS(env); } -// Wrapper around v8impl::Persistent that implements reference counting. -class RefBase : protected Finalizer, RefTracker { - protected: - RefBase(napi_env env, - uint32_t initial_refcount, - bool delete_self, - napi_finalize finalize_callback, - void* finalize_data, - void* finalize_hint) - : Finalizer(env, finalize_callback, finalize_data, finalize_hint), - _refcount(initial_refcount), - _delete_self(delete_self) { - Link(finalize_callback == nullptr - ? &env->reflist - : &env->finalizing_reflist); - } - - public: - static RefBase* New(napi_env env, - uint32_t initial_refcount, - bool delete_self, - napi_finalize finalize_callback, - void* finalize_data, - void* finalize_hint) { - return new RefBase(env, - initial_refcount, - delete_self, - finalize_callback, - finalize_data, - finalize_hint); - } - - inline void* Data() { - return _finalize_data; - } - - // 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 or - // cannot have been queued to run (ie 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 - // 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 inline void Delete(RefBase* reference) { - reference->Unlink(); - if ((reference->RefCount() != 0) || - (reference->_delete_self) || - (reference->_finalize_ran)) { - delete reference; - } else { - // defer until finalizer runs as - // it may alread be queued - reference->_delete_self = true; - } - } - - inline uint32_t Ref() { - return ++_refcount; - } - - inline uint32_t Unref() { - if (_refcount == 0) { - return 0; - } - return --_refcount; - } - - inline uint32_t RefCount() { - return _refcount; - } - - protected: - inline void Finalize(bool is_env_teardown = false) override { - // In addition to being called during environment teardown, this method is - // also the entry point for the garbage collector. During environment - // teardown we have to remove the garbage collector's reference to this - // method so that, if, as part of the user's callback, JS gets executed, - // resulting in a garbage collection pass, this method is not re-entered as - // part of that pass, because that'll cause a double free (as seen in - // https://github.com/nodejs/node/issues/37236). - // - // Since this class does not have access to the V8 persistent reference, - // this method is overridden in the `Reference` class below. Therein the - // weak callback is removed, ensuring that the garbage collector does not - // re-enter this method, and the method chains up to continue the process of - // environment-teardown-induced finalization. - - // During environment teardown we have to convert a strong reference to - // a weak reference to force the deferring behavior if the user's finalizer - // happens to delete this reference so that the code in this function that - // follows the call to the user's finalizer may safely access variables from - // this instance. - if (is_env_teardown && RefCount() > 0) _refcount = 0; - - if (_finalize_callback != nullptr) { - // This ensures that we never call the finalizer twice. - napi_finalize fini = _finalize_callback; - _finalize_callback = nullptr; - _env->CallFinalizer(fini, _finalize_data, _finalize_hint); - } - - // 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 (_delete_self || is_env_teardown) { - Delete(this); - } else { - _finalize_ran = true; - } - } - - private: - uint32_t _refcount; - bool _delete_self; -}; - -class Reference : public RefBase { - using SecondPassCallParameterRef = Reference*; - - protected: - template - Reference(napi_env env, v8::Local value, Args&&... args) - : RefBase(env, std::forward(args)...), - _persistent(env->isolate, value), - _secondPassParameter(new SecondPassCallParameterRef(this)), - _secondPassScheduled(false) { - if (RefCount() == 0) { - SetWeak(); - } - } - - public: - static inline Reference* New(napi_env env, - v8::Local value, - uint32_t initial_refcount, - bool delete_self, - napi_finalize finalize_callback = nullptr, - void* finalize_data = nullptr, - void* finalize_hint = nullptr) { - return new Reference(env, - value, - initial_refcount, - delete_self, - finalize_callback, - finalize_data, - finalize_hint); - } - - virtual ~Reference() { - // If the second pass callback is scheduled, it will delete the - // parameter passed to it, otherwise it will never be scheduled - // and we need to delete it here. - if (!_secondPassScheduled) { - delete _secondPassParameter; - } - } - - inline uint32_t Ref() { - uint32_t refcount = RefBase::Ref(); - if (refcount == 1) { - ClearWeak(); - } - return refcount; - } - - inline uint32_t Unref() { - uint32_t old_refcount = RefCount(); - uint32_t refcount = RefBase::Unref(); - if (old_refcount == 1 && refcount == 0) { - SetWeak(); - } - return refcount; - } - - inline v8::Local Get() { - if (_persistent.IsEmpty()) { - return v8::Local(); - } else { - return v8::Local::New(_env->isolate, _persistent); - } - } - - protected: - inline void Finalize(bool is_env_teardown = false) override { - if (is_env_teardown) env_teardown_finalize_started_ = true; - if (!is_env_teardown && env_teardown_finalize_started_) return; - - // During env teardown, `~napi_env()` alone is responsible for finalizing. - // Thus, we don't want any stray gc passes to trigger a second call to - // `RefBase::Finalize()`. ClearWeak will ensure that even if the - // gc is in progress no Finalization will be run for this Reference - // by the gc. - if (is_env_teardown) { - ClearWeak(); - } - - // Chain up to perform the rest of the finalization. - RefBase::Finalize(is_env_teardown); - } - - private: - // ClearWeak is marking the Reference so that the gc should not - // collect it, but it is possible that a second pass callback - // may have been scheduled already if we are in shutdown. We clear - // the secondPassParameter so that even if it has been - // secheduled no Finalization will be run. - inline void ClearWeak() { - if (!_persistent.IsEmpty()) { - _persistent.ClearWeak(); - } - if (_secondPassParameter != nullptr) { - *_secondPassParameter = nullptr; - } - } - - // Mark the reference as weak and eligible for collection - // by the gc. - inline void SetWeak() { - if (_secondPassParameter == nullptr) { - // This means that the Reference has already been processed - // by the second pass calback, so its already been Finalized, do - // nothing - return; - } - _persistent.SetWeak( - _secondPassParameter, FinalizeCallback, - v8::WeakCallbackType::kParameter); - *_secondPassParameter = this; - } - - // The N-API finalizer callback may make calls into the engine. V8's heap is - // not in a consistent state during the weak callback, and therefore it does - // not support calls back into it. However, it provides a mechanism for adding - // a finalizer which may make calls back into the engine by allowing us to - // attach such a second-pass finalizer from the first pass finalizer. Thus, - // we do that here to ensure that the N-API finalizer callback is free to call - // into the engine. - static void FinalizeCallback( - const v8::WeakCallbackInfo& data) { - SecondPassCallParameterRef* parameter = data.GetParameter(); - Reference* reference = *parameter; - if (reference == nullptr) { - return; - } - - // The reference must be reset during the first pass. - reference->_persistent.Reset(); - // Mark the parameter not delete-able until the second pass callback is - // invoked. - reference->_secondPassScheduled = true; - data.SetSecondPassCallback(SecondPassCallback); - } - - // Second pass callbacks are scheduled with platform tasks. At env teardown, - // the tasks may have already be scheduled and we are unable to cancel the - // second pass callback task. We have to make sure that parameter is kept - // alive until the second pass callback is been invoked. In order to do - // this and still allow our code to Finalize/delete the Reference during - // shutdown we have to use a seperately allocated parameter instead - // of a parameter within the Reference object itself. This is what - // is stored in _secondPassParameter and it is alocated in the - // constructor for the Reference. - static void SecondPassCallback( - const v8::WeakCallbackInfo& data) { - SecondPassCallParameterRef* parameter = data.GetParameter(); - Reference* reference = *parameter; - delete parameter; - if (reference == nullptr) { - // the reference itself has already been deleted so nothing to do - return; - } - reference->_secondPassParameter = nullptr; - reference->Finalize(); - } - - bool env_teardown_finalize_started_ = false; - v8impl::Persistent _persistent; - SecondPassCallParameterRef* _secondPassParameter; - bool _secondPassScheduled; -}; - class ArrayBufferReference final : public Reference { public: // Same signatures for ctor and New() as Reference, except this only works @@ -823,6 +534,273 @@ inline napi_status Wrap(napi_env env, } // end of anonymous namespace +// Wrapper around v8impl::Persistent that implements reference counting. +RefBase::RefBase(napi_env env, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) + : Finalizer(env, finalize_callback, finalize_data, finalize_hint), + _refcount(initial_refcount), + _delete_self(delete_self) { + Link(finalize_callback == nullptr ? &env->reflist : &env->finalizing_reflist); +} + +RefBase* RefBase::New(napi_env env, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) { + return new RefBase(env, + initial_refcount, + delete_self, + finalize_callback, + finalize_data, + finalize_hint); +} + +RefBase::~RefBase() { + Unlink(); +} + +void* RefBase::Data() { + return _finalize_data; +} + +// 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 or +// cannot have been queued to run (ie 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 +// 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. +void RefBase::Delete(RefBase* reference) { + if ((reference->RefCount() != 0) || (reference->_delete_self) || + (reference->_finalize_ran)) { + delete reference; + } else { + // defer until finalizer runs as + // it may already be queued + reference->_delete_self = true; + } +} + +uint32_t RefBase::Ref() { + return ++_refcount; +} + +uint32_t RefBase::Unref() { + if (_refcount == 0) { + return 0; + } + return --_refcount; +} + +uint32_t RefBase::RefCount() { + return _refcount; +} + +void RefBase::Finalize(bool is_env_teardown) { + // In addition to being called during environment teardown, this method is + // also the entry point for the garbage collector. During environment + // teardown we have to remove the garbage collector's reference to this + // method so that, if, as part of the user's callback, JS gets executed, + // resulting in a garbage collection pass, this method is not re-entered as + // part of that pass, because that'll cause a double free (as seen in + // https://github.com/nodejs/node/issues/37236). + // + // Since this class does not have access to the V8 persistent reference, + // this method is overridden in the `Reference` class below. Therein the + // weak callback is removed, ensuring that the garbage collector does not + // re-enter this method, and the method chains up to continue the process of + // environment-teardown-induced finalization. + + // During environment teardown we have to convert a strong reference to + // a weak reference to force the deferring behavior if the user's finalizer + // happens to delete this reference so that the code in this function that + // follows the call to the user's finalizer may safely access variables from + // this instance. + if (is_env_teardown && RefCount() > 0) _refcount = 0; + + if (_finalize_callback != nullptr) { + // This ensures that we never call the finalizer twice. + napi_finalize fini = _finalize_callback; + _finalize_callback = nullptr; + _env->CallFinalizer(fini, _finalize_data, _finalize_hint); + } + + // 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 (_delete_self || is_env_teardown) { + Delete(this); + } else { + _finalize_ran = true; + } +} + +template +Reference::Reference(napi_env env, v8::Local value, Args&&... args) + : RefBase(env, std::forward(args)...), + _persistent(env->isolate, value), + _secondPassParameter(new SecondPassCallParameterRef(this)), + _secondPassScheduled(false) { + if (RefCount() == 0) { + SetWeak(); + } +} + +Reference* Reference::New(napi_env env, + v8::Local value, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint) { + return new Reference(env, + value, + initial_refcount, + delete_self, + finalize_callback, + finalize_data, + finalize_hint); +} + +Reference::~Reference() { + // If the second pass callback is scheduled, it will delete the + // parameter passed to it, otherwise it will never be scheduled + // and we need to delete it here. + if (!_secondPassScheduled) { + delete _secondPassParameter; + } +} + +uint32_t Reference::Ref() { + uint32_t refcount = RefBase::Ref(); + if (refcount == 1) { + ClearWeak(); + } + return refcount; +} + +uint32_t Reference::Unref() { + uint32_t old_refcount = RefCount(); + uint32_t refcount = RefBase::Unref(); + if (old_refcount == 1 && refcount == 0) { + SetWeak(); + } + return refcount; +} + +v8::Local Reference::Get() { + if (_persistent.IsEmpty()) { + return v8::Local(); + } else { + return v8::Local::New(_env->isolate, _persistent); + } +} + +void Reference::Finalize(bool is_env_teardown) { + if (is_env_teardown) env_teardown_finalize_started_ = true; + if (!is_env_teardown && env_teardown_finalize_started_) return; + + // During env teardown, `~napi_env()` alone is responsible for finalizing. + // Thus, we don't want any stray gc passes to trigger a second call to + // `RefBase::Finalize()`. ClearWeak will ensure that even if the + // gc is in progress no Finalization will be run for this Reference + // by the gc. + if (is_env_teardown) { + ClearWeak(); + } + + // Chain up to perform the rest of the finalization. + RefBase::Finalize(is_env_teardown); +} + +// ClearWeak is marking the Reference so that the gc should not +// collect it, but it is possible that a second pass callback +// may have been scheduled already if we are in shutdown. We clear +// the secondPassParameter so that even if it has been +// scheduled no Finalization will be run. +void Reference::ClearWeak() { + if (!_persistent.IsEmpty()) { + _persistent.ClearWeak(); + } + if (_secondPassParameter != nullptr) { + *_secondPassParameter = nullptr; + } +} + +// Mark the reference as weak and eligible for collection +// by the gc. +void Reference::SetWeak() { + if (_secondPassParameter == nullptr) { + // This means that the Reference has already been processed + // by the second pass callback, so its already been Finalized, do + // nothing + return; + } + _persistent.SetWeak( + _secondPassParameter, FinalizeCallback, v8::WeakCallbackType::kParameter); + *_secondPassParameter = this; +} + +// The N-API finalizer callback may make calls into the engine. V8's heap is +// not in a consistent state during the weak callback, and therefore it does +// not support calls back into it. However, it provides a mechanism for adding +// a finalizer which may make calls back into the engine by allowing us to +// attach such a second-pass finalizer from the first pass finalizer. Thus, +// we do that here to ensure that the N-API finalizer callback is free to call +// into the engine. +void Reference::FinalizeCallback( + const v8::WeakCallbackInfo& data) { + SecondPassCallParameterRef* parameter = data.GetParameter(); + Reference* reference = *parameter; + if (reference == nullptr) { + return; + } + + // The reference must be reset during the first pass. + reference->_persistent.Reset(); + // Mark the parameter not delete-able until the second pass callback is + // invoked. + reference->_secondPassScheduled = true; + + data.SetSecondPassCallback(SecondPassCallback); +} + +// Second pass callbacks are scheduled with platform tasks. At env teardown, +// the tasks may have already be scheduled and we are unable to cancel the +// second pass callback task. We have to make sure that parameter is kept +// alive until the second pass callback is been invoked. In order to do +// this and still allow our code to Finalize/delete the Reference during +// shutdown we have to use a separately allocated parameter instead +// of a parameter within the Reference object itself. This is what +// is stored in _secondPassParameter and it is allocated in the +// constructor for the Reference. +void Reference::SecondPassCallback( + const v8::WeakCallbackInfo& data) { + SecondPassCallParameterRef* parameter = data.GetParameter(); + Reference* reference = *parameter; + delete parameter; + if (reference == nullptr) { + // the reference itself has already been deleted so nothing to do + return; + } + reference->_secondPassParameter = nullptr; + reference->Finalize(); +} + } // end of namespace v8impl // Warning: Keep in-sync with napi_status enum diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 95f390b58287e8..1ffbce8be2cefa 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -366,6 +366,81 @@ class TryCatch : public v8::TryCatch { napi_env _env; }; +// Wrapper around v8impl::Persistent that implements reference counting. +class RefBase : protected Finalizer, RefTracker { + protected: + RefBase(napi_env env, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint); + + public: + static RefBase* New(napi_env env, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback, + void* finalize_data, + void* finalize_hint); + + static inline void Delete(RefBase* reference); + + virtual ~RefBase(); + void* Data(); + uint32_t Ref(); + uint32_t Unref(); + uint32_t RefCount(); + + protected: + void Finalize(bool is_env_teardown = false) override; + + private: + uint32_t _refcount; + bool _delete_self; +}; + +class Reference : public RefBase { + using SecondPassCallParameterRef = Reference*; + + protected: + template + Reference(napi_env env, v8::Local value, Args&&... args); + + public: + static Reference* New(napi_env env, + v8::Local value, + uint32_t initial_refcount, + bool delete_self, + napi_finalize finalize_callback = nullptr, + void* finalize_data = nullptr, + void* finalize_hint = nullptr); + + virtual ~Reference(); + uint32_t Ref(); + uint32_t Unref(); + v8::Local Get(); + + protected: + void Finalize(bool is_env_teardown = false) override; + + private: + void ClearWeak(); + void SetWeak(); + + static void FinalizeCallback( + const v8::WeakCallbackInfo& data); + static void SecondPassCallback( + const v8::WeakCallbackInfo& data); + + bool env_teardown_finalize_started_ = false; + v8impl::Persistent _persistent; + SecondPassCallParameterRef* _secondPassParameter; + bool _secondPassScheduled; + + FRIEND_TEST(JsNativeApiV8Test, Reference); +}; + } // end of namespace v8impl #endif // SRC_JS_NATIVE_API_V8_H_ diff --git a/src/js_native_api_v8_internals.h b/src/js_native_api_v8_internals.h index ddd219818cdfa9..8428390ef1eaf3 100644 --- a/src/js_native_api_v8_internals.h +++ b/src/js_native_api_v8_internals.h @@ -16,6 +16,7 @@ #include "node_version.h" #include "env.h" #include "node_internals.h" +#include "gtest/gtest_prod.h" #define NAPI_ARRAYSIZE(array) \ node::arraysize((array)) diff --git a/src/node_api.cc b/src/node_api.cc index a153e5dfcd204d..d5e05bcdd96ed0 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -2,6 +2,7 @@ #define NAPI_EXPERIMENTAL #include "js_native_api_v8.h" #include "node_api.h" +#include "node_api_internals.h" #include "node_binding.h" #include "node_buffer.h" #include "node_errors.h" @@ -11,51 +12,36 @@ #include -struct node_napi_env__ : public napi_env__ { - explicit node_napi_env__(v8::Local context, - const std::string& module_filename): - napi_env__(context), filename(module_filename) { - CHECK_NOT_NULL(node_env()); - } - - inline node::Environment* node_env() const { - return node::Environment::GetCurrent(context()); - } +node_napi_env__::node_napi_env__(v8::Local context, + const std::string& module_filename) + : napi_env__(context), filename(module_filename) { + CHECK_NOT_NULL(node_env()); +} - bool can_call_into_js() const override { - return node_env()->can_call_into_js(); - } +bool node_napi_env__::can_call_into_js() const { + return node_env()->can_call_into_js(); +} - v8::Maybe mark_arraybuffer_as_untransferable( - v8::Local ab) const override { - return ab->SetPrivate( - context(), - node_env()->untransferable_object_private_symbol(), - v8::True(isolate)); - } +v8::Maybe node_napi_env__::mark_arraybuffer_as_untransferable( + v8::Local ab) const { + return ab->SetPrivate(context(), + node_env()->untransferable_object_private_symbol(), + v8::True(isolate)); +} - void CallFinalizer(napi_finalize cb, void* data, void* hint) override { - // we need to keep the env live until the finalizer has been run - // EnvRefHolder provides an exception safe wrapper to Ref and then - // Unref once the lamba is freed - EnvRefHolder liveEnv(static_cast(this)); - node_env()->SetImmediate([=, liveEnv = std::move(liveEnv)] - (node::Environment* node_env) { - napi_env env = liveEnv.env(); - v8::HandleScope handle_scope(env->isolate); - v8::Context::Scope context_scope(env->context()); - env->CallIntoModule([&](napi_env env) { - cb(env, data, hint); +void node_napi_env__::CallFinalizer(napi_finalize cb, void* data, void* hint) { + // we need to keep the env live until the finalizer has been run + // EnvRefHolder provides an exception safe wrapper to Ref and then + // Unref once the lamba is freed + EnvRefHolder liveEnv(static_cast(this)); + node_env()->SetImmediate( + [=, liveEnv = std::move(liveEnv)](node::Environment* node_env) { + napi_env env = liveEnv.env(); + v8::HandleScope handle_scope(env->isolate); + v8::Context::Scope context_scope(env->context()); + env->CallIntoModule([&](napi_env env) { cb(env, data, hint); }); }); - }); - } - - const char* GetFilename() const { return filename.c_str(); } - - std::string filename; -}; - -typedef node_napi_env__* node_napi_env; +} namespace v8impl { diff --git a/src/node_api_internals.h b/src/node_api_internals.h new file mode 100644 index 00000000000000..318ada38083435 --- /dev/null +++ b/src/node_api_internals.h @@ -0,0 +1,30 @@ +#ifndef SRC_NODE_API_INTERNALS_H_ +#define SRC_NODE_API_INTERNALS_H_ + +#include "v8.h" +#define NAPI_EXPERIMENTAL +#include "env-inl.h" +#include "js_native_api_v8.h" +#include "node_api.h" +#include "util-inl.h" + +struct node_napi_env__ : public napi_env__ { + node_napi_env__(v8::Local context, + const std::string& module_filename); + + bool can_call_into_js() const override; + v8::Maybe mark_arraybuffer_as_untransferable( + v8::Local ab) const override; + void CallFinalizer(napi_finalize cb, void* data, void* hint) override; + + inline node::Environment* node_env() const { + return node::Environment::GetCurrent(context()); + } + inline const char* GetFilename() const { return filename.c_str(); } + + std::string filename; +}; + +using node_napi_env = node_napi_env__*; + +#endif // SRC_NODE_API_INTERNALS_H_ diff --git a/test/cctest/test_js_native_api_v8.cc b/test/cctest/test_js_native_api_v8.cc new file mode 100644 index 00000000000000..2d95c86d09d5d5 --- /dev/null +++ b/test/cctest/test_js_native_api_v8.cc @@ -0,0 +1,102 @@ +#include +#include +#include +#include "env-inl.h" +#include "gtest/gtest.h" +#include "js_native_api_v8.h" +#include "node_api_internals.h" +#include "node_binding.h" +#include "node_test_fixture.h" + +namespace v8impl { + +using v8::Local; +using v8::Object; + +static napi_env addon_env; +static uint32_t finalizer_call_count = 0; + +class JsNativeApiV8Test : public EnvironmentTestFixture { + private: + void SetUp() override { + EnvironmentTestFixture::SetUp(); + finalizer_call_count = 0; + } + + void TearDown() override { NodeTestFixture::TearDown(); } +}; + +TEST_F(JsNativeApiV8Test, Reference) { + const v8::HandleScope handle_scope(isolate_); + Argv argv; + + napi_ref ref; + void* embedder_fields[v8::kEmbedderFieldsInWeakCallback] = {nullptr, nullptr}; + v8::WeakCallbackInfo::Callback + callback; + Reference::SecondPassCallParameterRef* parameter = nullptr; + + { + Env test_env{handle_scope, argv}; + + node::Environment* env = *test_env; + node::LoadEnvironment(env, ""); + + napi_addon_register_func init = [](napi_env env, napi_value exports) { + addon_env = env; + return exports; + }; + Local module_obj = Object::New(isolate_); + Local exports_obj = Object::New(isolate_); + napi_module_register_by_symbol( + exports_obj, module_obj, env->context(), init); + ASSERT_NE(addon_env, nullptr); + node_napi_env internal_env = reinterpret_cast(addon_env); + EXPECT_EQ(internal_env->node_env(), env); + + // Create a new scope to manage the handles. + { + const v8::HandleScope handle_scope(isolate_); + napi_value value; + napi_create_object(addon_env, &value); + // Create a weak reference; + napi_add_finalizer( + addon_env, + value, + nullptr, + [](napi_env env, void* finalize_data, void* finalize_hint) { + finalizer_call_count++; + }, + nullptr, + &ref); + parameter = reinterpret_cast(ref)->_secondPassParameter; + } + + // We can hardly trigger a non-forced Garbage Collection in a stable way. + // Here we just invoke the weak callbacks directly. + // The persistant handles should be reset in the weak callback in respect + // to the API contract of v8 weak callbacks. + v8::WeakCallbackInfo data( + reinterpret_cast(isolate_), + parameter, + embedder_fields, + &callback); + Reference::FinalizeCallback(data); + EXPECT_EQ(callback, &Reference::SecondPassCallback); + } + // Env goes out of scope, the environment has been teardown. All node-api ref + // trackers should have been destroyed. + + // Now we call the second pass callback to verify the method do not abort with + // memory violations. + v8::WeakCallbackInfo data( + reinterpret_cast(isolate_), + parameter, + embedder_fields, + nullptr); + Reference::SecondPassCallback(data); + + // After Environment Teardown + EXPECT_EQ(finalizer_call_count, uint32_t(1)); +} +} // namespace v8impl diff --git a/test/cctest/test_node_api.cc b/test/cctest/test_node_api.cc new file mode 100644 index 00000000000000..ad5be52fc8ffcc --- /dev/null +++ b/test/cctest/test_node_api.cc @@ -0,0 +1,41 @@ +#include +#include +#include +#include "env-inl.h" +#include "gtest/gtest.h" +#include "node_api_internals.h" +#include "node_binding.h" +#include "node_test_fixture.h" + +using v8::Local; +using v8::Object; + +static napi_env addon_env; + +class NodeApiTest : public EnvironmentTestFixture { + private: + void SetUp() override { EnvironmentTestFixture::SetUp(); } + + void TearDown() override { NodeTestFixture::TearDown(); } +}; + +TEST_F(NodeApiTest, CreateNodeApiEnv) { + const v8::HandleScope handle_scope(isolate_); + Argv argv; + + Env test_env{handle_scope, argv}; + + node::Environment* env = *test_env; + node::LoadEnvironment(env, ""); + + napi_addon_register_func init = [](napi_env env, napi_value exports) { + addon_env = env; + return exports; + }; + Local module_obj = Object::New(isolate_); + Local exports_obj = Object::New(isolate_); + napi_module_register_by_symbol(exports_obj, module_obj, env->context(), init); + ASSERT_NE(addon_env, nullptr); + node_napi_env internal_env = reinterpret_cast(addon_env); + EXPECT_EQ(internal_env->node_env(), env); +}