Skip to content

fixes v8 GC access violation for zombie objects #638

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

Closed
blagoev wants to merge 5 commits into from
Closed

fixes v8 GC access violation for zombie objects #638

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7c59fab
fix v8 GC access violation after napi ObjectWrap instance is left in …
blagoev Dec 20, 2019
beb4920
handle exceptions disabled case
blagoev Dec 24, 2019
80d6607
specify return value type
blagoev Dec 24, 2019
41c7a89
add a test for constructor exceptions
blagoev Dec 30, 2019
253a9e1
refactored constructor exceptions fix.
blagoev Dec 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
handle exceptions disabled case
  • Loading branch information
@blagoev
blagoev committed Dec 24, 2019
commit beb4920e723a14ca91b1ea3611e7a561ffabe4ec
9 changes: 9 additions & 0 deletions napi-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3700,6 +3700,7 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
napi_value wrapper = details::WrapCallback([&] {
CallbackInfo callbackInfo(env, info);
callbackInfo.zombie = new Zombie();
Copy link
Copy Markdown
Contributor

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

Suggested change
callbackInfo.zombie = new Zombie();
ObjectWrapPingPong ping_pong({callbackInfo.Data(), false});
callbackInfo.SetData(&ping_pong);

#ifdef NAPI_CPP_EXCEPTIONS
try {
instance = new T(callbackInfo);
return callbackInfo.This();
Expand All @@ -3708,6 +3709,14 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
callbackInfo.zombie->isZombie = true;
Copy link
Copy Markdown
Contributor

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

Suggested change
callbackInfo.zombie->isZombie = true;
if (ping_pong.object_wrapping_failed == false) {
napi_status status = napi_remove_wrap(callbackInfo.Env(), callbackInfo.This(), nullptr);
NAPI_FATAL_IF_FAILED(status,
"ObjectWrap<T>::ConstructorCallbackWrapper",
"Failed to remove wrap from failed ObjectWrap instance construction");
}

throw;
}
#else
instance = new T(callbackInfo);
if (callbackInfo.Env().IsExceptionPending()) {
callbackInfo.zombie->isZombie = true;
Copy link
Copy Markdown
Contributor

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

Suggested change
callbackInfo.zombie->isZombie = true;
if (ping_pong.object_wrapping_failed == false) {
napi_status status = napi_remove_wrap(callbackInfo.Env(), callbackInfo.This(), nullptr);
NAPI_FATAL_IF_FAILED(status,
"ObjectWrap<T>::ConstructorCallbackWrapper",
"Failed to remove wrap from failed ObjectWrap instance construction");
}

I guess this part should probably be factored out and a call made to it from above and from here.

return nullptr;
Copy link
Copy Markdown
Contributor

@gabrielschulhof gabrielschulhof Dec 30, 2019

Choose a reason for hiding this comment

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

Suggested change
return nullptr;

It's OK to fall through to the return below.

}
return callbackInfo.This();
#endif
});

return wrapper;
Expand Down