Skip to content

Commit

Permalink
node-api: avoid SecondPassCallback crash
Browse files Browse the repository at this point in the history
PR #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:
#38273 (comment)
for how this was discovered on OSX while trying to
upgrade to a new V8 version.

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #38899
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
mhdawson authored and targos committed Jun 8, 2021
1 parent 07339ec commit 65a7fd3
Showing 1 changed file with 6 additions and 4 deletions.
10 changes: 6 additions & 4 deletions src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,8 @@ class Reference : public RefBase {
Reference(napi_env env, v8::Local<v8::Value> value, Args&&... args)
: RefBase(env, std::forward<Args>(args)...),
_persistent(env->isolate, value),
_secondPassParameter(new SecondPassCallParameterRef(this)) {
_secondPassParameter(new SecondPassCallParameterRef(this)),
_secondPassScheduled(false) {
if (RefCount() == 0) {
SetWeak();
}
Expand All @@ -348,7 +349,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;
}
}
Expand Down Expand Up @@ -445,8 +446,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);
}

Expand All @@ -468,12 +468,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<v8::Value> _persistent;
SecondPassCallParameterRef* _secondPassParameter;
bool _secondPassScheduled;
};

enum UnwrapAction {
Expand Down

0 comments on commit 65a7fd3

Please sign in to comment.