From aa6b9a502582534a9853bab5b32a8103380809cd Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Mon, 13 Nov 2017 03:00:16 +1100 Subject: [PATCH 1/6] Fix crash when emitting unhandled error on native EventEmitter --- atom/common/api/event_emitter_caller.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/atom/common/api/event_emitter_caller.cc b/atom/common/api/event_emitter_caller.cc index 50d586680d50..1f6b5e81b58d 100644 --- a/atom/common/api/event_emitter_caller.cc +++ b/atom/common/api/event_emitter_caller.cc @@ -20,8 +20,12 @@ v8::Local CallMethodWithArgs(v8::Isolate* isolate, v8::MicrotasksScope::kRunMicrotasks); // Use node::MakeCallback to call the callback, and it will also run pending // tasks in Node.js. - return node::MakeCallback(isolate, obj, method, args->size(), &args->front(), - {0, 0}).ToLocalChecked(); + v8::MaybeLocal ret = node::MakeCallback(isolate, obj, method, args->size(), &args->front(), + {0, 0}); + if (ret.IsEmpty()) { + return v8::Boolean::New(isolate, false); + } + return ret.ToLocalChecked(); } } // namespace internal From 3c0b233d043e75d257217cf025b25cef40260e7b Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Mon, 13 Nov 2017 11:42:42 +1100 Subject: [PATCH 2/6] Fix line length in caller.cc --- atom/common/api/event_emitter_caller.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/atom/common/api/event_emitter_caller.cc b/atom/common/api/event_emitter_caller.cc index 1f6b5e81b58d..746b27006b9c 100644 --- a/atom/common/api/event_emitter_caller.cc +++ b/atom/common/api/event_emitter_caller.cc @@ -20,8 +20,9 @@ v8::Local CallMethodWithArgs(v8::Isolate* isolate, v8::MicrotasksScope::kRunMicrotasks); // Use node::MakeCallback to call the callback, and it will also run pending // tasks in Node.js. - v8::MaybeLocal ret = node::MakeCallback(isolate, obj, method, args->size(), &args->front(), - {0, 0}); + v8::MaybeLocal ret = node::MakeCallback(isolate, obj, method, + args->size(), + &args->front(), {0, 0}); if (ret.IsEmpty()) { return v8::Boolean::New(isolate, false); } From 8a8f1696285994e31f8a428d9ae98441b27adaa1 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Wed, 15 Nov 2017 18:21:47 +1100 Subject: [PATCH 3/6] Add comments and use ToLocal instead of ToLocalChecked --- atom/common/api/event_emitter_caller.cc | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/atom/common/api/event_emitter_caller.cc b/atom/common/api/event_emitter_caller.cc index 746b27006b9c..e89e9d7533ae 100644 --- a/atom/common/api/event_emitter_caller.cc +++ b/atom/common/api/event_emitter_caller.cc @@ -20,13 +20,22 @@ v8::Local CallMethodWithArgs(v8::Isolate* isolate, v8::MicrotasksScope::kRunMicrotasks); // Use node::MakeCallback to call the callback, and it will also run pending // tasks in Node.js. - v8::MaybeLocal ret = node::MakeCallback(isolate, obj, method, - args->size(), - &args->front(), {0, 0}); + v8::MaybeLocal ret = node::MakeCallback(isolate, obj, method, args->size(), &args->front(), + {0, 0}); + // If the JS function throws an exception (doesn't return a value) the result + // of MakeCallback will be empty, in this case we need to return "false" as + // that indicates that the event emitter did not handle the event if (ret.IsEmpty()) { return v8::Boolean::New(isolate, false); } - return ret.ToLocalChecked(); + + v8::Local localRet; + if (ret.ToLocal(&localRet)) { + return localRet; + } + // Should be unreachable, but the compiler complains if we don't check + // the result of ToLocal + return v8::Undefined(isolate); } } // namespace internal From 57f934a806ee733dba5a7f0d049f6a16779de57a Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Wed, 15 Nov 2017 18:23:24 +1100 Subject: [PATCH 4/6] Appease the linter --- atom/common/api/event_emitter_caller.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/atom/common/api/event_emitter_caller.cc b/atom/common/api/event_emitter_caller.cc index e89e9d7533ae..a3b454b8661e 100644 --- a/atom/common/api/event_emitter_caller.cc +++ b/atom/common/api/event_emitter_caller.cc @@ -20,8 +20,9 @@ v8::Local CallMethodWithArgs(v8::Isolate* isolate, v8::MicrotasksScope::kRunMicrotasks); // Use node::MakeCallback to call the callback, and it will also run pending // tasks in Node.js. - v8::MaybeLocal ret = node::MakeCallback(isolate, obj, method, args->size(), &args->front(), - {0, 0}); + v8::MaybeLocal ret = node::MakeCallback(isolate, obj, method, + args->size(), + &args->front(), {0, 0}); // If the JS function throws an exception (doesn't return a value) the result // of MakeCallback will be empty, in this case we need to return "false" as // that indicates that the event emitter did not handle the event From bdbc6bb1654fb4a4dad1da3348e4b6d74d45ca65 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Fri, 17 Nov 2017 06:09:35 +1100 Subject: [PATCH 5/6] Clean up empty logic --- atom/common/api/event_emitter_caller.cc | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/atom/common/api/event_emitter_caller.cc b/atom/common/api/event_emitter_caller.cc index a3b454b8661e..d46eb1dfa55e 100644 --- a/atom/common/api/event_emitter_caller.cc +++ b/atom/common/api/event_emitter_caller.cc @@ -24,19 +24,14 @@ v8::Local CallMethodWithArgs(v8::Isolate* isolate, args->size(), &args->front(), {0, 0}); // If the JS function throws an exception (doesn't return a value) the result - // of MakeCallback will be empty, in this case we need to return "false" as - // that indicates that the event emitter did not handle the event - if (ret.IsEmpty()) { - return v8::Boolean::New(isolate, false); - } - + // of MakeCallback will be empty and therefore ToLocal will be false, in this + // case we need to return "false" as that indicates that the event emitter did + // not handle the event v8::Local localRet; if (ret.ToLocal(&localRet)) { return localRet; } - // Should be unreachable, but the compiler complains if we don't check - // the result of ToLocal - return v8::Undefined(isolate); + return v8::Boolean::New(isolate, false); } } // namespace internal From fc265b7600199e30a3843ad56c42a21f44059367 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Fri, 17 Nov 2017 06:15:53 +1100 Subject: [PATCH 6/6] linter plz --- atom/common/api/event_emitter_caller.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atom/common/api/event_emitter_caller.cc b/atom/common/api/event_emitter_caller.cc index d46eb1dfa55e..ff920c679733 100644 --- a/atom/common/api/event_emitter_caller.cc +++ b/atom/common/api/event_emitter_caller.cc @@ -26,7 +26,7 @@ v8::Local CallMethodWithArgs(v8::Isolate* isolate, // If the JS function throws an exception (doesn't return a value) the result // of MakeCallback will be empty and therefore ToLocal will be false, in this // case we need to return "false" as that indicates that the event emitter did - // not handle the event + // not handle the event v8::Local localRet; if (ret.ToLocal(&localRet)) { return localRet;