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

n-api: ensure in-module exceptions are propagated #19537

Closed
wants to merge 1 commit into from
Closed

n-api: ensure in-module exceptions are propagated #19537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
78 changes: 41 additions & 37 deletions src/node_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,23 @@ struct napi_env__ {
(out) = v8::type::New((buffer), (byte_offset), (length)); \
} while (0)

#define NAPI_CALL_INTO_MODULE(env, call, handle_exception) \
do { \
int open_handle_scopes = (env)->open_handle_scopes; \
int open_callback_scopes = (env)->open_callback_scopes; \
napi_clear_last_error((env)); \
call; \
CHECK_EQ((env)->open_handle_scopes, open_handle_scopes); \
CHECK_EQ((env)->open_callback_scopes, open_callback_scopes); \
if (!(env)->last_exception.IsEmpty()) { \
handle_exception( \
v8::Local<v8::Value>::New((env)->isolate, (env)->last_exception)); \
(env)->last_exception.Reset(); \
} \
} while (0)

#define NAPI_CALL_INTO_MODULE_THROW(env, call) \
NAPI_CALL_INTO_MODULE((env), call, (env)->isolate->ThrowException)

namespace {
namespace v8impl {
Expand Down Expand Up @@ -336,10 +353,11 @@ class Finalizer {
static void FinalizeBufferCallback(char* data, void* hint) {
Finalizer* finalizer = static_cast<Finalizer*>(hint);
if (finalizer->_finalize_callback != nullptr) {
finalizer->_finalize_callback(
finalizer->_env,
data,
finalizer->_finalize_hint);
NAPI_CALL_INTO_MODULE_THROW(finalizer->_env,
finalizer->_finalize_callback(
finalizer->_env,
data,
finalizer->_finalize_hint));
}

Delete(finalizer);
Expand Down Expand Up @@ -441,10 +459,11 @@ class Reference : private Finalizer {
bool delete_self = reference->_delete_self;

if (reference->_finalize_callback != nullptr) {
reference->_finalize_callback(
reference->_env,
reference->_finalize_data,
reference->_finalize_hint);
NAPI_CALL_INTO_MODULE_THROW(reference->_env,
reference->_finalize_callback(
reference->_env,
reference->_finalize_data,
reference->_finalize_hint));
}

if (delete_self) {
Expand Down Expand Up @@ -529,32 +548,17 @@ class CallbackWrapperBase : public CallbackWrapper {
napi_callback cb = reinterpret_cast<napi_callback>(
v8::Local<v8::External>::Cast(
_cbdata->GetInternalField(kInternalFieldIndex))->Value());
v8::Isolate* isolate = _cbinfo.GetIsolate();

napi_env env = static_cast<napi_env>(
v8::Local<v8::External>::Cast(
_cbdata->GetInternalField(kEnvIndex))->Value());

// Make sure any errors encountered last time we were in N-API are gone.
napi_clear_last_error(env);

int open_handle_scopes = env->open_handle_scopes;
int open_callback_scopes = env->open_callback_scopes;

napi_value result = cb(env, cbinfo_wrapper);
napi_value result;
NAPI_CALL_INTO_MODULE_THROW(env, result = cb(env, cbinfo_wrapper));

if (result != nullptr) {
this->SetReturnValue(result);
}

CHECK_EQ(env->open_handle_scopes, open_handle_scopes);
CHECK_EQ(env->open_callback_scopes, open_callback_scopes);

if (!env->last_exception.IsEmpty()) {
isolate->ThrowException(
v8::Local<v8::Value>::New(isolate, env->last_exception));
env->last_exception.Reset();
}
}

const Info& _cbinfo;
Expand Down Expand Up @@ -861,8 +865,10 @@ void napi_module_register_cb(v8::Local<v8::Object> exports,
// one is found.
napi_env env = v8impl::GetEnv(context);

napi_value _exports =
mod->nm_register_func(env, v8impl::JsValueFromV8LocalValue(exports));
napi_value _exports;
NAPI_CALL_INTO_MODULE_THROW(env,
_exports = mod->nm_register_func(env,
v8impl::JsValueFromV8LocalValue(exports)));

// If register function returned a non-null exports object different from
// the exports object we passed it, set that as the "exports" property of
Expand Down Expand Up @@ -3357,19 +3363,17 @@ class Work : public node::AsyncResource {
v8::HandleScope scope(env->isolate);
CallbackScope callback_scope(work);

work->_complete(env, ConvertUVErrorCode(status), work->_data);
NAPI_CALL_INTO_MODULE(env,
work->_complete(env, ConvertUVErrorCode(status), work->_data),
[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);
});

// Note: Don't access `work` after this point because it was
// likely deleted by the complete callback.

// 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.)
if (!env->last_exception.IsEmpty()) {
v8::Local<v8::Value> local_err = v8::Local<v8::Value>::New(
env->isolate, env->last_exception);
v8impl::trigger_fatal_exception(env, local_err);
}
}
}

Expand Down
30 changes: 29 additions & 1 deletion test/addons-napi/test_exception/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,26 @@
'use strict';
// Flags: --expose-gc

const common = require('../../common');
const test_exception = require(`./build/${common.buildType}/test_exception`);
const assert = require('assert');
const theError = new Error('Some error');

// The test module throws an error during Init, but in order for its exports to
// not be lost, it attaches them to the error's "bindings" property. This way,
// we can make sure that exceptions thrown during the module initialization
// phase are propagated through require() into JavaScript.
// https://github.com/nodejs/node/issues/19437
const test_exception = (function() {
let resultingException;
try {
require(`./build/${common.buildType}/test_exception`);
} catch (anException) {
resultingException = anException;
}
assert.strictEqual(resultingException.message, 'Error during Init');
return resultingException.binding;
})();

{
const throwTheError = () => { throw theError; };

Expand Down Expand Up @@ -50,3 +66,15 @@ const theError = new Error('Some error');
'Exception state did not remain clear as expected,' +
` .wasPending() returned ${exception_pending}`);
}

// Make sure that exceptions that occur during finalization are propagated.
function testFinalize(binding) {
let x = test_exception[binding]();
x = null;
assert.throws(() => { global.gc(); }, /Error during Finalize/);
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be worth running this a number of times with the stress job to make sure its not flaky as there is no guarantee objects will be collected,


// To assuage the linter's concerns.
(function() {})(x);
}
testFinalize('createExternal');
testFinalize('createExternalBuffer');
42 changes: 37 additions & 5 deletions test/addons-napi/test_exception/test_exception.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

static bool exceptionWasPending = false;

napi_value returnException(napi_env env, napi_callback_info info) {
static napi_value returnException(napi_env env, napi_callback_info info) {
size_t argc = 1;
napi_value args[1];
NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
Expand All @@ -22,7 +22,7 @@ napi_value returnException(napi_env env, napi_callback_info info) {
return NULL;
}

napi_value allowException(napi_env env, napi_callback_info info) {
static napi_value allowException(napi_env env, napi_callback_info info) {
size_t argc = 1;
napi_value args[1];
NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
Expand All @@ -38,23 +38,55 @@ napi_value allowException(napi_env env, napi_callback_info info) {
return NULL;
}

napi_value wasPending(napi_env env, napi_callback_info info) {
static napi_value wasPending(napi_env env, napi_callback_info info) {
napi_value result;
NAPI_CALL(env, napi_get_boolean(env, exceptionWasPending, &result));

return result;
}

napi_value Init(napi_env env, napi_value exports) {
static void finalizer(napi_env env, void *data, void *hint) {
NAPI_CALL_RETURN_VOID(env,
napi_throw_error(env, NULL, "Error during Finalize"));
}

static napi_value createExternal(napi_env env, napi_callback_info info) {
napi_value external;

NAPI_CALL(env,
napi_create_external(env, NULL, finalizer, NULL, &external));

return external;
}

static char buffer_data[12];

static napi_value createExternalBuffer(napi_env env, napi_callback_info info) {
napi_value buffer;
NAPI_CALL(env, napi_create_external_buffer(env, sizeof(buffer_data),
buffer_data, finalizer, NULL, &buffer));
return buffer;
}

static napi_value Init(napi_env env, napi_value exports) {
napi_property_descriptor descriptors[] = {
DECLARE_NAPI_PROPERTY("returnException", returnException),
DECLARE_NAPI_PROPERTY("allowException", allowException),
DECLARE_NAPI_PROPERTY("wasPending", wasPending),
DECLARE_NAPI_PROPERTY("createExternal", createExternal),
DECLARE_NAPI_PROPERTY("createExternalBuffer", createExternalBuffer),
};

NAPI_CALL(env, napi_define_properties(
env, exports, sizeof(descriptors) / sizeof(*descriptors), descriptors));

napi_value error, code, message;
NAPI_CALL(env, napi_create_string_utf8(env, "Error during Init",
NAPI_AUTO_LENGTH, &message));
NAPI_CALL(env, napi_create_string_utf8(env, "", NAPI_AUTO_LENGTH, &code));
NAPI_CALL(env, napi_create_error(env, code, message, &error));
NAPI_CALL(env, napi_set_named_property(env, error, "binding", exports));
NAPI_CALL(env, napi_throw(env, error));

return exports;
}

Expand Down