src: fix a crashing issue in Error::ThrowAsJavaScriptException via NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS #975

KevinEady · 2021-04-22T19:14:32Z

When terminating an environment (e.g., by calling worker.terminate), napi_throw, which is called by Error::ThrowAsJavaScriptException, returns napi_pending_exception, which is then incorrectly treated as a fatal error resulting in a crash. In order to not terminate the Node process, define the preprocessor directive NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS.

When terminating an environment (e.g., by calling worker.terminate), napi_throw, which is called by Error::ThrowAsJavaScriptException, returns napi_pending_exception, which is then incorrectly treated as a fatal error resulting in a crash.

Introduce the compile option `NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS` to dictate fix for crashing behavior.

mhdawson · 2021-04-22T21:14:03Z

napi-inl.h

+    if (status == napi_pending_exception) {
+      // The environment could be terminating.
+      return;
+    }


I don't think this block should be added here.

mhdawson · 2021-04-22T21:14:36Z

common.gypi

@@ -15,6 +15,7 @@
      }
    }]
  ],
+  'defines': [ 'NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS' ],


This should not be here by default right ?

Hi @mhdawson ,

So this relates to my comment on #902 (comment) where:

Not having the define in binding.gyp and using #define prior to #include <napi.h>: I was not able to opt-in.

You can see in commit a089697 the change to move the setting from binding.gyp to #define and now all of the builds fail.

I do not think it is possible to have a per-compilation unit setting for this feature, because the Error::ThrowAsJavaScriptException is a compiled, exported symbol:

➜ test git:(swallow-unthrowable-exceptions) ✗ nm -C build/Release/binding_noexcept.node | grep ThrowAsJavaScriptException 00000000000024f0 t Napi::Error::ThrowAsJavaScriptException() const

So, when the first compilation unit uses it, it will be compiled with whatever preprocessor definitions are currently in scope.

- Move define to src - Correct fix implementation

KevinEady · 2021-04-27T17:16:57Z

Hi @mhdawson and team. This PR is ready for review. As discussed in last week's meeting, the last commit contains (1) the implementation change and (2) creation of new test target. Just struggling with the CI.

KevinEady · 2021-04-27T17:18:06Z

@legendecas I believe you are working on build system changes. Since this introduces new target, will possibly need to collaborate between the two PRs.

mhdawson · 2021-04-28T22:15:55Z

@KevinEady, if the build changes were to refactor the testing so that it is easier to add a new config that we run the tests under, unless I'm remembering wrong, I think I already landed that.

KevinEady · 2021-04-29T23:10:55Z

@KevinEady, if the build changes were to refactor the testing so that it is easier to add a new config that we run the tests under, unless I'm remembering wrong, I think I already landed that.

Hi @mhdawson , no the build changes were to create a new target that only compiles the single file with the NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS define.

mhdawson · 2021-04-30T19:34:11Z

@KevinEady got it.

mhdawson · 2021-06-14T15:25:29Z

@KevinEady I'm wondering if this is still waiting on the other PR mentioned or if its ready to go?

KevinEady · 2021-06-14T17:53:56Z

Hi @mhdawson ,

This PR is ready to go and indeed addresses the issue in #902. I am wondering though if there is any real-world code that we can see if this fix actually solves the problem, cc: @rudolftam ?

Thanks, Kevin

mhdawson

LGTM

mhdawson · 2021-06-14T22:39:44Z

I'll just hold off landing this until the 4.0 release goes out.

mhdawson · 2021-06-14T22:40:19Z

Actually thinking about it, I don't see the harm of it being in 4.x so landing now.

When terminating an environment (e.g., by calling worker.terminate), napi_throw, which is called by Error::ThrowAsJavaScriptException, returns napi_pending_exception, which is then incorrectly treated as a fatal error resulting in a crash. PR-URL: #975 Reviewed-By: Nicola Del Gobbo <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

mhdawson · 2021-06-14T22:49:07Z

Landed as 028107f

mhdawson · 2021-06-14T22:50:06Z

@KevinEady I've landed but I am wondering about testing with the additional define. Should I see additional output when I run npm test or is that all hidden behind the scenes and I should only see one line for each test?

KevinEady · 2021-06-15T13:48:39Z

Hi @mhdawson ,

There are two additional build targets (1, 2) and one test (1) that runs everything in the background via a separate node process:

node-addon-api/test/error_terminating_environment.js

Lines 47 to 64 in 3f291f7

    
           switch (index_for_test_case) { 
        
             case 0: 
        
               binding.error.throwJSError('test', true); 
        
               break; 
        
             case 1: 
        
               binding.error.throwTypeError('test', true); 
        
               break; 
        
             case 2: 
        
               binding.error.throwRangeError('test', true); 
        
               break; 
        
             case 3: 
        
               binding.error.throwDefaultError(false, true); 
        
               break; 
        
             case 4: 
        
               binding.error.throwDefaultError(true, true); 
        
               break; 
        
             default: assert.fail('Invalid index'); 
        
           }

ghost · 2021-06-19T06:16:44Z

@KevinEady re: I am wondering though if there is any real-world code that we can see if this fix actually solves the problem

It was in one of my unreleased hobby projects in which I ran into this issue. It uses Node.js as an embedded library to provide scripting support for the end-user. As the scripts must be terminable (mainly for the same reasons as in web browsers), everything around them must be capable of handling it without crashing.

I'm glad that this is now possible when using node-addon-api. I'm worried, though, that the opt-in nature of this fix will mean that most addons will needlessly keep crashing in the future as well.

while (true) {
	console.log(`I hope you're not planning on terminating me. :)`)
	
	do_something_with_addon_a_fix_enabled()
	do_something_with_addon_b_fix_disabled()
	do_something_with_addon_c_fix_disabled()
	do_something_with_addon_d_fix_disabled()
}

When terminating an environment (e.g., by calling worker.terminate), napi_throw, which is called by Error::ThrowAsJavaScriptException, returns napi_pending_exception, which is then incorrectly treated as a fatal error resulting in a crash. PR-URL: nodejs#975 Reviewed-By: Nicola Del Gobbo <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

When terminating an environment (e.g., by calling worker.terminate), napi_throw, which is called by Error::ThrowAsJavaScriptException, returns napi_pending_exception, which is then incorrectly treated as a fatal error resulting in a crash. PR-URL: nodejs/node-addon-api#975 Reviewed-By: Nicola Del Gobbo <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

rudolftam and others added 2 commits April 22, 2021 20:32

src,doc: add compile option

5382c4a

Introduce the compile option `NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS` to dictate fix for crashing behavior.

mhdawson reviewed Apr 22, 2021

View reviewed changes

KevinEady force-pushed the swallow-unthrowable-exceptions branch from cd7d47d to 3ca4de5 Compare April 23, 2021 08:44

Address review comments

a089697

- Move define to src - Correct fix implementation

KevinEady force-pushed the swallow-unthrowable-exceptions branch from 3ca4de5 to a089697 Compare April 23, 2021 08:46

KevinEady added 2 commits April 23, 2021 11:08

Move compile option back to gyp file

0187fec

test: Move test to separate target

3f291f7

KevinEady force-pushed the swallow-unthrowable-exceptions branch from 7ce3576 to 3f291f7 Compare April 27, 2021 16:28

KevinEady marked this pull request as ready for review April 27, 2021 17:16

legendecas approved these changes May 6, 2021

View reviewed changes

mhdawson mentioned this pull request Jun 14, 2021

src: fix a crashing issue in Error::ThrowAsJavaScriptException #902

Closed

mhdawson approved these changes Jun 14, 2021

View reviewed changes

mhdawson closed this Jun 14, 2021

deepak1556 mentioned this pull request Jun 22, 2021

Unhandled c++ exception in keytar native module microsoft/vscode#111288

Closed

KevinEady mentioned this pull request Aug 13, 2021

NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS only catch one throw #1027

Closed

deepak1556 mentioned this pull request Aug 31, 2021

Linux: nsfw watcher crashes when inotify limit reached microsoft/vscode#131957

Closed

kleisauke mentioned this pull request May 22, 2023

Is it possible to get (or compile) .pdb file for sharp.node for specfic sharp version? lovell/sharp#3569

Closed

baby636 mentioned this pull request Jun 5, 2023

[Snyk] Upgrade fs-extra from 9.1.0 to 11.1.1 baby636/node-addon-api#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: fix a crashing issue in Error::ThrowAsJavaScriptException via NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS #975

src: fix a crashing issue in Error::ThrowAsJavaScriptException via NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS #975

KevinEady commented Apr 22, 2021

mhdawson Apr 22, 2021

KevinEady Apr 23, 2021

mhdawson Apr 22, 2021

KevinEady Apr 23, 2021

KevinEady commented Apr 27, 2021

KevinEady commented Apr 27, 2021

mhdawson commented Apr 28, 2021

KevinEady commented Apr 29, 2021

mhdawson commented Apr 30, 2021

mhdawson commented Jun 14, 2021

KevinEady commented Jun 14, 2021

mhdawson left a comment

mhdawson commented Jun 14, 2021

mhdawson commented Jun 14, 2021

mhdawson commented Jun 14, 2021

mhdawson commented Jun 14, 2021

KevinEady commented Jun 15, 2021

ghost commented Jun 19, 2021

src: fix a crashing issue in Error::ThrowAsJavaScriptException via NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS #975

src: fix a crashing issue in Error::ThrowAsJavaScriptException via NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS #975

Conversation

KevinEady commented Apr 22, 2021

mhdawson Apr 22, 2021

Choose a reason for hiding this comment

KevinEady Apr 23, 2021

Choose a reason for hiding this comment

mhdawson Apr 22, 2021

Choose a reason for hiding this comment

KevinEady Apr 23, 2021

Choose a reason for hiding this comment

KevinEady commented Apr 27, 2021

KevinEady commented Apr 27, 2021

mhdawson commented Apr 28, 2021

KevinEady commented Apr 29, 2021

mhdawson commented Apr 30, 2021

mhdawson commented Jun 14, 2021

KevinEady commented Jun 14, 2021

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Jun 14, 2021

mhdawson commented Jun 14, 2021

mhdawson commented Jun 14, 2021

mhdawson commented Jun 14, 2021

KevinEady commented Jun 15, 2021

ghost commented Jun 19, 2021