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

Fixing uncaught exception on window close #10275

Merged
merged 2 commits into from Aug 19, 2017

Conversation

Projects
None yet
4 participants
@juturu
Contributor

juturu commented Aug 15, 2017

Fixes #10274

@zeke

zeke approved these changes Aug 17, 2017

Looks good to me.

@@ -465,6 +465,10 @@ ipcMain.on('ELECTRON_BROWSER_WINDOW_CONFIRM', function (event, message, title) {
// Implements window.close()
ipcMain.on('ELECTRON_BROWSER_WINDOW_CLOSE', function (event) {
event.sender.getOwnerBrowserWindow().close()

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Aug 17, 2017

Member

try {} catch (err) {} is quite costly if I recall correctly.

Can we just check if the window is null or not? I.e.

const window = event.sender.getOwnerBrowserWindow();
if (window) {
  window.close()
}
event.returnValue = null

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Aug 17, 2017

Member

Or is the purpose of this to propagate the error rather than prevent it

This comment has been minimized.

@juturu

juturu Aug 17, 2017

Contributor

I tried to follow how other methods in this file had try catch. I started with null check and changed into this based on other method implementation. Let me know if you feel we should change into this.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Aug 17, 2017

Member

I feel as though this should be a null check, looking through the rest of the file the other try/catch blocks are for potentially user influenced code. Like require or calling functions or global.

This is just an edge case we need to handle I think with a null check (bug in code vs error that should be propagated). Thoughts?

This comment has been minimized.

@juturu

juturu Aug 17, 2017

Contributor

Makes sense. Updated, thanks @MarshallOfSound

This comment has been minimized.

@YurySolovyov

YurySolovyov Aug 17, 2017

Contributor

I think try/catch is not a problem with TurboFan since Chrome 59

@MarshallOfSound MarshallOfSound merged commit 15db4ee into master Aug 19, 2017

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
electron-mas-x64 Build #4881 succeeded in 30 min
Details
electron-osx-x64 Build #4875 succeeded in 25 min
Details
electron-win-ia32 Build #3898 succeeded in 11 min
Details

@MarshallOfSound MarshallOfSound deleted the windowclose-exception branch Aug 19, 2017

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