Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit f156f87

Browse files
legendecaslegendecas
committedJan 14, 2021
n-api: emit uncaught-exception on unhandled tsfn callbacks
1 parent 2ba8728 commit f156f87

File tree

9 files changed

+211
-35
lines changed

9 files changed

+211
-35
lines changed
 

‎src/js_native_api_v8.h

+18-3
Original file line numberDiff line numberDiff line change
@@ -101,11 +101,26 @@ struct napi_env__ {
101101
}
102102
}
103103

104+
template <typename T>
105+
inline void CallbackIntoModule(T&& call) {
106+
CallIntoModule(call, [](napi_env env, v8::Local<v8::Value> local_err) {
107+
// If there was an unhandled exception in the complete callback,
108+
// report it as a fatal exception. (There is no JavaScript on the
109+
// callstack that can possibly handle it.)
110+
v8impl::trigger_fatal_exception(env, local_err);
111+
});
112+
}
113+
104114
virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) {
105115
v8::HandleScope handle_scope(isolate);
106-
CallIntoModule([&](napi_env env) {
107-
cb(env, data, hint);
108-
});
116+
CallIntoModule([&](napi_env env) { cb(env, data, hint); },
117+
[](napi_env env, v8::Local<v8::Value> local_err) {
118+
// If there was an unhandled exception in the complete
119+
// callback, report it as a fatal exception. (There is no
120+
// JavaScript on the callstack that can possibly handle
121+
// it.)
122+
v8impl::trigger_fatal_exception(env, local_err);
123+
});
109124
}
110125

111126
v8impl::Persistent<v8::Value> last_exception;

‎src/js_native_api_v8_internals.h

+2
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ using Persistent = v8::Global<T>;
3333

3434
using PersistentToLocal = node::PersistentToLocal;
3535

36+
void trigger_fatal_exception(napi_env env, v8::Local<v8::Value> local_err);
37+
3638
} // end of namespace v8impl
3739

3840
#endif // SRC_JS_NATIVE_API_V8_INTERNALS_H_

‎src/node_api.cc

+17-32
Original file line numberDiff line numberDiff line change
@@ -38,20 +38,22 @@ struct node_napi_env__ : public napi_env__ {
3838

3939
void CallFinalizer(napi_finalize cb, void* data, void* hint) override {
4040
napi_env env = static_cast<napi_env>(this);
41-
node_env()->SetImmediate([=](node::Environment* node_env) {
42-
v8::HandleScope handle_scope(env->isolate);
43-
v8::Context::Scope context_scope(env->context());
44-
env->CallIntoModule([&](napi_env env) {
45-
cb(env, data, hint);
46-
});
47-
});
41+
v8::HandleScope handle_scope(env->isolate);
42+
v8::Context::Scope context_scope(env->context());
43+
(napi_env__::CallFinalizer)(cb, data, hint);
4844
}
4945
};
5046

5147
typedef node_napi_env__* node_napi_env;
5248

5349
namespace v8impl {
5450

51+
void trigger_fatal_exception(napi_env env, v8::Local<v8::Value> local_err) {
52+
v8::Local<v8::Message> local_msg =
53+
v8::Exception::CreateMessage(env->isolate, local_err);
54+
node::errors::TriggerUncaughtException(env->isolate, local_err, local_msg);
55+
}
56+
5557
namespace {
5658

5759
class BufferFinalizer : private Finalizer {
@@ -71,12 +73,9 @@ class BufferFinalizer : private Finalizer {
7173
v8::HandleScope handle_scope(finalizer->_env->isolate);
7274
v8::Context::Scope context_scope(finalizer->_env->context());
7375

74-
finalizer->_env->CallIntoModule([&](napi_env env) {
75-
finalizer->_finalize_callback(
76-
env,
77-
finalizer->_finalize_data,
78-
finalizer->_finalize_hint);
79-
});
76+
finalizer->_env->CallFinalizer(finalizer->_finalize_callback,
77+
finalizer->_finalize_data,
78+
finalizer->_finalize_hint);
8079
});
8180
}
8281

@@ -107,13 +106,6 @@ static inline napi_env NewEnv(v8::Local<v8::Context> context) {
107106
return result;
108107
}
109108

110-
static inline void trigger_fatal_exception(
111-
napi_env env, v8::Local<v8::Value> local_err) {
112-
v8::Local<v8::Message> local_msg =
113-
v8::Exception::CreateMessage(env->isolate, local_err);
114-
node::errors::TriggerUncaughtException(env->isolate, local_err, local_msg);
115-
}
116-
117109
class ThreadSafeFunction : public node::AsyncResource {
118110
public:
119111
ThreadSafeFunction(v8::Local<v8::Function> func,
@@ -312,19 +304,17 @@ class ThreadSafeFunction : public node::AsyncResource {
312304
v8::Local<v8::Function>::New(env->isolate, ref);
313305
js_callback = v8impl::JsValueFromV8LocalValue(js_cb);
314306
}
315-
env->CallIntoModule([&](napi_env env) {
316-
call_js_cb(env, js_callback, context, data);
317-
});
307+
env->CallbackIntoModule(
308+
[&](napi_env env) { call_js_cb(env, js_callback, context, data); });
318309
}
319310
}
320311

321312
void Finalize() {
322313
v8::HandleScope scope(env->isolate);
323314
if (finalize_cb) {
324315
CallbackScope cb_scope(this);
325-
env->CallIntoModule([&](napi_env env) {
326-
finalize_cb(env, finalize_data, context);
327-
});
316+
env->CallbackIntoModule(
317+
[&](napi_env env) { finalize_cb(env, finalize_data, context); });
328318
}
329319
EmptyQueueAndDelete();
330320
}
@@ -1043,13 +1033,8 @@ class Work : public node::AsyncResource, public node::ThreadPoolWork {
10431033

10441034
CallbackScope callback_scope(this);
10451035

1046-
_env->CallIntoModule([&](napi_env env) {
1036+
_env->CallbackIntoModule([&](napi_env env) {
10471037
_complete(env, ConvertUVErrorCode(status), _data);
1048-
}, [](napi_env env, v8::Local<v8::Value> local_err) {
1049-
// If there was an unhandled exception in the complete callback,
1050-
// report it as a fatal exception. (There is no JavaScript on the
1051-
// callstack that can possibly handle it.)
1052-
v8impl::trigger_fatal_exception(env, local_err);
10531038
});
10541039

10551040
// Note: Don't access `work` after this point because it was

‎test/js-native-api/test_reference/test_finalizer.js

+20
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
'use strict';
2+
// Flags: --expose-gc --no-concurrent-array-buffer-freeing --no-concurrent-array-buffer-sweeping
3+
4+
const common = require('../../common');
5+
const test_reference = require(`./build/${common.buildType}/test_reference`);
6+
const assert = require('assert');
7+
8+
process.on('uncaughtException', common.mustCall((err) => {
9+
assert.throws(() => { throw err; }, /finalizer error/);
10+
}));
11+
12+
(async function() {
13+
{
14+
test_reference.createExternalWithJsFinalize(
15+
common.mustCall(() => {
16+
throw new Error('finalizer error');
17+
}));
18+
}
19+
global.gc();
20+
})().then(common.mustCall());

‎test/js-native-api/test_reference/test_reference.c

+41
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,20 @@ static void FinalizeExternal(napi_env env, void* data, void* hint) {
2020
finalize_count++;
2121
}
2222

23+
static void FinalizeExternalCallJs(napi_env env, void* data, void* hint) {
24+
int *actual_value = data;
25+
NAPI_ASSERT_RETURN_VOID(env, actual_value == &test_value,
26+
"The correct pointer was passed to the finalizer");
27+
28+
napi_ref finalizer_ref = (napi_ref)hint;
29+
napi_value js_finalizer;
30+
napi_value recv;
31+
NAPI_CALL_RETURN_VOID(env, napi_get_reference_value(env, finalizer_ref, &js_finalizer));
32+
NAPI_CALL_RETURN_VOID(env, napi_get_global(env, &recv));
33+
NAPI_CALL_RETURN_VOID(env, napi_call_function(env, recv, js_finalizer, 0, NULL, NULL));
34+
NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, finalizer_ref));
35+
}
36+
2337
static napi_value CreateExternal(napi_env env, napi_callback_info info) {
2438
int* data = &test_value;
2539

@@ -49,6 +63,31 @@ CreateExternalWithFinalize(napi_env env, napi_callback_info info) {
4963
return result;
5064
}
5165

66+
static napi_value
67+
CreateExternalWithJsFinalize(napi_env env, napi_callback_info info) {
68+
size_t argc = 1;
69+
napi_value args[1];
70+
NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
71+
NAPI_ASSERT(env, argc == 1, "Wrong number of arguments");
72+
napi_value finalizer = args[0];
73+
napi_valuetype finalizer_valuetype;
74+
NAPI_CALL(env, napi_typeof(env, finalizer, &finalizer_valuetype));
75+
NAPI_ASSERT(env, finalizer_valuetype == napi_function, "Wrong type of first argument");
76+
napi_ref finalizer_ref;
77+
NAPI_CALL(env, napi_create_reference(env, finalizer, 1, &finalizer_ref));
78+
79+
napi_value result;
80+
NAPI_CALL(env,
81+
napi_create_external(env,
82+
&test_value,
83+
FinalizeExternalCallJs,
84+
finalizer_ref, /* finalize_hint */
85+
&result));
86+
87+
finalize_count = 0;
88+
return result;
89+
}
90+
5291
static napi_value CheckExternal(napi_env env, napi_callback_info info) {
5392
size_t argc = 1;
5493
napi_value arg;
@@ -176,6 +215,8 @@ napi_value Init(napi_env env, napi_value exports) {
176215
DECLARE_NAPI_PROPERTY("createExternal", CreateExternal),
177216
DECLARE_NAPI_PROPERTY("createExternalWithFinalize",
178217
CreateExternalWithFinalize),
218+
DECLARE_NAPI_PROPERTY("createExternalWithJsFinalize",
219+
CreateExternalWithJsFinalize),
179220
DECLARE_NAPI_PROPERTY("checkExternal", CheckExternal),
180221
DECLARE_NAPI_PROPERTY("createReference", CreateReference),
181222
DECLARE_NAPI_PROPERTY("deleteReference", DeleteReference),

‎test/node-api/test_buffer/test_buffer.c

+36
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,17 @@ static void noopDeleter(napi_env env, void* data, void* finalize_hint) {
2020
deleterCallCount++;
2121
}
2222

23+
static void malignDeleter(napi_env env, void* data, void* finalize_hint) {
24+
NAPI_ASSERT_RETURN_VOID(env, data != NULL && strcmp(data, theText) == 0, "invalid data");
25+
napi_ref finalizer_ref = (napi_ref)finalize_hint;
26+
napi_value js_finalizer;
27+
napi_value recv;
28+
NAPI_CALL_RETURN_VOID(env, napi_get_reference_value(env, finalizer_ref, &js_finalizer));
29+
NAPI_CALL_RETURN_VOID(env, napi_get_global(env, &recv));
30+
NAPI_CALL_RETURN_VOID(env, napi_call_function(env, recv, js_finalizer, 0, NULL, NULL));
31+
NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, finalizer_ref));
32+
}
33+
2334
static napi_value newBuffer(napi_env env, napi_callback_info info) {
2435
napi_value theBuffer;
2536
char* theCopy;
@@ -119,6 +130,30 @@ static napi_value staticBuffer(napi_env env, napi_callback_info info) {
119130
return theBuffer;
120131
}
121132

133+
static napi_value malignFinalizerBuffer(napi_env env, napi_callback_info info) {
134+
size_t argc = 1;
135+
napi_value args[1];
136+
NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
137+
NAPI_ASSERT(env, argc == 1, "Wrong number of arguments");
138+
napi_value finalizer = args[0];
139+
napi_valuetype finalizer_valuetype;
140+
NAPI_CALL(env, napi_typeof(env, finalizer, &finalizer_valuetype));
141+
NAPI_ASSERT(env, finalizer_valuetype == napi_function, "Wrong type of first argument");
142+
napi_ref finalizer_ref;
143+
NAPI_CALL(env, napi_create_reference(env, finalizer, 1, &finalizer_ref));
144+
145+
napi_value theBuffer;
146+
NAPI_CALL(
147+
env,
148+
napi_create_external_buffer(env,
149+
sizeof(theText),
150+
(void*)theText,
151+
malignDeleter,
152+
finalizer_ref, // finalize_hint
153+
&theBuffer));
154+
return theBuffer;
155+
}
156+
122157
static napi_value Init(napi_env env, napi_value exports) {
123158
napi_value theValue;
124159

@@ -134,6 +169,7 @@ static napi_value Init(napi_env env, napi_value exports) {
134169
DECLARE_NAPI_PROPERTY("bufferHasInstance", bufferHasInstance),
135170
DECLARE_NAPI_PROPERTY("bufferInfo", bufferInfo),
136171
DECLARE_NAPI_PROPERTY("staticBuffer", staticBuffer),
172+
DECLARE_NAPI_PROPERTY("malignFinalizerBuffer", malignFinalizerBuffer),
137173
};
138174

139175
NAPI_CALL(env, napi_define_properties(

‎test/node-api/test_buffer/test_finalizer.js

+21
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
'use strict';
2+
// Flags: --expose-gc --no-concurrent-array-buffer-freeing --no-concurrent-array-buffer-sweeping
3+
4+
const common = require('../../common');
5+
const binding = require(`./build/${common.buildType}/test_buffer`);
6+
const assert = require('assert');
7+
const tick = require('util').promisify(require('../../common/tick'));
8+
9+
process.on('uncaughtException', common.mustCall((err) => {
10+
assert.throws(() => { throw err; }, /finalizer error/);
11+
}));
12+
13+
(async function() {
14+
{
15+
binding.malignFinalizerBuffer(common.mustCall(() => {
16+
throw new Error('finalizer error');
17+
}));
18+
}
19+
global.gc();
20+
await tick(10);
21+
})().then(common.mustCall());

‎test/node-api/test_threadsafe_function/binding.c

+30
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,35 @@ static napi_value StartThreadNoJsFunc(napi_env env, napi_callback_info info) {
267267
/** block_on_full */true, /** alt_ref_js_cb */true);
268268
}
269269

270+
// Testing calling into JavaScript
271+
static void ThreadSafeFunctionFinalize(napi_env env,
272+
void* finalize_data,
273+
void* finalize_hint) {
274+
napi_ref js_func_ref = (napi_ref) finalize_data;
275+
napi_value js_func;
276+
napi_value recv;
277+
NAPI_CALL_RETURN_VOID(env, napi_get_reference_value(env, js_func_ref, &js_func));
278+
NAPI_CALL_RETURN_VOID(env, napi_get_global(env, &recv));
279+
NAPI_CALL_RETURN_VOID(env, napi_call_function(env, recv, js_func, 0, NULL, NULL));
280+
NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, js_func_ref));
281+
}
282+
283+
// Testing calling into JavaScript
284+
static napi_value CallIntoModule(napi_env env, napi_callback_info info) {
285+
size_t argc = 4;
286+
napi_value argv[4];
287+
NAPI_CALL(env, napi_get_cb_info(env, info, &argc, argv, NULL, NULL));
288+
289+
napi_ref finalize_func;
290+
NAPI_CALL(env, napi_create_reference(env, argv[3], 1, &finalize_func));
291+
292+
napi_threadsafe_function tsfn;
293+
NAPI_CALL(env, napi_create_threadsafe_function(env, argv[0], argv[1], argv[2], 0, 1, finalize_func, ThreadSafeFunctionFinalize, NULL, NULL, &tsfn));
294+
NAPI_CALL(env, napi_call_threadsafe_function(tsfn, NULL, napi_tsfn_blocking));
295+
NAPI_CALL(env, napi_release_threadsafe_function(tsfn, napi_tsfn_release));
296+
return NULL;
297+
}
298+
270299
// Module init
271300
static napi_value Init(napi_env env, napi_value exports) {
272301
size_t index;
@@ -305,6 +334,7 @@ static napi_value Init(napi_env env, napi_value exports) {
305334
DECLARE_NAPI_PROPERTY("StopThread", StopThread),
306335
DECLARE_NAPI_PROPERTY("Unref", Unref),
307336
DECLARE_NAPI_PROPERTY("Release", Release),
337+
DECLARE_NAPI_PROPERTY("CallIntoModule", CallIntoModule),
308338
};
309339

310340
NAPI_CALL(env, napi_define_properties(env, exports,

‎test/node-api/test_threadsafe_function/test_uncaught_exception.js

+26
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
'use strict';
2+
3+
const common = require('../../common');
4+
const assert = require('assert');
5+
const binding = require(`./build/${common.buildType}/binding`);
6+
7+
const callbackCheck = common.mustCall((err) => {
8+
assert.throws(() => { throw err; }, /callback error/);
9+
process.removeListener('uncaughtException', callbackCheck);
10+
process.on('uncaughtException', finalizerCheck);
11+
});
12+
const finalizerCheck = common.mustCall((err) => {
13+
assert.throws(() => { throw err; }, /finalizer error/);
14+
});
15+
process.on('uncaughtException', callbackCheck);
16+
17+
binding.CallIntoModule(
18+
common.mustCall(() => {
19+
throw new Error('callback error');
20+
}),
21+
{},
22+
'resource_name',
23+
common.mustCall(function finalizer() {
24+
throw new Error('finalizer error');
25+
})
26+
);

0 commit comments

Comments
 (0)
Please sign in to comment.