n-api: ensure scope present for finalization by mhdawson · Pull Request #33508 · nodejs/node · GitHub

Spent some time looking at nodejs/node-addon-api#729 which was related to a crash during finalization when using node-addon-api ObjectWrap.

I think it should be fixed in core as opposed to node-addon-api as it should be possible to hit the same problem using N-API directly instead of node-addon-api.

It seems to be more subtle than the scope never being in place. The added is the simplest test that causes the problem to recreate when run under debug and without the change adding the scope.

Refs: nodejs/node-addon-api#722

Ensure a scope is on stack during finalization
as finalization functions can create JS Objects

Signed-off-by: Michael Dawson michael_dawson@ca.ibm.com

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

@addaleax, @gabrielschulhof hoping you can confirm if you think this adds the scope in the right place. While investigating I checked that the status of the env when the assert is hit indicates that we can still call back to JS, which I'm assuming confirms it's ok to have a Scope/allocation.

I could see moving the scope out of the loop to reduce overhead, but left it in the loop as that seems safer in terms of memory use.

We could create a version of CallIntoModule and CallIntoModuleThrow that declares these scopes, because currently invocations of CallIntoModule and CallIntoModuleThrow are almost always preceded by scope declarations:

FinalizeBufferCallback precedes the call into the addon with a HandleScope and a ContextScope. @addaleax can we replace the ContextScope with a CallbackScope for consistency with other calls into the addon?

node/src/node_api.cc

Lines 57 to 65 in 9949a2e

    
           v8::HandleScope handle_scope(finalizer->_env->isolate); 
        
           v8::Context::Scope context_scope(finalizer->_env->context()); 
        
           finalizer->_env->CallIntoModuleThrow([&](napi_env env) { 
        
             finalizer->_finalize_callback( 
        
                 env, 
        
                 finalizer->_finalize_data, 
        
                 finalizer->_finalize_hint); 
        
           });

In TSFN's DispatchOne we already precede the invocation of CallIntoModule with a HandleScope and a CallbackScope:

node/src/node_api.cc

Lines 303 to 313 in 9949a2e

    
           v8::HandleScope scope(env->isolate); 
        
           CallbackScope cb_scope(this); 
        
           napi_value js_callback = nullptr; 
        
           if (!ref.IsEmpty()) { 
        
             v8::Local<v8::Function> js_cb = 
        
               v8::Local<v8::Function>::New(env->isolate, ref); 
        
             js_callback = v8impl::JsValueFromV8LocalValue(js_cb); 
        
           } 
        
           env->CallIntoModuleThrow([&](napi_env env) { 
        
             call_js_cb(env, js_callback, context, data); 
        
           });

The same is true in TSFN's Finalize:

node/src/node_api.cc

Lines 318 to 324 in 9949a2e

    
           v8::HandleScope scope(env->isolate); 
        
           if (finalize_cb) { 
        
             CallbackScope cb_scope(this); 
        
             env->CallIntoModuleThrow([&](napi_env env) { 
        
               finalize_cb(env, finalize_data, context); 
        
             }); 
        
           }

In napi_module_register_by_symbol we definitely don't need any scopes because we know the call is coming from JS:

node/src/node_api.cc

Lines 458 to 460 in 9949a2e

    
           env->CallIntoModuleThrow([&](napi_env env) { 
        
             _exports = init(env, v8impl::JsValueFromV8LocalValue(exports)); 
        
           });

In AfterThreadPoolWork we already precede the invocation with a declaration of HandleScope and CallbackScope.

node/src/node_api.cc

Lines 858 to 871 in 9949a2e

    
           // Establish a handle scope here so that every callback doesn't have to. 
        
           // Also it is needed for the exception-handling below. 
        
           v8::HandleScope scope(_env->isolate); 
        
           CallbackScope callback_scope(this); 
        
           _env->CallIntoModule([&](napi_env env) { 
        
             _complete(env, ConvertUVErrorCode(status), _data); 
        
           }, [](napi_env env, v8::Local<v8::Value> local_err) { 
        
             // If there was an unhandled exception in the complete callback, 
        
             // report it as a fatal exception. (There is no JavaScript on the 
        
             // callstack that can possibly handle it.) 
        
             v8impl::trigger_fatal_exception(env, local_err); 
        
           });

This is the one you want to change. Would it be expensive if we declared both a HandleScope and a CallbackScope, thereby making it consistent with the other invocations where we need both?

node/src/js_native_api_v8.cc

Lines 270 to 276 in 9949a2e

    
             _env->CallIntoModuleThrow([&](napi_env env) { 
        
               _finalize_callback( 
        
                   env, 
        
                   _finalize_data, 
        
                   _finalize_hint); 
        
             }); 
        
           }

In InvokeCallback we definitely don't need any scopes because we know the call is coming from JS:

node/src/js_native_api_v8.cc

Lines 478 to 480 in 9949a2e

    
           env->CallIntoModuleThrow([&](napi_env env) { 
        
             result = cb(env, cbinfo_wrapper); 
        
           });

So, if we can agree in the two places that we can replace with/add a CallbackScope before the invocation, then we can add a method to napi_env called CallIntoModuleWithScopes that declares them and use that method everywhere.

@gabrielschulhof Just to be explicit here, CallbackScope is for asynchronous operations – Finalizers could definitely fall under that, yes.

@mhdawson with #33570 in place it should be a lot easier to write the variant that declares scopes since we won't have as many variations on napi_env::CallIntoModule.

@gabrielschulhof once #33570 lands, I'll update this PR to add CallIntoModuleWithScopes and use it in all places except for napi_module_register_by_symbol and validate that the new test fails without the scopes/passes with them.

@mhdawson it must also not be used in InvokeCallback because control is coming from JS there (last one on the list above).

Codecov Report

Merging #33508 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #33508      +/-   ##
==========================================
- Coverage   96.77%   96.77%   -0.01%     
==========================================
  Files         202      201       -1     
  Lines       67129    66642     -487     
==========================================
- Hits        64966    64492     -474     
+ Misses       2163     2150      -13

Impacted Files	Coverage Δ
lib/internal/modules/esm/loader.js	88.65% <0.00%> (-0.99%)	⬇️
lib/internal/fs/streams.js	94.71% <0.00%> (-0.46%)	⬇️
lib/internal/streams/destroy.js	98.91% <0.00%> (-0.35%)	⬇️
lib/internal/util/inspect.js	96.15% <0.00%> (-0.10%)	⬇️
lib/_stream_writable.js	99.24% <0.00%> (-0.03%)	⬇️
lib/_stream_readable.js	98.57% <0.00%> (-0.02%)	⬇️
lib/internal/event_target.js
lib/_http_agent.js	99.11% <0.00%> (+<0.01%)	⬆️
lib/internal/errors.js	98.37% <0.00%> (+<0.01%)	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a1df3b...c5f3e91. Read the comment docs.

Refs: nodejs/node-addon-api#722 Ensure a scope is on stack during finalization as finalization functions can create JS Objects Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

@gabrielschulhof I spent some time looking at a common CallIntoModuleWithScopes, however, that required passing in the AsyncResource which is not available at the place I'm trying to fix and due to need for a HandleScope outside of the callInfoModule or not having a CallbackScope only 2 of the other CallIntoModule locations would have used the new method.

I think based only something like only 25% of the paths being able to use it, its not worth adding the new function and simply added the HandleScope to the path from which it was missing.

Think this is now ready to go. Ran the new test with node_g and still passed after the latest change so should be good to go.

LGTM

I nevertheless think we can streamline our use of scopes, but that's beyond the scope purview of this PR 🙂

Resume build: https://ci.nodejs.org/job/node-test-pull-request/31772/

CI: https://ci.nodejs.org/job/node-test-pull-request/31778/

CI: https://ci.nodejs.org/job/node-test-pull-request/31804/

Resume build: https://ci.nodejs.org/job/node-test-pull-request/31804/

CI: https://ci.nodejs.org/job/node-test-pull-request/31807/

New build since that seems to be what's needed: https://ci.nodejs.org/job/node-test-pull-request/31807/

Clean CI run landing.

The last build was yellow. I think this can land.

Landed in 362e4a1

Refs: nodejs/node-addon-api#722 Ensure a scope is on stack during finalization as finalization functions can create JS Objects Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com> PR-URL: #33508 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: nodejs/node-addon-api#722 Ensure a scope is on stack during finalization as finalization functions can create JS Objects Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com> PR-URL: nodejs#33508 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: nodejs/node-addon-api#722 Ensure a scope is on stack during finalization as finalization functions can create JS Objects Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com> PR-URL: #33508 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: nodejs/node-addon-api#722 Ensure a scope is on stack during finalization as finalization functions can create JS Objects Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com> PR-URL: nodejs#33508 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: nodejs/node-addon-api#722 Ensure a scope is on stack during finalization as finalization functions can create JS Objects Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com> PR-URL: #33508 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 22, 2020

mhdawson changed the title napi: ensure scope present for finalization WIP: napi: ensure scope present for finalization May 22, 2020

mhdawson marked this pull request as draft May 22, 2020 02:09

mhdawson mentioned this pull request May 22, 2020

Fix ObjectWrap destructor from env exit under node_g nodejs/node-addon-api#729

Closed

mhdawson force-pushed the finalizer-scope branch 2 times, most recently from 9642cc5 to 6a62c2a Compare May 25, 2020 22:16

mhdawson changed the title WIP: napi: ensure scope present for finalization n-api: ensure scope present for finalization May 25, 2020

mhdawson marked this pull request as ready for review May 25, 2020 22:19

mhdawson requested review from addaleax and gabrielschulhof May 25, 2020 22:19

gabrielschulhof closed this May 26, 2020

gabrielschulhof reopened this May 26, 2020

legendecas added the node-api Issues and PRs related to the Node-API. label May 26, 2020

gabrielschulhof mentioned this pull request May 26, 2020

n-api: remove napi_env::CallIntoModuleThrow #33570

Closed

2 tasks

legendecas mentioned this pull request May 27, 2020

test: run in both a child process and a worker thread nodejs/node-addon-api#738

Closed

nick-erm mentioned this pull request May 27, 2020

Question: What about for support WorkingThreads on WRTC addon? node-webrtc/node-webrtc#623

Open

gabrielschulhof mentioned this pull request May 28, 2020

n-api segfault in v8impl::Reference::FinalizeCallback #33283

Closed

n-api: ensure scope present for finalization

c5f3e91

Refs: nodejs/node-addon-api#722 Ensure a scope is on stack during finalization as finalization functions can create JS Objects Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

mhdawson force-pushed the finalizer-scope branch from 6a62c2a to c5f3e91 Compare May 29, 2020 01:42

gabrielschulhof approved these changes May 29, 2020

View reviewed changes

github-actions Bot mentioned this pull request Jun 9, 2020

CI Relability 2020-06-09 JaneaSystems/nodejs-reliability#9

Open

9 tasks

mhdawson added v12.x labels Jun 9, 2020

mhdawson closed this Jun 9, 2020

gabrielschulhof mentioned this pull request Jun 10, 2020

[v14.x] Backport 33570 and 33508 to v14.x #33817

Closed

3 tasks

github-actions Bot mentioned this pull request Jun 10, 2020

CI Relability 2020-06-10 JaneaSystems/nodejs-reliability#10

Open

13 tasks

gabrielschulhof mentioned this pull request Jun 10, 2020

[v12.x] Backport 33570 and 33508 to v12.x #33819

Closed

3 tasks

codebytere mentioned this pull request Jun 28, 2020

v14.5.0 proposal #34093

Merged

codebytere mentioned this pull request Jul 13, 2020

v12.18.3 proposal #34343

Merged

mhdawson deleted the finalizer-scope branch September 14, 2020 21:19

mischnic mentioned this pull request Nov 14, 2020

[v2] FATAL ERROR: v8::HandleScope::CreateHandle() parcel-bundler/parcel#5245

Closed

calebmshafer mentioned this pull request Dec 15, 2020

Update to Electron 11 to pick up Node fixes iTwin/itwinjs-core#436

Merged

n-api: ensure scope present for finalization by mhdawson · Pull Request #33508 · nodejs/node · GitHub

Uh oh!

Uh oh!

n-api: ensure scope present for finalization - #33508

n-api: ensure scope present for finalization#33508mhdawson wants to merge 2 commits intonodejs:masternodejs/node:masterfrom mhdawson:finalizer-scopemhdawson/io.js:finalizer-scopeCopy head branch name to clipboard

Conversation

mhdawson commented May 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mhdawson commented May 25, 2020

Uh oh!

gabrielschulhof commented May 26, 2020

Uh oh!

gabrielschulhof commented May 26, 2020

Uh oh!

addaleax commented May 26, 2020

Uh oh!

gabrielschulhof commented May 26, 2020

Uh oh!

mhdawson commented May 26, 2020

Uh oh!

gabrielschulhof commented May 26, 2020

Uh oh!

codecov-commenter commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mhdawson commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented May 29, 2020

Uh oh!

gabrielschulhof left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jun 8, 2020

Uh oh!

nodejs-github-bot commented Jun 8, 2020

Uh oh!

nodejs-github-bot commented Jun 9, 2020

Uh oh!

mhdawson commented Jun 9, 2020

Uh oh!

nodejs-github-bot commented Jun 9, 2020

Uh oh!

mhdawson commented Jun 9, 2020

Uh oh!

mhdawson commented Jun 9, 2020

Uh oh!

gabrielschulhof commented Jun 9, 2020

Uh oh!

mhdawson commented Jun 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

n-api: ensure scope present for finalization#33508
mhdawson wants to merge 2 commits into
nodejs:masterfrom
mhdawson:finalizer-scope

mhdawson commented May 22, 2020 •
edited
Loading

codecov-commenter commented May 27, 2020 •
edited
Loading

mhdawson commented May 29, 2020 •
edited
Loading