Ensure uniqueness of window state keys

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 () => {

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(() =>

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));
+    });
+  });
+});