From fe756bdc8666e4baf46901e27d7fb1880209dba6 Mon Sep 17 00:00:00 2001 From: Yavor Georgiev Date: Tue, 11 Jul 2017 13:17:41 +0200 Subject: [PATCH 1/3] Rethrow callback errors as fatal Node.js errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of rethrowing the JavaScript error emitted from a callback function, we should instead pass it to `node::FatalException`. Callbacks are employed when there is no other JavaScript frame on the stack prior so rethrowing the JS error as a C++ exception is not going to propagate the error back to JavaScript. `node::FatalException` will raise the `uncaughtError` event on the `process` object , print the error and stacktrace, and alert the debugger is there is one attached. This would make our async callbacks behave consistently with Node’s own async callbacks such as `setTimeout` when encountering an error. --- CHANGELOG.md | 13 +++++++++++++ src/node/node_function.hpp | 6 +++--- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d5c155b..e1d45805 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,16 @@ +vNext Release notes (TBD) +============================================================= +### Breaking changes +* None + +### Enhancements +* None + +### Bug fixes +* Fix crash on Node.js when a listener callback throws an error. + The error will now be forwarded to Node's fatal error handling facilities. This means better error reporting, + the ability to debug such errors in a Node.js debugger, and proper invocation of the `uncaughtError` event on the `process` object. + 1.9.0 Release notes (2017-7-10) ============================================================= ### Breaking changes diff --git a/src/node/node_function.hpp b/src/node/node_function.hpp index cb07959a..56c33498 100644 --- a/src/node/node_function.hpp +++ b/src/node/node_function.hpp @@ -38,13 +38,13 @@ inline v8::Local node::Function::call(v8::Isolate* isolate, const v8: template<> inline v8::Local node::Function::callback(v8::Isolate* isolate, const v8::Local &function, const v8::Local &this_object, size_t argc, const v8::Local arguments[]) { - Nan::TryCatch trycatch; + v8::TryCatch trycatch(isolate); auto recv = this_object.IsEmpty() ? isolate->GetCurrentContext()->Global() : this_object; - auto result = Nan::MakeCallback(recv, function, (int)argc, const_cast*>(arguments)); + auto result = ::node::MakeCallback(isolate, recv, function, (int)argc, const_cast*>(arguments)); if (trycatch.HasCaught()) { - throw node::Exception(isolate, trycatch.Exception()); + ::node::FatalException(isolate, trycatch); } return result; } From 1e58351ace7189166163b2d0268944d1fc5a82df Mon Sep 17 00:00:00 2001 From: Yavor Georgiev Date: Tue, 11 Jul 2017 16:10:22 +0200 Subject: [PATCH 2/3] =?UTF-8?q?Don=E2=80=99t=20enter=20node::MakeCallback?= =?UTF-8?q?=20if=20we=20have=20a=20calling=20context?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit this means we already have JavaScript frames on the execution stack --- src/node/node_function.hpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/node/node_function.hpp b/src/node/node_function.hpp index 56c33498..914dbed7 100644 --- a/src/node/node_function.hpp +++ b/src/node/node_function.hpp @@ -38,6 +38,14 @@ inline v8::Local node::Function::call(v8::Isolate* isolate, const v8: template<> inline v8::Local node::Function::callback(v8::Isolate* isolate, const v8::Local &function, const v8::Local &this_object, size_t argc, const v8::Local arguments[]) { + if (!isolate->GetCallingContext().IsEmpty()) { + // if there are any JavaScript frames on the stack below this one we don't need to + // go through the trouble of calling MakeCallback. MakeCallback is only for when a + // thread with no JavaScript frames on its stack needs to call into JavaScript, like in + // an uv_async callback. + return call(isolate, function, this_object, argc, arguments); + } + v8::TryCatch trycatch(isolate); auto recv = this_object.IsEmpty() ? isolate->GetCurrentContext()->Global() : this_object; From b1d23bb7824ac276f69e35589f33b8ecfeff6831 Mon Sep 17 00:00:00 2001 From: Yavor Georgiev Date: Tue, 11 Jul 2017 17:25:39 +0200 Subject: [PATCH 3/3] Realm.open and Realm.openAsync should try/catch opening the realm and pass the error on to the promise and callback respectively so that it can be handled by the caller --- lib/extensions.js | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/extensions.js b/lib/extensions.js index c8f3d339..8e8e838a 100644 --- a/lib/extensions.js +++ b/lib/extensions.js @@ -49,9 +49,13 @@ module.exports = function(realmConstructor) { reject(error); } - let syncedRealm = new realmConstructor(config); - //FIXME: RN hangs here. Remove when node's makeCallback alternative is implemented - setTimeout(() => { resolve(syncedRealm); }, 1); + try { + let syncedRealm = new this(config); + //FIXME: RN hangs here. Remove when node's makeCallback alternative is implemented + setTimeout(() => { resolve(syncedRealm); }, 1); + } catch (e) { + reject(e); + } }); }); }, @@ -62,9 +66,13 @@ module.exports = function(realmConstructor) { callback(error); } - let syncedRealm = new realmConstructor(config); - //FIXME: RN hangs here. Remove when node's makeCallback alternative is implemented - setTimeout(() => { callback(null, syncedRealm); }, 1); + try { + let syncedRealm = new this(config); + //FIXME: RN hangs here. Remove when node's makeCallback alternative is implemented + setTimeout(() => { callback(null, syncedRealm); }, 1); + } catch (e) { + callback(e); + } }); }, }));