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

create a HandleScope in FinalizeCallback #832

Closed
wants to merge 2 commits into from
Closed

create a HandleScope in FinalizeCallback #832

wants to merge 2 commits into from

Conversation

blagoev
Copy link
Contributor

@blagoev blagoev commented Nov 16, 2020 •
edited
Loading

Seems like FinalizeCallback needs to create a HandleScope since it calls ObjectWrap::~ObjectWrap() which might need to create a temporary Object handle here https://github.com/nodejs/node-addon-api/blob/master/napi-inl.h#L3558

at Realm (https://github.com/realm/realm-js) we have crashes with stacktrace at this location.

@blagoev
Copy link
Contributor Author

blagoev commented Nov 17, 2020

seems like clang-format fails on the whole napi-inl.h file and is not related to the changes in this PR

@legendecas legendecas mentioned this pull request Nov 17, 2020
Merged
@KevinEady KevinEady mentioned this pull request Nov 18, 2020
Merged
2 tasks
@mhdawson mhdawson mentioned this pull request Nov 18, 2020
Closed
9 tasks
@mhdawson
Copy link
Member

@blagoev could you add the stack trace to the issue? It would be good to understand the path that leads to the case where there is no HandleScope already on the stack.

@gabrielschulhof
Copy link
Contributor

@mhdawson I think this needs to be fixed in core. This fix is certainly not incorrect, but @KevinEady's backtraces in #742 indicate that there is no handle scope when the napi_env is iterating over outstanding references during ~napi_env, because each reference's Finalize() is being called without env->CallIntoModule(). We can have both fixes for sure.

@gabrielschulhof
Copy link
Contributor

@mhdawson nevermind. This was fixed in core (nodejs/node#34366) in RunAndClearNativeImmediates, which is part of @KevinEady's stack.

gabrielschulhof
gabrielschulhof approved these changes Nov 19, 2020
View reviewed changes
Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

We need this for older versions of Node.js 👍

@blagoev
Copy link
Contributor Author

blagoev commented Nov 19, 2020

yes I can confirm the crash was happening when closing the application which might be where ~napi_env is called.

mhdawson
mhdawson reviewed Nov 20, 2020
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

NickNaso
NickNaso approved these changes Nov 20, 2020
View reviewed changes
Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

LGTM

KevinEady
KevinEady approved these changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

I think this is just missing #819 in terms of the lint checks. As far as I can see it passes in my environment. Going to land.

@mhdawson mhdawson removed the request for review from legendecas November 20, 2020 19:33
mhdawson pushed a commit that referenced this pull request Nov 20, 2020
@blagoev @mhdawson
src: create a HandleScope in FinalizeCallback
1427b3e 
Refs: #832

Seems like FinalizeCallback needs to create a HandleScope
since it calls ObjectWrap::~ObjectWrap() which might need
to create a temporary Object handle here
https://github.com/nodejs/node-addon-api/blob/master/napi-inl.h#L3558

at Realm (https://github.com/realm/realm-js) we have crashes
with stacktrace at this location.

While fixed in core for later versions we still need for
older versions of Node.js

PR-URL: #832
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
@mhdawson
Copy link
Member

Landed as 1427b3e

@mhdawson mhdawson closed this Nov 20, 2020
blagoev added a commit to realm/realm-js that referenced this pull request Nov 24, 2020
@blagoev
use custom node-addon until official release for node-addon-api@3.0.3…
76de29b 
… to use this fix nodejs/node-addon-api#832
@blagoev blagoev mentioned this pull request Nov 24, 2020
Merged
blagoev added a commit to realm/realm-js that referenced this pull request Nov 25, 2020
@blagoev
use custom node-addon until official release for node-addon-api@3.0.3…
0e8161a 
… to use this fix nodejs/node-addon-api#832 (#3419)
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
@kevindavies8
src: create a HandleScope in FinalizeCallback
64e8238 
Refs: nodejs/node-addon-api#832

Seems like FinalizeCallback needs to create a HandleScope
since it calls ObjectWrap::~ObjectWrap() which might need
to create a temporary Object handle here
https://github.com/nodejs/node-addon-api/blob/master/napi-inl.h#L3558

at Realm (https://github.com/realm/realm-js) we have crashes
with stacktrace at this location.

While fixed in core for later versions we still need for
older versions of Node.js

PR-URL: nodejs/node-addon-api#832
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
@Marlyfleitas
src: create a HandleScope in FinalizeCallback
646822e 
Refs: nodejs/node-addon-api#832

Seems like FinalizeCallback needs to create a HandleScope
since it calls ObjectWrap::~ObjectWrap() which might need
to create a temporary Object handle here
https://github.com/nodejs/node-addon-api/blob/master/napi-inl.h#L3558

at Realm (https://github.com/realm/realm-js) we have crashes
with stacktrace at this location.

While fixed in core for later versions we still need for
older versions of Node.js

PR-URL: nodejs/node-addon-api#832
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
@wroy7860
src: create a HandleScope in FinalizeCallback
bd80884 
Refs: nodejs/node-addon-api#832

Seems like FinalizeCallback needs to create a HandleScope
since it calls ObjectWrap::~ObjectWrap() which might need
to create a temporary Object handle here
https://github.com/nodejs/node-addon-api/blob/master/napi-inl.h#L3558

at Realm (https://github.com/realm/realm-js) we have crashes
with stacktrace at this location.

While fixed in core for later versions we still need for
older versions of Node.js

PR-URL: nodejs/node-addon-api#832
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
src: create a HandleScope in FinalizeCallback
f508c02 
Refs: nodejs/node-addon-api#832

Seems like FinalizeCallback needs to create a HandleScope
since it calls ObjectWrap::~ObjectWrap() which might need
to create a temporary Object handle here
https://github.com/nodejs/node-addon-api/blob/master/napi-inl.h#L3558

at Realm (https://github.com/realm/realm-js) we have crashes
with stacktrace at this location.

While fixed in core for later versions we still need for
older versions of Node.js

PR-URL: nodejs/node-addon-api#832
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@KevinEady KevinEady KevinEady approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@NickNaso NickNaso NickNaso approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@blagoev @mhdawson @gabrielschulhof @NickNaso @KevinEady