Skip to content
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

fix: prevent crash on web-contents creation when error is thrown #28971

Merged
merged 8 commits into from May 11, 2021

Conversation

VerteDinde
Copy link
Member

@VerteDinde VerteDinde commented May 3, 2021

Description of Change

Fixes #28749

Currently, if an unhandled error is thrown during web-contents creation on a new browser window within JavaScript's scope, the error is not being correctly bubbled up, causing a DCHECK and hard app crash. More specifically, here are the steps that could lead to a crash (thanks @MarshallOfSound for the breakdown!):

  • User's JS code triggers some native code
  • The native code emits an event
  • The event is emitted in the same scope as the JS code that triggered the native code
  • The event handler throws an error
  • The native code then tries to run more JS (emit another event, call a getter, etc.), which cannot happen because there is a pending exception

This PR adds Node's exception error handler to the _init method for web-contents creation, bubbling the error into a different scope and allowing us to handle it more appropriately, rather than hard crashing.

Checklist

Release Notes

Notes: Fix crash when an exception occurs within the event emitter.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label May 3, 2021
@VerteDinde VerteDinde force-pushed the catch-event-emmitter-errors branch 2 times, most recently from 91f9dca to 6637fd9 Compare May 5, 2021 05:13
@VerteDinde VerteDinde changed the title fix: prevent crash when exception occurs within the event emitter fix: prevent crash on web-contents creation when error is thrown May 5, 2021
@zcbenz zcbenz added the semver/patch backwards-compatible bug fixes label May 5, 2021
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label May 5, 2021
@zcbenz
Copy link
Member

zcbenz commented May 5, 2021

Can you add test for this? Putting https://gist.github.com/nornagon/2b0373a300af0931c0c29b0e8dff8b73 under spec-main/fixtures/crash-cases/ should be enough.

@VerteDinde
Copy link
Member Author

@zcbenz Will do! Thanks for reviewing! 🙂

shell/browser/api/electron_api_web_contents.cc Outdated Show resolved Hide resolved
@VerteDinde VerteDinde force-pushed the catch-event-emmitter-errors branch from 6637fd9 to e7a5d89 Compare May 5, 2021 20:31
@VerteDinde VerteDinde force-pushed the catch-event-emmitter-errors branch from ba2ebea to 98e5080 Compare May 5, 2021 20:48
@VerteDinde VerteDinde force-pushed the catch-event-emmitter-errors branch 2 times, most recently from a40f874 to 7c66202 Compare May 6, 2021 19:29
@VerteDinde VerteDinde force-pushed the catch-event-emmitter-errors branch from 7c66202 to bbfa943 Compare May 6, 2021 21:29
@MarshallOfSound MarshallOfSound merged commit 06f51b7 into master May 11, 2021
@release-clerk
Copy link

release-clerk bot commented May 11, 2021

Release Notes Persisted

Fix crash when an exception occurs within the event emitter.

@MarshallOfSound MarshallOfSound deleted the catch-event-emmitter-errors branch May 11, 2021 20:57
@trop
Copy link
Contributor

trop bot commented May 11, 2021

I have automatically backported this PR to "13-x-y", please check out #29106

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when throwing an error in app.on('web-contents-created')
6 participants