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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when emitting unhandled error on native EventEmitter #11099

Merged
merged 6 commits into from Nov 16, 2017

Conversation

Projects
None yet
4 participants
@MarshallOfSound
Member

MarshallOfSound commented Nov 12, 2017

This is very similar to my original fix but is now semantically correct and I can actually explain how it fixes it 馃憤

Basically the return of node::MakeCallback will be empty if the method threw an exception. Turns out that any call to emit('error') on an EventEmitter that didn't have an error handler threw a synchronous error inline with the emit call (wut......). This simply handles the empty case by claiming the method returned false (the event was unhandled) which is semantically correct in regards to the EventEmitter documentation.

See https://github.com/nodejs/node/blob/master/lib/events.js#L176 for the relevant EventEmitter code.

Fixes #10433

/cc @bpasero

@MarshallOfSound MarshallOfSound requested a review from electron/reviewers as a code owner Nov 12, 2017

@MarshallOfSound MarshallOfSound requested review from mgc and ckerr Nov 12, 2017

@@ -20,8 +20,12 @@ v8::Local<v8::Value> 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<v8::Value> ret = node::MakeCallback(isolate, obj, method, args->size(), &args->front(),

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Nov 12, 2017

Contributor

@MarshallOfSound The linter doesn't seem to like the length of this line.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Nov 13, 2017

Member

Fixed 馃憤

v8::MaybeLocal<v8::Value> ret = node::MakeCallback(isolate, obj, method,
args->size(),
&args->front(), {0, 0});
if (ret.IsEmpty()) {

This comment has been minimized.

@ckerr

ckerr Nov 13, 2017

Member

Nice job finding this! Since this was such a hard bug to find and understand, the top thing I'd want in this fix is a short code comment explaining how node::MakeCallback() can fail

if (ret.IsEmpty()) {
return v8::Boolean::New(isolate, false);
}
return ret.ToLocalChecked();

This comment has been minimized.

@ckerr

ckerr Nov 13, 2017

Member

Use of ToLocalChecked() is odd here: (1) it involves a redundant IsEmpty() test, and (2) its purpose in life is to crash on failure, which we're explicitly avoiding. It makes more sense to use bool MaybeLocal::ToLocal(Local<S>* out) const instead.

{0, 0}).ToLocalChecked();
v8::MaybeLocal<v8::Value> ret = node::MakeCallback(isolate, obj, method,
args->size(),
&args->front(), {0, 0});

This comment has been minimized.

@ckerr

ckerr Nov 13, 2017

Member

Question -- not part of the review, as this isn't new to your patch anyway -- why do we use {0, 0} instead of {}? Do we support a compiler that doesn't support uniform initialization?

This comment has been minimized.

@zcbenz

zcbenz Nov 14, 2017

Contributor

{0, 0} is used in Node's source code, we are following their style.

This comment has been minimized.

@ckerr

ckerr Nov 14, 2017

Member

Makes sense. Thanks for the answer :)

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Nov 15, 2017

@ckerr Comments added and ToLocal used 馃憤

}
// Should be unreachable, but the compiler complains if we don't check
// the result of ToLocal
return v8::Undefined(isolate);

This comment has been minimized.

@ckerr

ckerr Nov 16, 2017

Member

I appreciate the new comment and using bool ToLocal() so I hate to complain further, but I don't understand why we need a third return point to make the compiler happy? What would be wrong with

// 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
v8::Local<v8::Value> localVal;
return ret.ToLocal(&localVal))
       ? localVal
       : v8::Boolean::New(isolate, false);
@ckerr

LGTM.

Waiting for CI before approve & merge

@ckerr

ckerr approved these changes Nov 16, 2017

@ckerr ckerr merged commit 9f922e9 into master Nov 16, 2017

6 of 7 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/branch This commit looks good
Details

@ckerr ckerr deleted the fix-emit-call-crash branch Nov 16, 2017

juturu pushed a commit that referenced this pull request May 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment