From 0f6eadcfceb27784a65f9898b301fa5839c71210 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 7 Sep 2016 11:27:47 +0200 Subject: [PATCH 1/3] Wait for windows' state to be saved before closing the app or any window 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. --- spec/atom-environment-spec.coffee | 7 ++---- spec/integration/helpers/start-atom.coffee | 9 +------- src/application-delegate.coffee | 11 +++++++++ src/atom-environment.coffee | 26 ++++++++++++---------- src/main-process/atom-application.coffee | 12 ++++++---- src/main-process/atom-window.coffee | 22 +++++++++++++++--- 6 files changed, 55 insertions(+), 32 deletions(-) diff --git a/spec/atom-environment-spec.coffee b/spec/atom-environment-spec.coffee index fa8c3cc5d..cd8408a73 100644 --- a/spec/atom-environment-spec.coffee +++ b/spec/atom-environment-spec.coffee @@ -228,7 +228,7 @@ describe "AtomEnvironment", -> expect(atom.saveState).toHaveBeenCalledWith({isUnloading: false}) expect(atom.saveState).not.toHaveBeenCalledWith({isUnloading: true}) - it "saves state immediately when unloading the editor window, ignoring pending and successive mousedown/keydown events", -> + it "ignores mousedown/keydown events happening after calling unloadEditorWindow", -> spyOn(atom, 'saveState') idleCallbacks = [] spyOn(window, 'requestIdleCallback').andCallFake (callback) -> idleCallbacks.push(callback) @@ -236,15 +236,12 @@ describe "AtomEnvironment", -> mousedown = new MouseEvent('mousedown') atom.document.dispatchEvent(mousedown) atom.unloadEditorWindow() - expect(atom.saveState).toHaveBeenCalledWith({isUnloading: true}) - expect(atom.saveState).not.toHaveBeenCalledWith({isUnloading: false}) + expect(atom.saveState).not.toHaveBeenCalled() - atom.saveState.reset() advanceClock atom.saveStateDebounceInterval idleCallbacks.shift()() expect(atom.saveState).not.toHaveBeenCalled() - atom.saveState.reset() mousedown = new MouseEvent('mousedown') atom.document.dispatchEvent(mousedown) advanceClock atom.saveStateDebounceInterval diff --git a/spec/integration/helpers/start-atom.coffee b/spec/integration/helpers/start-atom.coffee index 17f11fe2c..1eb610a2f 100644 --- a/spec/integration/helpers/start-atom.coffee +++ b/spec/integration/helpers/start-atom.coffee @@ -125,11 +125,6 @@ buildAtomClient = (args, env) -> @execute "atom.commands.dispatch(document.activeElement, '#{command}')" .call(done) - .addCommand "simulateQuit", (done) -> - @execute -> atom.unloadEditorWindow() - .execute -> require("electron").remote.app.emit("before-quit") - .call(done) - module.exports = (args, env, fn) -> [chromedriver, chromedriverLogs, chromedriverExit] = [] @@ -154,9 +149,7 @@ module.exports = (args, env, fn) -> waitsFor("tests to run", (done) -> finish = once -> - client - .simulateQuit() - .end() + client.end() .then(-> chromedriver.kill()) .then(chromedriverExit.then( (errorCode) -> diff --git a/src/application-delegate.coffee b/src/application-delegate.coffee index 853ac54f5..f2979ba0d 100644 --- a/src/application-delegate.coffee +++ b/src/application-delegate.coffee @@ -244,6 +244,17 @@ class ApplicationDelegate new Disposable -> ipcRenderer.removeListener('context-command', outerCallback) + onSaveWindowState: (callback) -> + outerCallback = (event, message) -> + callback(event) + + ipcRenderer.on('save-window-state', outerCallback) + new Disposable -> + ipcRenderer.removeListener('save-window-state', outerCallback) + + didSaveWindowState: -> + ipcRenderer.send('did-save-window-state') + didCancelWindowUnload: -> ipcRenderer.send('did-cancel-window-unload') diff --git a/src/atom-environment.coffee b/src/atom-environment.coffee index 803e5246a..977f9d70b 100644 --- a/src/atom-environment.coffee +++ b/src/atom-environment.coffee @@ -674,6 +674,10 @@ class AtomEnvironment extends Model @disposables.add(@applicationDelegate.onDidOpenLocations(@openLocations.bind(this))) @disposables.add(@applicationDelegate.onApplicationMenuCommand(@dispatchApplicationMenuCommand.bind(this))) @disposables.add(@applicationDelegate.onContextMenuCommand(@dispatchContextMenuCommand.bind(this))) + @disposables.add @applicationDelegate.onSaveWindowState => + callback = => @applicationDelegate.didSaveWindowState() + @saveState({isUnloading: true}).catch(callback).then(callback) + @listenForUpdates() @registerDefaultTargetForKeymaps() @@ -713,7 +717,6 @@ class AtomEnvironment extends Model unloadEditorWindow: -> return if not @project - @saveState({isUnloading: true}) @storeWindowBackground() @packages.deactivatePackages() @saveBlobStoreSync() @@ -851,18 +854,17 @@ class AtomEnvironment extends Model @blobStore.save() saveState: (options) -> - return Promise.resolve() unless @enablePersistence - new Promise (resolve, reject) => - return if not @project - - state = @serialize(options) - savePromise = - if storageKey = @getStateKey(@project?.getPaths()) - @stateStore.save(storageKey, state) - else - @applicationDelegate.setTemporaryWindowState(state) - savePromise.catch(reject).then(resolve) + if @enablePersistence and @project + state = @serialize(options) + savePromise = + if storageKey = @getStateKey(@project?.getPaths()) + @stateStore.save(storageKey, state) + else + @applicationDelegate.setTemporaryWindowState(state) + savePromise.catch(reject).then(resolve) + else + resolve() loadState: -> if @enablePersistence diff --git a/src/main-process/atom-application.coffee b/src/main-process/atom-application.coffee index 0eebf043f..701710337 100644 --- a/src/main-process/atom-application.coffee +++ b/src/main-process/atom-application.coffee @@ -96,11 +96,10 @@ class AtomApplication @launch(options) destroy: -> - @disposable.dispose() windowsClosePromises = @windows.map (window) -> window.close() window.closedPromise - Promise.all(windowsClosePromises) + Promise.all(windowsClosePromises).then(=> @disposable.dispose()) launch: (options) -> if options.pathsToOpen?.length > 0 or options.urlsToOpen?.length > 0 or options.test @@ -233,8 +232,11 @@ class AtomApplication @openPathOnEvent('application:open-your-stylesheet', 'atom://.atom/stylesheet') @openPathOnEvent('application:open-license', path.join(process.resourcesPath, 'LICENSE.md')) - @disposable.add ipcHelpers.on app, 'before-quit', => - @quitting = true + @disposable.add ipcHelpers.on app, 'before-quit', (event) => + unless @quitting + event.preventDefault() + @quitting = true + Promise.all(@windows.map((window) -> window.saveState())).then(-> app.quit()) @disposable.add ipcHelpers.on app, 'will-quit', => @killAllProcesses() @@ -322,6 +324,8 @@ class AtomApplication @disposable.add ipcHelpers.on ipcMain, 'did-cancel-window-unload', => @quitting = false + for window in @windows + window.didCancelWindowUnload() clipboard = require '../safe-clipboard' @disposable.add ipcHelpers.on ipcMain, 'write-text-to-selection-clipboard', (event, selectedText) -> diff --git a/src/main-process/atom-window.coffee b/src/main-process/atom-window.coffee index effef48ab..b0490f940 100644 --- a/src/main-process/atom-window.coffee +++ b/src/main-process/atom-window.coffee @@ -1,4 +1,4 @@ -{BrowserWindow, app, dialog} = require 'electron' +{BrowserWindow, app, dialog, ipcMain} = require 'electron' path = require 'path' fs = require 'fs' url = require 'url' @@ -128,8 +128,12 @@ class AtomWindow false handleEvents: -> - @browserWindow.on 'close', => - @atomApplication.saveState(false) + @browserWindow.on 'close', (event) => + unless @atomApplication.quitting or @unloading + event.preventDefault() + @unloading = true + @atomApplication.saveState(false) + @saveState().then(=> @close()) @browserWindow.on 'closed', => @fileRecoveryService.didCloseWindow(this) @@ -170,6 +174,18 @@ class AtomWindow @browserWindow.on 'blur', => @browserWindow.focusOnWebView() + didCancelWindowUnload: -> + @unloading = false + + saveState: -> + new Promise (resolve) => + callback = (event) => + if BrowserWindow.fromWebContents(event.sender) is @browserWindow + ipcMain.removeListener('did-save-window-state', callback) + resolve() + ipcMain.on('did-save-window-state', callback) + @browserWindow.webContents.send('save-window-state') + openPath: (pathToOpen, initialLine, initialColumn) -> @openLocations([{pathToOpen, initialLine, initialColumn}]) From 34a99f9c829138a19cc192df9255f37743689451 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 7 Sep 2016 14:18:52 +0200 Subject: [PATCH 2/3] Write tests to ensure quitting the application works as expected --- spec/main-process/atom-application.test.js | 35 +++++++++++++++++++++- src/main-process/atom-window.coffee | 3 +- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/spec/main-process/atom-application.test.js b/spec/main-process/atom-application.test.js index 0c3218a99..408ea84ff 100644 --- a/spec/main-process/atom-application.test.js +++ b/spec/main-process/atom-application.test.js @@ -14,9 +14,10 @@ const ATOM_RESOURCE_PATH = path.resolve(__dirname, '..', '..') describe('AtomApplication', function () { this.timeout(60 * 1000) - let originalAtomHome, atomApplicationsToDestroy + let originalAppQuit, originalAtomHome, atomApplicationsToDestroy beforeEach(function () { + originalAppQuit = electron.app.quit originalAtomHome = process.env.ATOM_HOME process.env.ATOM_HOME = makeTempDir('atom-home') // Symlinking the compile cache into the temporary home dir makes the windows load much faster @@ -31,6 +32,7 @@ describe('AtomApplication', function () { }) afterEach(async function () { + electron.app.quit = originalAppQuit process.env.ATOM_HOME = originalAtomHome for (let atomApplication of atomApplicationsToDestroy) { await atomApplication.destroy() @@ -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 () { + mockElectronAppQuit() + const dirAPath = makeTempDir("a") + const dirBPath = makeTempDir("b") + const atomApplication = buildAtomApplication() + const window1 = atomApplication.launch(parseCommandLine([path.join(dirAPath, 'file-a')])) + await focusWindow(window1) + const window2 = atomApplication.launch(parseCommandLine([path.join(dirBPath, 'file-b')])) + await focusWindow(window2) + electron.app.quit() + assert(!electron.app.hasQuitted()) + await Promise.all([window1.lastSaveStatePromise, window2.lastSaveStatePromise]) + assert(electron.app.hasQuitted()) + }) + }) + function buildAtomApplication () { const atomApplication = new AtomApplication({ resourcePath: ATOM_RESOURCE_PATH, @@ -383,6 +402,20 @@ describe('AtomApplication', function () { await conditionPromise(() => window.atomApplication.lastFocusedWindow === window) } + function mockElectronAppQuit () { + let quitted = false + electron.app.quit = function () { + let shouldQuit = true + electron.app.emit('before-quit', {preventDefault: () => { shouldQuit = false }}) + if (shouldQuit) { + quitted = true + } + } + electron.app.hasQuitted = function () { + return quitted + } + } + function makeTempDir (name) { return fs.realpathSync(require('temp').mkdirSync(name)) } diff --git a/src/main-process/atom-window.coffee b/src/main-process/atom-window.coffee index b0490f940..236fbf8e0 100644 --- a/src/main-process/atom-window.coffee +++ b/src/main-process/atom-window.coffee @@ -178,13 +178,14 @@ class AtomWindow @unloading = false saveState: -> - new Promise (resolve) => + @lastSaveStatePromise = new Promise (resolve) => callback = (event) => if BrowserWindow.fromWebContents(event.sender) is @browserWindow ipcMain.removeListener('did-save-window-state', callback) resolve() ipcMain.on('did-save-window-state', callback) @browserWindow.webContents.send('save-window-state') + @lastSaveStatePromise openPath: (pathToOpen, initialLine, initialColumn) -> @openLocations([{pathToOpen, initialLine, initialColumn}]) From e6c15ee10b6b81e364c210448a134ee60041f4be Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 7 Sep 2016 15:47:23 +0200 Subject: [PATCH 3/3] `onSaveWindowState` -> `onSaveWindowStateRequest` --- src/application-delegate.coffee | 2 +- src/atom-environment.coffee | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/application-delegate.coffee b/src/application-delegate.coffee index f2979ba0d..18fb59f54 100644 --- a/src/application-delegate.coffee +++ b/src/application-delegate.coffee @@ -244,7 +244,7 @@ class ApplicationDelegate new Disposable -> ipcRenderer.removeListener('context-command', outerCallback) - onSaveWindowState: (callback) -> + onSaveWindowStateRequest: (callback) -> outerCallback = (event, message) -> callback(event) diff --git a/src/atom-environment.coffee b/src/atom-environment.coffee index 977f9d70b..f84e90469 100644 --- a/src/atom-environment.coffee +++ b/src/atom-environment.coffee @@ -674,7 +674,7 @@ class AtomEnvironment extends Model @disposables.add(@applicationDelegate.onDidOpenLocations(@openLocations.bind(this))) @disposables.add(@applicationDelegate.onApplicationMenuCommand(@dispatchApplicationMenuCommand.bind(this))) @disposables.add(@applicationDelegate.onContextMenuCommand(@dispatchContextMenuCommand.bind(this))) - @disposables.add @applicationDelegate.onSaveWindowState => + @disposables.add @applicationDelegate.onSaveWindowStateRequest => callback = => @applicationDelegate.didSaveWindowState() @saveState({isUnloading: true}).catch(callback).then(callback)