Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

node-api: avoid SecondPassCallback crash #38899

Closed
wants to merge 1 commit into from
Closed

node-api: avoid SecondPassCallback crash #38899

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/js_native_api_v8.cc
View file
Open in desktop
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;
Copy link
Member Author

@mhdawson mhdawson Jun 3, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legendecas removing this is the line and using the boolean flag instead is what resolves the issue. The with this line is that it sets the reference->_secondPassParameter to nullptr which means that when we later try to clear what it points to its null and we con't actually clear it.

Putting that in another way the old flow was

-> set reference->_secondPassParameter to nullptr
-> schedule second pass callback
-> start env teardown and decide we want to try to cancel the Finalization in the second pass callback. To do that we need reference->_secondPassParameter but it has already been set to null so we do nothing....
-> the second pass callback still runs Finalization even though it should have been aborted.

This patch avoids setting reference->_secondPassParameter to null too early.
->

Copy link
Member

@legendecas legendecas Jun 4, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your explanation. IIUC, This line transfers the ownership of secondPassParameter to the second pass callback, but didn't transfer the ownership of the content of secondPassParameter (i.e. the v8impl::Reference) to the callback. In that sense, I'm afraid I didn't get the point how the change clears the content of secondPassParameter on the env teardown to prevent second pass callback to finalize.


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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is this branch that early breaks from the SecondPassCallback. However, it seems that we did fail to reach to this branch.

}
reference->_secondPassParameter = nullptr;
reference->Finalize();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the problem happens here as the SecondPassCallParameterRef ownership is been transferred to the SecondPass weak parameter but its content (v8impl::Reference) is already been destroyed by env teardown. I'm afraid I did not understand how the patch could fix the problem?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legendecas see comment above which explains how the fix addresses the issue.

}

bool env_teardown_finalize_started_ = false;
v8impl::Persistent<v8::Value> _persistent;
SecondPassCallParameterRef* _secondPassParameter;
bool _secondPassScheduled;
};

enum UnwrapAction {
Expand Down