Avoid serialization race condition with slow package deactivation

If any package takes longer than one second (the
`saveStateDebounceInterval`) to deactivate, and the unload was triggered
by a key or mouse down, mouse event, you can end up in a situation where
sate is serialized _after_ the packages are deactivated.

The result in a bug where panes, such as the File Tree, will randomly
be closed when you reload or reopen Atom.

This can be reproduced by creating a package that has an artificially
slow `deactivate` method. With such a package enabled, every reload ends
up serializing a state where all panes are closed.

I'm a bit nervous about this exact fix, since we have to track every
place where it's possible for `prepare-to-unload` to be fired, without
the window actually closing.

I handled the only instance I saw, but the logic is complex enough, that
I'm not 100% confident there are not other instances.

If it did happen that `prepare-to-unload` was fired and some other logic
caused the window to not actually close, we could end up in a state
where mousedown/keydown events were no longer causing state to get
serialized.
This commit is contained in:
Jordan Eldredge
2018-08-23 13:27:42 -07:00
parent 9aa50b9cd4
commit 9895badff8
4 changed files with 22 additions and 5 deletions

View File

@@ -325,6 +325,13 @@ class ApplicationDelegate {
return new Disposable(() => ipcRenderer.removeListener('prepare-to-unload', outerCallback))
}
onUnloadAborted (callback) {
const outerCallback = (event, message) => callback(event)
ipcRenderer.on('unload-aborted', outerCallback)
return new Disposable(() => ipcRenderer.removeListener('unload-aborted', outerCallback))
}
onDidChangeHistoryManager (callback) {
const outerCallback = (event, message) => callback(event)

View File

@@ -64,7 +64,7 @@ class AtomEnvironment {
this.applicationDelegate = params.applicationDelegate
this.nextProxyRequestId = 0
this.unloaded = false
this.unloading = false
this.loadTime = null
this.emitter = new Emitter()
this.disposables = new CompositeDisposable()
@@ -280,7 +280,7 @@ class AtomEnvironment {
attachSaveStateListeners () {
const saveState = _.debounce(() => {
this.window.requestIdleCallback(() => {
if (!this.unloaded) this.saveState({isUnloading: false})
if (!this.unloading) this.saveState({isUnloading: false})
})
}, this.saveStateDebounceInterval)
this.document.addEventListener('mousedown', saveState, true)
@@ -775,7 +775,7 @@ class AtomEnvironment {
await this.stateStore.clear()
}
this.unloaded = false
this.unloading = false
const updateProcessEnvPromise = this.updateProcessEnvAndTriggerHooks()
@@ -812,9 +812,15 @@ class AtomEnvironment {
projectHasPaths: this.project.getPaths().length > 0
})
if (closing) await this.packages.deactivatePackages()
if (closing) {
this.unloading = true;
await this.packages.deactivatePackages()
}
return closing
}))
this.disposables.add(this.applicationDelegate.onUnloadAborted(() => {
this.unloading = false
}));
this.listenForUpdates()
@@ -898,7 +904,6 @@ class AtomEnvironment {
this.storeWindowBackground()
this.saveBlobStoreSync()
this.unloaded = true
}
saveBlobStoreSync () {

View File

@@ -442,6 +442,7 @@ class AtomApplication extends EventEmitter {
if (windowUnloadedResults.every(Boolean)) {
app.quit()
} else {
this.getAllWindows().forEach(window => window.unloadAborted())
this.quitting = false
}
}

View File

@@ -245,6 +245,10 @@ class AtomWindow extends EventEmitter {
return this.lastPrepareToUnloadPromise
}
unloadAborted () {
this.browserWindow.webContents.send('unload-aborted')
}
openPath (pathToOpen, initialLine, initialColumn) {
return this.openLocations([{pathToOpen, initialLine, initialColumn}])
}