diff --git a/spec/atom-environment-spec.js b/spec/atom-environment-spec.js index d2ae0cfba3..0e6adc584f 100644 --- a/spec/atom-environment-spec.js +++ b/spec/atom-environment-spec.js @@ -213,29 +213,18 @@ describe('AtomEnvironment', () => { atom.enablePersistence = false; }); - it('selects the state based on the current project paths', async () => { + it('computes the state key based on the current project paths', async () => { jasmine.useRealClock(); const [dir1, dir2] = [temp.mkdirSync('dir1-'), temp.mkdirSync('dir2-')]; - const loadSettings = Object.assign(atom.getLoadSettings(), { - initialProjectRoots: [dir1], - windowState: null - }); - - spyOn(atom, 'getLoadSettings').and.callFake(() => loadSettings); - spyOn(atom, 'serialize').and.returnValue({ stuff: 'cool' }); - - atom.project.setPaths([dir1, dir2]); - - // State persistence will fail if other Atom instances are running - expect(await atom.stateStore.connect()).toBe(true); - - await atom.saveState(); - expect(await atom.loadState()).toBeFalsy(); - - loadSettings.initialProjectRoots = [dir2, dir1]; - expect(await atom.loadState()).toEqual({ stuff: 'cool' }); + let stateKey = await atom.getStateKey([dir1, dir2]); + expect(stateKey).toEqual( + await atom.getStateKey([dir1, dir2], { pathsOnly: true }) + ); + expect(stateKey).toEqual( + await atom.getStateKey([dir2, dir1], { pathsOnly: true }) + ); }); it('saves state when the CPU is idle after a keydown or mousedown event', async () => { diff --git a/spec/main-process/atom-application.test.js b/spec/main-process/atom-application.test.js index b67a0b9038..cac4a223f9 100644 --- a/spec/main-process/atom-application.test.js +++ b/spec/main-process/atom-application.test.js @@ -9,6 +9,7 @@ const sandbox = require('sinon').createSandbox(); const AtomApplication = require('../../src/main-process/atom-application'); const parseCommandLine = require('../../src/main-process/parse-command-line'); +const { releaseStateKey } = require('../../src/main-process/state-keys'); const { emitterEventPromise, conditionPromise @@ -1064,12 +1065,16 @@ describe('AtomApplication', function() { .storageFolder.store.calledWith('application.json', { version: '1', windows: [ - { projectRoots: [scenario.convertRootPath('a')] }, + { + projectRoots: [scenario.convertRootPath('a')], + stateKey: sinon.match(/^editor-/) + }, { projectRoots: [ scenario.convertRootPath('b'), scenario.convertRootPath('c') - ] + ], + stateKey: sinon.match(/^editor-/) } ] }) @@ -1092,7 +1097,12 @@ describe('AtomApplication', function() { .getApplication(0) .storageFolder.store.calledWith('application.json', { version: '1', - windows: [{ projectRoots: [scenario.convertRootPath('a')] }] + windows: [ + { + projectRoots: [scenario.convertRootPath('a')], + stateKey: sinon.match(/^editor-/) + } + ] }) ); }); @@ -1106,6 +1116,102 @@ describe('AtomApplication', function() { w.browserWindow.emit('blur'); await promise; }); + + it('assigns unique state keys to two windows sharing the same project root', async function() { + await scenario.launch(parseCommandLine(['a'])); + + const statePromise = emitterEventPromise( + scenario.getApplication(0), + 'application:did-save-state' + ); + await scenario.open(parseCommandLine(['--new-window', 'a'])); + await statePromise; + + const { windows } = scenario.getApplication(0).storageFolder.store.lastCall.args[1]; + assert.lengthOf(windows, 2); + assert.isDefined(windows[0].stateKey); + assert.isDefined(windows[1].stateKey); + assert.notStrictEqual(windows[0].stateKey, windows[1].stateKey); + }); + + it('preserves a window\'s state key across sessions', async function() { + // First session: capture the saved stateKey + const [w] = await scenario.launch(parseCommandLine(['a'])); + + const statePromise1 = emitterEventPromise( + scenario.getApplication(0), + 'application:did-save-state' + ); + w.browserWindow.emit('blur'); + await statePromise1; + + const { stateKey: savedStateKey } = scenario + .getApplication(0) + .storageFolder.store.lastCall.args[1].windows[0]; + assert.isDefined(savedStateKey); + await scenario.destroy(); + + // Second session: restore with the saved stateKey + const app2 = scenario.addApplication({ + applicationJson: { + version: '1', + windows: [{ projectRoots: [scenario.convertRootPath('a')], stateKey: savedStateKey }] + } + }); + app2.config.set('core.restorePreviousWindowsOnStart', 'always'); + const [w2] = await scenario.launch({ app: app2 }); + + const statePromise2 = emitterEventPromise(app2, 'application:did-save-state'); + w2.browserWindow.emit('blur'); + await statePromise2; + + const { stateKey: restoredStateKey } = app2.storageFolder.store.lastCall.args[1].windows[0]; + assert.strictEqual(restoredStateKey, savedStateKey); + }); + + it('assigns a hash-based state key when restoring a window saved without one', async function() { + // Simulate a session saved by an older version of Pulsar (no `stateKey` + // field). + const app = scenario.addApplication({ + applicationJson: { + version: '1', + windows: [{ projectRoots: [scenario.convertRootPath('a')] }] + } + }); + app.config.set('core.restorePreviousWindowsOnStart', 'always'); + const [w] = await scenario.launch({ app }); + + const statePromise = emitterEventPromise(app, 'application:did-save-state'); + w.browserWindow.emit('blur'); + await statePromise; + + const { stateKey } = app.storageFolder.store.lastCall.args[1].windows[0]; + assert.match(stateKey, /^editor-[0-9a-f]{40}$/); + }); + + it('resolves a state key collision when two windows were saved with the same legacy key', async function() { + const legacyKey = 'editor-' + 'a'.repeat(40); + const app = scenario.addApplication({ + applicationJson: { + version: '1', + windows: [ + { projectRoots: [scenario.convertRootPath('a')], stateKey: legacyKey }, + { projectRoots: [scenario.convertRootPath('b')], stateKey: legacyKey } + ] + } + }); + app.config.set('core.restorePreviousWindowsOnStart', 'always'); + await scenario.launch({ app }); + + const statePromise = emitterEventPromise(app, 'application:did-save-state'); + app.getAllWindows()[0].browserWindow.emit('blur'); + await statePromise; + + const { windows } = app.storageFolder.store.lastCall.args[1]; + assert.lengthOf(windows, 2); + assert.notStrictEqual(windows[0].stateKey, windows[1].stateKey); + }); + }); describe('when closing the last window', function() { @@ -1613,6 +1719,12 @@ class LaunchScenario { } async destroy() { + // app.destroy() closes windows without going through removeWindow(), so + // releaseStateKey() is never called. Do it explicitly here to prevent + // USED_KEYS from leaking between tests. + for (const window of this.windows) { + releaseStateKey(window); + } await Promise.all(Array.from(this.applications, app => app.destroy())); if (this.originalAtomHome) { @@ -1635,6 +1747,8 @@ class LaunchScenario { this.windows.add(newWindow); return newWindow; }); + this.sinon.stub(app, 'resolveTestRunnerPath').returns('/stubbed/test-runner'); + this.sinon.stub(app, 'resolveLegacyTestRunnerPath').returns('/stubbed/legacy-test-runner'); this.sinon .stub(app.storageFolder, 'load') .callsFake(() => diff --git a/spec/main-process/state-keys.test.js b/spec/main-process/state-keys.test.js new file mode 100644 index 0000000000..26e4f822fc --- /dev/null +++ b/spec/main-process/state-keys.test.js @@ -0,0 +1,173 @@ +/* globals assert */ + +const { getStateKey, reserveStateKey, releaseStateKey, resetStateKeys } = require('../../src/main-process/state-keys'); + +// Create a minimal stand-in for an AtomWindow. State keys only care about +// object identity, so a plain object is sufficient. +function makeWindow() { + return {}; +} + +describe('state-keys', function() { + describe('getStateKey', function() { + afterEach(() => resetStateKeys()); + + it('returns a key with the editor- prefix', function() { + const win = makeWindow(); + const key = getStateKey(win, ['/some/path']); + assert.match(key, /^editor-/); + }); + + it('returns a hash-based key for a given set of paths', function() { + const win = makeWindow(); + const key = getStateKey(win, ['/some/path']); + assert.match(key, /^editor-[0-9a-f]{40}$/); + }); + + it('returns the same key on repeated calls for the same window', function() { + const win = makeWindow(); + const key1 = getStateKey(win, ['/some/path']); + const key2 = getStateKey(win, ['/some/path']); + assert.strictEqual(key1, key2); + }); + + it('returns the same key regardless of path order (when pathsOnly is true)', function() { + const win1 = makeWindow(); + const win2 = makeWindow(); + // Since we're passing `pathsOnly: true`, `getStateKey` becomes a pure + // function that computes state key from paths and nothing else. When + // `pathsOnly` is `false` (as it is by default), `getStateKey` would + // notice the collision and assign `key2` a randomly generated key to + // avoid it. + const key1 = getStateKey(win1, ['/path/a', '/path/b'], { pathsOnly: true }); + const key2 = getStateKey(win2, ['/path/b', '/path/a'], { pathsOnly: true }); + assert.strictEqual(key1, key2); + }); + + it('returns different keys for windows with different paths', function() { + const win1 = makeWindow(); + const win2 = makeWindow(); + const key1 = getStateKey(win1, ['/path/a']); + const key2 = getStateKey(win2, ['/path/b']); + assert.notStrictEqual(key1, key2); + }); + + it('assigns a random key when the ideal hash key is already taken', function() { + const win1 = makeWindow(); + const win2 = makeWindow(); + const key1 = getStateKey(win1, ['/path/a']); + const key2 = getStateKey(win2, ['/path/a']); + assert.notStrictEqual(key1, key2); + // The second key should be a UUID-based fallback, not a 40-char hash + assert.match(key2, /^editor-[0-9a-f]{8}-[0-9a-f]{4}-/); + }); + + it('allows the ideal key to be reused after its window is released', function() { + const win1 = makeWindow(); + const key1 = getStateKey(win1, ['/path/a']); + releaseStateKey(win1); + + const win2 = makeWindow(); + const key2 = getStateKey(win2, ['/path/a']); + assert.strictEqual(key1, key2); + }); + + describe('with pathsOnly: true', function() { + it('always returns the hash-based key, ignoring what the window is registered as', function() { + const win = makeWindow(); + // Register the window under the hash key for /path/a + getStateKey(win, ['/path/a']); + + // Now a second window claims /path/a and takes the hash key... + const win2 = makeWindow(); + getStateKey(win2, ['/path/a']); + // ...so win gets a UUID. A pathsOnly call should still return the hash. + + const idealKey = getStateKey(win, ['/path/a'], { pathsOnly: true }); + assert.match(idealKey, /^editor-[0-9a-f]{40}$/); + }); + + it('does not register any key for the window', function() { + const freshWin = makeWindow(); + getStateKey(freshWin, ['/path/z'], { pathsOnly: true }); + + // The window should have no registered key; a subsequent normal call + // should go through the full assignment path, not return a cached value. + const win2 = makeWindow(); + const hashKey = getStateKey(win2, ['/path/z']); + + // If pathsOnly had registered a key for freshWin and claimed the hash, + // win2 would get a UUID. It should get the hash. + assert.match(hashKey, /^editor-[0-9a-f]{40}$/); + }); + }); + }); + + describe('reserveStateKey', function() { + it('pre-registers a key so getStateKey returns it immediately', function() { + const savedKey = 'editor-' + 'a'.repeat(40); + const win = makeWindow(); + reserveStateKey(win, savedKey); + + const key = getStateKey(win, ['/any/path']); + assert.strictEqual(key, savedKey); + }); + + it('blocks other windows from claiming the same key', function() { + const savedKey = 'editor-' + 'a'.repeat(40); + const win1 = makeWindow(); + const win2 = makeWindow(); + + reserveStateKey(win1, savedKey); + // win2's ideal key would also be savedKey if the paths matched, but + // since we can't trivially craft that, we test via a second reserveStateKey. + reserveStateKey(win2, savedKey); + const key2actual = getStateKey(win2, []); + // win2 should have a random key since savedKey was taken + assert.notStrictEqual(key2actual, savedKey); + }); + + it('assigns a random key when the requested key is already taken by another window', function() { + const savedKey = 'editor-' + 'b'.repeat(40); + const win1 = makeWindow(); + const win2 = makeWindow(); + + reserveStateKey(win1, savedKey); + reserveStateKey(win2, savedKey); + + const key1 = getStateKey(win1, []); + const key2 = getStateKey(win2, []); + assert.strictEqual(key1, savedKey); + assert.notStrictEqual(key2, savedKey); + assert.match(key2, /^editor-[0-9a-f]{8}-[0-9a-f]{4}-/); + }); + + it('is idempotent when called twice with the same window and key', function() { + const savedKey = 'editor-' + 'c'.repeat(40); + const win = makeWindow(); + + reserveStateKey(win, savedKey); + // should not throw or reassign + reserveStateKey(win, savedKey); + + assert.strictEqual(getStateKey(win, []), savedKey); + }); + }); + + describe('releaseStateKey', function() { + it('allows the released key to be claimed by a new window', function() { + const win1 = makeWindow(); + const key1 = getStateKey(win1, ['/path/a']); + releaseStateKey(win1); + + const win2 = makeWindow(); + const key2 = getStateKey(win2, ['/path/a']); + assert.strictEqual(key2, key1); + }); + + it('is a no-op for a window that was never assigned a key', function() { + const win = makeWindow(); + assert.doesNotThrow(() => releaseStateKey(win)); + }); + }); +}); diff --git a/src/application-delegate.js b/src/application-delegate.js index 21c125932a..6a9b0a4282 100644 --- a/src/application-delegate.js +++ b/src/application-delegate.js @@ -10,10 +10,14 @@ module.exports = class ApplicationDelegate { this._ipcMessageEmitter = null; } + async getStateKey (projectPaths, options) { + return await ipcHelpers.call('get-state-key', projectPaths, options); + } + ipcMessageEmitter() { if (!this._ipcMessageEmitter) { this._ipcMessageEmitter = new Emitter(); - ipcRenderer.on('message', (event, message, detail) => { + ipcRenderer.on('message', (_, message, detail) => { this._ipcMessageEmitter.emit(message, detail); }); } @@ -34,7 +38,7 @@ module.exports = class ApplicationDelegate { pickFolder(callback) { const responseChannel = 'atom-pick-folder-response'; - ipcRenderer.on(responseChannel, function (event, path) { + ipcRenderer.on(responseChannel, function (_, path) { ipcRenderer.removeAllListeners(responseChannel); return callback(path); }); diff --git a/src/atom-environment.js b/src/atom-environment.js index a9084b3c57..6617d02eae 100644 --- a/src/atom-environment.js +++ b/src/atom-environment.js @@ -1385,7 +1385,10 @@ class AtomEnvironment { } async addToProject(projectPaths) { - const state = await this.loadState(this.getStateKey(projectPaths)); + let stateKeyForNewProjectPaths = await this.getStateKey(projectPaths, { pathsOnly: true }); + const state = await this.loadState(stateKeyForNewProjectPaths); + // When this is an “empty” project, adding one or more paths could lead the + // window to offer to “adopt” any state that existed for those paths. if (state && this.project.getPaths().length === 0) { this.attemptRestoreProjectStateForPaths(state, projectPaths); } else { @@ -1474,7 +1477,7 @@ or use Pane::saveItemAs for programmatic saving.`); if (this.enablePersistence && this.project) { const state = this.serialize(options); if (!storageKey) - storageKey = this.getStateKey(this.project && this.project.getPaths()); + storageKey = await this.getStateKey(this.project && this.project.getPaths()); if (storageKey) { await this.stateStore.save(storageKey, state); } else { @@ -1483,17 +1486,17 @@ or use Pane::saveItemAs for programmatic saving.`); } } - loadState(stateKey) { + async loadState(stateKey) { if (this.enablePersistence) { if (!stateKey) - stateKey = this.getStateKey(this.getLoadSettings().initialProjectRoots); + stateKey = await this.getStateKey(this.getLoadSettings().initialProjectRoots); if (stateKey) { - return this.stateStore.load(stateKey); + return await this.stateStore.load(stateKey); } else { - return this.applicationDelegate.getTemporaryWindowState(); + return await this.applicationDelegate.getTemporaryWindowState(); } } else { - return Promise.resolve(null); + return null; } } @@ -1560,21 +1563,8 @@ or use Pane::saveItemAs for programmatic saving.`); } } - getStateKey(paths) { - if (paths && paths.length > 0) { - const sha1 = crypto - .createHash('sha1') - .update( - paths - .slice() - .sort() - .join('\n') - ) - .digest('hex'); - return `editor-${sha1}`; - } else { - return null; - } + async getStateKey (paths, options = {}) { + return this.applicationDelegate.getStateKey(paths, options); } getConfigDirPath() { @@ -1710,7 +1700,7 @@ or use Pane::saveItemAs for programmatic saving.`); missingFolders.map(location => location.pathToOpen) ); const state = await this.loadState( - this.getStateKey(Array.from(foldersForStateKey)) + await this.getStateKey(Array.from(foldersForStateKey), { pathsOnly: true }) ); // only restore state if this is the first path added to the project diff --git a/src/main-process/atom-application.js b/src/main-process/atom-application.js index 0652659c50..2da96585f0 100644 --- a/src/main-process/atom-application.js +++ b/src/main-process/atom-application.js @@ -2,6 +2,7 @@ const AtomWindow = require('./atom-window'); const ApplicationMenu = require('./application-menu'); const AtomProtocolHandler = require('./atom-protocol-handler'); const { onDidChangeScrollbarStyle, getScrollbarStyle } = require('./scrollbar-style'); +const { getStateKey, releaseStateKey, reserveStateKey } = require('./state-keys'); const StorageFolder = require('../storage-folder'); const Config = require('../config'); const ConfigFile = require('../config-file'); @@ -340,6 +341,7 @@ module.exports = class AtomApplication extends EventEmitter { openWithOptions(options) { const { pathsToOpen, + stateKey, executedFrom, foldersToOpen, urlsToOpen, @@ -363,6 +365,7 @@ module.exports = class AtomApplication extends EventEmitter { return this.runTests({ headless: true, devMode, + stateKey, resourcePath: this.resourcePath, executedFrom, pathsToOpen, @@ -395,6 +398,7 @@ module.exports = class AtomApplication extends EventEmitter { clearWindowState, addToLastWindow, preserveFocus, + stateKey, env }); } else if (urlsToOpen && urlsToOpen.length > 0) { @@ -427,6 +431,7 @@ module.exports = class AtomApplication extends EventEmitter { // Public: Removes the {AtomWindow} from the global window list. removeWindow(window) { + releaseStateKey(window); this.windowStack.removeWindow(window); if (this.getAllWindows().length === 0 && process.platform !== 'darwin') { app.quit(); @@ -1006,6 +1011,15 @@ module.exports = class AtomApplication extends EventEmitter { window => window.temporaryState ) ); + this.disposable.add( + ipcHelpers.respondTo( + 'get-state-key', + (browserWindow, projectPaths, options) => { + const win = this.atomWindowForBrowserWindow(browserWindow); + return getStateKey(win, projectPaths, options); + } + ) + ); this.disposable.add( ipcHelpers.respondTo('set-temporary-window-state', (win, state) => { @@ -1268,6 +1282,7 @@ module.exports = class AtomApplication extends EventEmitter { async openPaths({ pathsToOpen, foldersToOpen, + stateKey, executedFrom, pidToKillWhenClosed, newWindow, @@ -1427,6 +1442,9 @@ module.exports = class AtomApplication extends EventEmitter { }); openedWindow.preserveFocus = preserveFocus; this.addWindow(openedWindow); + if (stateKey) { + reserveStateKey(openedWindow, stateKey); + } openedWindow.focus(); } @@ -1505,7 +1523,10 @@ module.exports = class AtomApplication extends EventEmitter { version: APPLICATION_STATE_VERSION, windows: windows .filter(window => !window.isSpec) - .map(window => ({ projectRoots: window.projectRoots })) + .map(window => ({ + projectRoots: window.projectRoots, + stateKey: getStateKey(window, window.projectRoots) + })) }; state.windows.reverse(); @@ -1526,6 +1547,7 @@ module.exports = class AtomApplication extends EventEmitter { // Schema: {version: '1', windows: [{projectRoots: ['', ...]}, ...]} return state.windows.map(each => ({ foldersToOpen: each.projectRoots, + stateKey: each.stateKey, devMode: this.devMode, safeMode: this.safeMode, newWindow: true diff --git a/src/main-process/state-keys.js b/src/main-process/state-keys.js new file mode 100644 index 0000000000..00b017ff68 --- /dev/null +++ b/src/main-process/state-keys.js @@ -0,0 +1,101 @@ +// This file is responsible for managing and assigning "state keys" to windows. +// +// The state key is what we use to serialize this window's state in our state +// store (IndexedDB or SQLite). Ideally, it is produced by a hash of the sorted +// root paths for a given project; this allows us to determine the hypothetical +// state keys for other windows ("is there a state key for a window with paths +// X and Y?") so that we can adopt an orphaned state into an existing empty +// window when appropriate. +// +// But we also need a fallback case for when the ideal state key is already +// taken! There is no requirement for uniqueness of project root sets among +// open windows, so two windows could both be open at once and share the same +// root(s). (This is common, in fact, for users that are used to right-clicking +// in the tree view and selecting the "Open in New Window" option.) +// +// Hence, if the ideal key is taken, we fall back to a randomly generated state +// key. Randomly generated keys will never be used in state adoption, since +// they cannot be derived simply from a set of paths; but that's OK, since it +// implies that there's an existing window that can contibute its state for +// adoption. + +const crypto = require('crypto'); + +const USED_KEYS = new Set(); +let STATE_KEYS_BY_WINDOW = new WeakMap(); + +// Compute the state key used when +function getIdealStateKey (projectPaths) { + if (!Array.isArray(projectPaths) || projectPaths.length === 0) { + return null; + } + let normalizedPathString = projectPaths.slice().sort().join('\n'); + let hash = crypto.createHash('sha1').update(normalizedPathString).digest('hex'); + return `editor-${hash}`; +} + +function getRandomStateKey () { + return `editor-${crypto.randomUUID()}`; +} + +// Given a window and its project paths, retrieves that window's unique key for +// state serialization purposes, creating one if it does not already exist. +function getStateKey (win, projectPaths, { pathsOnly = false } = {}) { + if (pathsOnly) { + // We don't want to know this window's state key; we want to know what the + // state key of a project window _would_ be if it were computed from the + // given project paths. (This is used when one window wants to adopt + // another window's state.) + return getIdealStateKey(projectPaths); + } + + let existingKey = STATE_KEYS_BY_WINDOW.get(win); + if (existingKey) { + return existingKey; + } + + let idealKey = getIdealStateKey(projectPaths); + + let actualKey = idealKey; + if (actualKey === null) return null; + + if (USED_KEYS.has(idealKey)) { + actualKey = getRandomStateKey(); + } + USED_KEYS.add(actualKey); + STATE_KEYS_BY_WINDOW.set(win, actualKey); + return actualKey; +} + +// Reserve a given state key if it's free to reserve. Otherwise will generate a +// new random state key and reserve that instead. +// +// This function is called during startup if there are existing serialized +// windows to restore, since they'll have remembered their state keys from the +// previous session. This lets us skip generating a new one or re-computing the +// old one. +function reserveStateKey (win, requestedStateKey) { + let stateKey = requestedStateKey; + if (USED_KEYS.has(stateKey)) { + // If this window has already reserved this state key, we can bail early. + if (STATE_KEYS_BY_WINDOW.get(win) === stateKey) return; + stateKey = getRandomStateKey(); + } + USED_KEYS.add(stateKey); + STATE_KEYS_BY_WINDOW.set(win, stateKey); +} + +// Unregister a state key for a window. Call this when the window is about to +// be destroyed so that its state key can be reused by a future window. +function releaseStateKey (win) { + let stateKey = STATE_KEYS_BY_WINDOW.get(win); + if (!stateKey) return; + USED_KEYS.delete(stateKey); +} + +function resetStateKeys () { + USED_KEYS.clear(); + STATE_KEYS_BY_WINDOW = new WeakMap(); +} + +module.exports = { getStateKey, releaseStateKey, reserveStateKey, resetStateKeys };