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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid EventEmitter crashes #10889

Merged
merged 5 commits into from Oct 27, 2017

Conversation

Projects
None yet
3 participants
@mgc
Contributor

mgc commented Oct 24, 2017

This is an attempt to avoid #10527, in which notification events like close and show sometimes cause a crash.

First, the close event was a bit of a lie, as it was being dispatched somewhat randomly when the notification was GCed, not when the user actually dismissed it. Fixing this means not having to deal with emitting the event inside the destructor.

Second, it seems like there are some subtle bugs in native-mate. I'm not sure what is going on, but in reading through how Muon does things I noticed a few sensible looking changes from @bridiver: brave/muon@58c0ec0#diff-a31bffb28ca1eed32c06c2d6b8b53f45 brave/muon#323

Here is an additional native-mate PR: electron/native-mate#15

@mgc mgc self-assigned this Oct 24, 2017

@mgc mgc requested review from zcbenz and MarshallOfSound Oct 24, 2017

@mgc mgc requested review from electron/docs as code owners Oct 24, 2017

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Oct 24, 2017

Got an alternative fix for this which I believe solves root cause, might be worth trying that 👍

8bab2f9

@mgc

This comment has been minimized.

Contributor

mgc commented Oct 26, 2017

@MarshallOfSound @zcbenz how would you feel about just merging this patch regardless?

  • Current notification close event is broken either way, in that the behavior doesn't match the docs?
  • If for whatever reason a wrapper is empty, it would be preferable for emitting an event or marking destroyed to be a no-op rather than a crash?
Wrappable<T>::GetWrapper()->SetAlignedPointerInInternalField(0, nullptr);
v8::Local<v8::Object> wrapper = Wrappable<T>::GetWrapper();
if (!wrapper.IsEmpty()) {
Wrappable<T>::GetWrapper()->SetAlignedPointerInInternalField(0, nullptr);

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 27, 2017

Member

s/Wrappable<T>::GetWrapper()/wrapper ?

This comment has been minimized.

@mgc

mgc Oct 27, 2017

Contributor

Good point. Updating!

@jkleinsc jkleinsc merged commit b429daf into master Oct 27, 2017

8 checks passed

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/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-mas-x64 Build #5559 succeeded in 12 min
Details
electron-osx-x64 Build #5539 succeeded in 13 min
Details

@zcbenz zcbenz deleted the avoid-eventemitter-crashes branch Oct 31, 2017

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