GeezerVison is showing github.com — n-api: ensure in-module exceptions are propagated · nodejs/node@92f699e
File treeExpand file treeCollapse file tree Open diff view settings test/addons-napi/test_exception
Expand file treeCollapse file tree Open diff view settings Collapse file Copy file name to clipboardExpand all lines: src/node_api.cc +41-37Lines changed: 41 additions & 37 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -173,6 +173,23 @@ struct napi_env__ { | 173 | 173 | (out) = v8::type::New((buffer), (byte_offset), (length)); \ | 174 | 174 | } while (0) | 175 | 175 |
| | 176 | +#define NAPI_CALL_INTO_MODULE(env, call, handle_exception) \ | | 177 | + do { \ | | 178 | + int open_handle_scopes = (env)->open_handle_scopes; \ | | 179 | + int open_callback_scopes = (env)->open_callback_scopes; \ | | 180 | + napi_clear_last_error((env)); \ | | 181 | + call; \ | | 182 | + CHECK_EQ((env)->open_handle_scopes, open_handle_scopes); \ | | 183 | + CHECK_EQ((env)->open_callback_scopes, open_callback_scopes); \ | | 184 | + if (!(env)->last_exception.IsEmpty()) { \ | | 185 | + handle_exception( \ | | 186 | + v8::Local<v8::Value>::New((env)->isolate, (env)->last_exception)); \ | | 187 | + (env)->last_exception.Reset(); \ | | 188 | + } \ | | 189 | + } while (0) | | 190 | + | | 191 | +#define NAPI_CALL_INTO_MODULE_THROW(env, call) \ | | 192 | + NAPI_CALL_INTO_MODULE((env), call, (env)->isolate->ThrowException) | 176 | 193 |
| 177 | 194 | namespace { | 178 | 195 | namespace v8impl { | @@ -352,10 +369,11 @@ class Finalizer { | 352 | 369 | static void FinalizeBufferCallback(char* data, void* hint) { | 353 | 370 | Finalizer* finalizer = static_cast<Finalizer*>(hint); | 354 | 371 | if (finalizer->_finalize_callback != nullptr) { | 355 | | - finalizer->_finalize_callback( | 356 | | - finalizer->_env, | 357 | | - data, | 358 | | - finalizer->_finalize_hint); | | 372 | + NAPI_CALL_INTO_MODULE_THROW(finalizer->_env, | | 373 | + finalizer->_finalize_callback( | | 374 | + finalizer->_env, | | 375 | + data, | | 376 | + finalizer->_finalize_hint)); | 359 | 377 | } | 360 | 378 |
| 361 | 379 | Delete(finalizer); | @@ -467,10 +485,11 @@ class Reference : private Finalizer { | 467 | 485 | bool delete_self = reference->_delete_self; | 468 | 486 |
| 469 | 487 | if (reference->_finalize_callback != nullptr) { | 470 | | - reference->_finalize_callback( | 471 | | - reference->_env, | 472 | | - reference->_finalize_data, | 473 | | - reference->_finalize_hint); | | 488 | + NAPI_CALL_INTO_MODULE_THROW(reference->_env, | | 489 | + reference->_finalize_callback( | | 490 | + reference->_env, | | 491 | + reference->_finalize_data, | | 492 | + reference->_finalize_hint)); | 474 | 493 | } | 475 | 494 |
| 476 | 495 | if (delete_self) { | @@ -555,32 +574,17 @@ class CallbackWrapperBase : public CallbackWrapper { | 555 | 574 | napi_callback cb = reinterpret_cast<napi_callback>( | 556 | 575 | v8::Local<v8::External>::Cast( | 557 | 576 | _cbdata->GetInternalField(kInternalFieldIndex))->Value()); | 558 | | - v8::Isolate* isolate = _cbinfo.GetIsolate(); | 559 | 577 |
| 560 | 578 | napi_env env = static_cast<napi_env>( | 561 | 579 | v8::Local<v8::External>::Cast( | 562 | 580 | _cbdata->GetInternalField(kEnvIndex))->Value()); | 563 | 581 |
| 564 | | - // Make sure any errors encountered last time we were in N-API are gone. | 565 | | - napi_clear_last_error(env); | 566 | | - | 567 | | - int open_handle_scopes = env->open_handle_scopes; | 568 | | - int open_callback_scopes = env->open_callback_scopes; | 569 | | - | 570 | | - napi_value result = cb(env, cbinfo_wrapper); | | 582 | + napi_value result; | | 583 | + NAPI_CALL_INTO_MODULE_THROW(env, result = cb(env, cbinfo_wrapper)); | 571 | 584 |
| 572 | 585 | if (result != nullptr) { | 573 | 586 | this->SetReturnValue(result); | 574 | 587 | } | 575 | | - | 576 | | - CHECK_EQ(env->open_handle_scopes, open_handle_scopes); | 577 | | - CHECK_EQ(env->open_callback_scopes, open_callback_scopes); | 578 | | - | 579 | | - if (!env->last_exception.IsEmpty()) { | 580 | | - isolate->ThrowException( | 581 | | - v8::Local<v8::Value>::New(isolate, env->last_exception)); | 582 | | - env->last_exception.Reset(); | 583 | | - } | 584 | 588 | } | 585 | 589 |
| 586 | 590 | const Info& _cbinfo; | @@ -888,8 +892,10 @@ void napi_module_register_cb(v8::Local<v8::Object> exports, | 888 | 892 | // one is found. | 889 | 893 | napi_env env = v8impl::GetEnv(context); | 890 | 894 |
| 891 | | - napi_value _exports = | 892 | | - mod->nm_register_func(env, v8impl::JsValueFromV8LocalValue(exports)); | | 895 | + napi_value _exports; | | 896 | + NAPI_CALL_INTO_MODULE_THROW(env, | | 897 | + _exports = mod->nm_register_func(env, | | 898 | + v8impl::JsValueFromV8LocalValue(exports))); | 893 | 899 |
| 894 | 900 | // If register function returned a non-null exports object different from | 895 | 901 | // the exports object we passed it, set that as the "exports" property of | @@ -3384,19 +3390,17 @@ class Work : public node::AsyncResource { | 3384 | 3390 | v8::HandleScope scope(env->isolate); | 3385 | 3391 | CallbackScope callback_scope(work); | 3386 | 3392 |
| 3387 | | - work->_complete(env, ConvertUVErrorCode(status), work->_data); | | 3393 | + NAPI_CALL_INTO_MODULE(env, | | 3394 | + work->_complete(env, ConvertUVErrorCode(status), work->_data), | | 3395 | + [env] (v8::Local<v8::Value> local_err) { | | 3396 | + // If there was an unhandled exception in the complete callback, | | 3397 | + // report it as a fatal exception. (There is no JavaScript on the | | 3398 | + // callstack that can possibly handle it.) | | 3399 | + v8impl::trigger_fatal_exception(env, local_err); | | 3400 | + }); | 3388 | 3401 |
| 3389 | 3402 | // Note: Don't access `work` after this point because it was | 3390 | 3403 | // likely deleted by the complete callback. | 3391 | | - | 3392 | | - // If there was an unhandled exception in the complete callback, | 3393 | | - // report it as a fatal exception. (There is no JavaScript on the | 3394 | | - // callstack that can possibly handle it.) | 3395 | | - if (!env->last_exception.IsEmpty()) { | 3396 | | - v8::Local<v8::Value> local_err = v8::Local<v8::Value>::New( | 3397 | | - env->isolate, env->last_exception); | 3398 | | - v8impl::trigger_fatal_exception(env, local_err); | 3399 | | - } | 3400 | 3404 | } | 3401 | 3405 | } | 3402 | 3406 |
| |
Collapse file Copy file name to clipboardExpand all lines: test/addons-napi/test_exception/test.js +29-1Lines changed: 29 additions & 1 deletion | Original file line number | Diff line number | Diff line change |
|---|
| 1 | 1 | 'use strict'; | | 2 | +// Flags: --expose-gc | 2 | 3 |
| 3 | 4 | const common = require('../../common'); | 4 | | -const test_exception = require(`./build/${common.buildType}/test_exception`); | 5 | 5 | const assert = require('assert'); | 6 | 6 | const theError = new Error('Some error'); | 7 | 7 |
| | 8 | +// The test module throws an error during Init, but in order for its exports to | | 9 | +// not be lost, it attaches them to the error's "bindings" property. This way, | | 10 | +// we can make sure that exceptions thrown during the module initialization | | 11 | +// phase are propagated through require() into JavaScript. | | 12 | +// https://github.com/nodejs/node/issues/19437 | | 13 | +const test_exception = (function() { | | 14 | + let resultingException; | | 15 | + try { | | 16 | + require(`./build/${common.buildType}/test_exception`); | | 17 | + } catch (anException) { | | 18 | + resultingException = anException; | | 19 | + } | | 20 | + assert.strictEqual(resultingException.message, 'Error during Init'); | | 21 | + return resultingException.binding; | | 22 | +})(); | | 23 | + | 8 | 24 | { | 9 | 25 | const throwTheError = () => { throw theError; }; | 10 | 26 |
| @@ -50,3 +66,15 @@ const theError = new Error('Some error'); | 50 | 66 | 'Exception state did not remain clear as expected,' + | 51 | 67 | ` .wasPending() returned ${exception_pending}`); | 52 | 68 | } | | 69 | + | | 70 | +// Make sure that exceptions that occur during finalization are propagated. | | 71 | +function testFinalize(binding) { | | 72 | + let x = test_exception[binding](); | | 73 | + x = null; | | 74 | + assert.throws(() => { global.gc(); }, /Error during Finalize/); | | 75 | + | | 76 | + // To assuage the linter's concerns. | | 77 | + (function() {})(x); | | 78 | +} | | 79 | +testFinalize('createExternal'); | | 80 | +testFinalize('createExternalBuffer'); |
Collapse file Copy file name to clipboardExpand all lines: test/addons-napi/test_exception/test_exception.c +37-5Lines changed: 37 additions & 5 deletions | Original file line number | Diff line number | Diff line change |
|---|
| 3 | 3 |
| 4 | 4 | static bool exceptionWasPending = false; | 5 | 5 |
| 6 | | -napi_value returnException(napi_env env, napi_callback_info info) { | | 6 | +static napi_value returnException(napi_env env, napi_callback_info info) { | 7 | 7 | size_t argc = 1; | 8 | 8 | napi_value args[1]; | 9 | 9 | NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL)); | @@ -22,7 +22,7 @@ napi_value returnException(napi_env env, napi_callback_info info) { | 22 | 22 | return NULL; | 23 | 23 | } | 24 | 24 |
| 25 | | -napi_value allowException(napi_env env, napi_callback_info info) { | | 25 | +static napi_value allowException(napi_env env, napi_callback_info info) { | 26 | 26 | size_t argc = 1; | 27 | 27 | napi_value args[1]; | 28 | 28 | NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL)); | @@ -38,23 +38,55 @@ napi_value allowException(napi_env env, napi_callback_info info) { | 38 | 38 | return NULL; | 39 | 39 | } | 40 | 40 |
| 41 | | -napi_value wasPending(napi_env env, napi_callback_info info) { | | 41 | +static napi_value wasPending(napi_env env, napi_callback_info info) { | 42 | 42 | napi_value result; | 43 | 43 | NAPI_CALL(env, napi_get_boolean(env, exceptionWasPending, &result)); | 44 | 44 |
| 45 | 45 | return result; | 46 | 46 | } | 47 | 47 |
| 48 | | -napi_value Init(napi_env env, napi_value exports) { | | 48 | +static void finalizer(napi_env env, void *data, void *hint) { | | 49 | + NAPI_CALL_RETURN_VOID(env, | | 50 | + napi_throw_error(env, NULL, "Error during Finalize")); | | 51 | +} | | 52 | + | | 53 | +static napi_value createExternal(napi_env env, napi_callback_info info) { | | 54 | + napi_value external; | | 55 | + | | 56 | + NAPI_CALL(env, | | 57 | + napi_create_external(env, NULL, finalizer, NULL, &external)); | | 58 | + | | 59 | + return external; | | 60 | +} | | 61 | + | | 62 | +static char buffer_data[12]; | | 63 | + | | 64 | +static napi_value createExternalBuffer(napi_env env, napi_callback_info info) { | | 65 | + napi_value buffer; | | 66 | + NAPI_CALL(env, napi_create_external_buffer(env, sizeof(buffer_data), | | 67 | + buffer_data, finalizer, NULL, &buffer)); | | 68 | + return buffer; | | 69 | +} | | 70 | + | | 71 | +static napi_value Init(napi_env env, napi_value exports) { | 49 | 72 | napi_property_descriptor descriptors[] = { | 50 | 73 | DECLARE_NAPI_PROPERTY("returnException", returnException), | 51 | 74 | DECLARE_NAPI_PROPERTY("allowException", allowException), | 52 | 75 | DECLARE_NAPI_PROPERTY("wasPending", wasPending), | | 76 | + DECLARE_NAPI_PROPERTY("createExternal", createExternal), | | 77 | + DECLARE_NAPI_PROPERTY("createExternalBuffer", createExternalBuffer), | 53 | 78 | }; | 54 | | - | 55 | 79 | NAPI_CALL(env, napi_define_properties( | 56 | 80 | env, exports, sizeof(descriptors) / sizeof(*descriptors), descriptors)); | 57 | 81 |
| | 82 | + napi_value error, code, message; | | 83 | + NAPI_CALL(env, napi_create_string_utf8(env, "Error during Init", | | 84 | + NAPI_AUTO_LENGTH, &message)); | | 85 | + NAPI_CALL(env, napi_create_string_utf8(env, "", NAPI_AUTO_LENGTH, &code)); | | 86 | + NAPI_CALL(env, napi_create_error(env, code, message, &error)); | | 87 | + NAPI_CALL(env, napi_set_named_property(env, error, "binding", exports)); | | 88 | + NAPI_CALL(env, napi_throw(env, error)); | | 89 | + | 58 | 90 | return exports; | 59 | 91 | } | 60 | 92 |
| |
|