create a HandleScope in FinalizeCallback - #832
Conversation
Sorry, something went wrong.
|
seems like clang-format fails on the whole napi-inl.h file and is not related to the changes in this PR |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@mhdawson nevermind. This was fixed in core (nodejs/node#34366) in RunAndClearNativeImmediates, which is part of @KevinEady's stack. |
Sorry, something went wrong.
gabrielschulhof
left a comment
There was a problem hiding this comment.
We need this for older versions of Node.js 👍
Sorry, something went wrong.
|
yes I can confirm the crash was happening when closing the application which might be where ~napi_env is called. |
Sorry, something went wrong.
mhdawson
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
NickNaso
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
KevinEady
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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>
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>
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>
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>
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>
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.