Wait for windows' state to be saved before closing the app or any window #12619
Conversation
Previously, we used to save the window's state in the renderer process `beforeunload` event handler: because of the synchronous nature of event handlers and the asynchronous design of IndexedDB, this could potentially not save anything if windows close fast enough to prevent IndexedDB from committing the pending transaction containing the state. (Ref.: https://mzl.la/2bXCXDn) With this commit, we will intercept the `before-quit` events on `electron.app` and the `close` event on `BrowserWindow` (which will fire respectively before quitting the application and before closing a window), and prevent them from performing the default action. We will then ask each renderer process to save its state and, finally, close the window and/or the app.
| @@ -322,6 +324,8 @@ class AtomApplication | |||
|
|
|||
| @disposable.add ipcHelpers.on ipcMain, 'did-cancel-window-unload', => | |||
| @quitting = false | |||
| for window in @windows | |||
| window.didCancelWindowUnload() | |||
There was a problem hiding this comment.
Might not be more readable, and certainly not important, but these two lines could be compacted into
window.didCancelWindowUnload() for window in @windowsThere was a problem hiding this comment.
Oh, and we should probably call it something else to avoid shadowing the window global? Better safe than sorry 🙈
There was a problem hiding this comment.
Oh, and we should probably call it something else to avoid shadowing the window global? Better safe than sorry
I think the main process doesn't have a window global, so we should be good. 👍
|
LGTM! 🚀 |
| @@ -368,6 +370,23 @@ describe('AtomApplication', function () { | |||
| }) | |||
| }) | |||
|
|
|||
| describe('before quitting', function () { | |||
| it('waits until all the windows have saved their state and then quits', async function () { | |||
There was a problem hiding this comment.
Yo @as-cii, is this a new feature in Atom's spec runner? I previously used it and stuff from jasmine-fix to make it accept async callbacks
There was a problem hiding this comment.
@steelbrain These are integration tests, and are being executed outside of Atom's test runner harness. They are using a more modern release of Jasmine ✨
There was a problem hiding this comment.
These are main process tests and use Mocha. Eventually we want to use Mocha for our core render process tests too but its not a priority right now. You can use Mocha today for packages via atom-mocha-test-runner.
There was a problem hiding this comment.
These are main process tests and use Mocha.
Oh wow, I totally forgot about that. But I remember it now because of the explicit function vs arrow function discussion.
Thanks for setting the record straight 🙇
| event.preventDefault() | ||
| @unloading = true | ||
| @atomApplication.saveState(false) | ||
| @saveState().then(=> @close()) |
There was a problem hiding this comment.
Should we wait to assign @unloading = true until after the close promise resolves? I could see a weird case where close is fired twice before the state finishes saving.
There was a problem hiding this comment.
I could see a weird case where close is fired twice before the state finishes saving.
Yeah, depending on how big the state is (and, thus, how long it takes to serialize it), the user might be able to click the exit button more than once in rapid succession and cause the state to be serialized twice. I don't think it would be extremely, but it seems better to avoid it if we can.
|
Quick and awesome work to clear this obstacle @as-cii. ⚡ |
|
Why are renderers responsible for writing to disk? A renderer, by definition, is a one-way method. Maybe the entire write-to-disk logic should be moved somewhere else? |
In this case we're borrowing Electron's terminology to draw a distinction between the main process and the windows (render) processes. Each window is responsible for maintaining its own state and, since renderer processes have also access to the IndexedDB API, they just serialize and save state by themselves. I guess at some point we might let the main process handle serialization/persistence of windows' state, but that would come with the cost of transmitting potentially big object graphs via IPC and I am not sure it would be worth it. Moreover, I don't think the IndexedDB API is exposed in the main process, which would probably require us switching to a different backing store. |
This regression was caused by a nuance in the way we maintain state in `AtomApplication` for open windows. Specifically, when closing the last window on Windows and Linux, we were explicitly calling `app.quit` *before* removing such window from the list of open ones. In turn, this caused the new `before-quit` behavior introduced in #12619 to work improperly because it made the application wait on saving the state of a stale window before exiting. With this commit we are fixing that by making sure the stale window is removed before calling `app.quit` in `removeWindow`.
This fixes a test failure on #12300 where the new Electron version prevents state from being serialized.
Previously, we used to save the window's state in the renderer process
beforeunloadevent handler: because of the synchronous nature of event handlers and the asynchronous design of IndexedDB, this could potentially not save anything if windows close fast enough to prevent IndexedDB from committing the pending transaction containing the state. (Ref.: https://mzl.la/2bXCXDn)With this commit, we will intercept the
before-quitevents onelectron.appand thecloseevent onBrowserWindow(which will fire respectively before quitting the application and before closing awindow), and prevent them from performing the default action. We will then ask each renderer process to save its state and, finally, close the window and/or the app when the IndexedDB transaction completes.
/cc: @atom/core @thomasjo