Merge pull request #1501 from afshin/harden-restoration

Harden restoration

src/filebrowser/plugin.ts

@@ -127,22 +127,23 @@ function activate(app: JupyterLab, manager: IServiceManager, documentManager: ID
   };
 
   // Restore the state of the file browser on reload.
-  let key = `${NAMESPACE}:cwd`;
+  const key = `${NAMESPACE}:cwd`;
+  let connect = () => {
+    // Save the subsequent state of the file browser in the state database.
+    fbModel.pathChanged.connect((sender, args) => {
+      state.save(key, { path: args.newValue });
+    });
+  };
   Promise.all([state.fetch(key), app.started, manager.ready]).then(([cwd]) => {
     if (!cwd) {
       return;
     }
-
     let path = cwd['path'] as string;
     return manager.contents.get(path)
-      .then(() => { fbModel.cd(path); })
-      .catch(() => { state.remove(key); });
-  }).then(() => {
-    // Save the subsequent state of the file browser in the state database.
-    fbModel.pathChanged.connect((sender, args) => {
-      state.save(`${NAMESPACE}:cwd`, { path: args.newValue });
-    });
-  });
+      .then(() => fbModel.cd(path))
+      .catch(() => state.remove(key));
+  }).then(connect)
+    .catch(() => state.remove(key).then(connect));
 
   each(registry.creators(), creator => { addCreator(creator.name); });
 

src/instancerestorer/instancerestorer.ts

@@ -273,15 +273,17 @@ class InstanceRestorer implements IInstanceRestorer {
    * calls to `fetch` are guaranteed to return after restoration is complete.
    */
   fetch(): Promise<IInstanceRestorer.ILayout> {
+    const blank: IInstanceRestorer.ILayout = {
+      currentWidget: null,
+      fresh: true,
+      leftArea: { collapsed: true, currentWidget: null, widgets: null },
+      rightArea: { collapsed: true, currentWidget: null, widgets: null }
+    };
     let layout = this._state.fetch(KEY);
+
     return Promise.all([layout, this.restored]).then(([data]) => {
       if (!data) {
-        return {
-          currentWidget: null,
-          fresh: true,
-          leftArea: { collapsed: true, currentWidget: null, widgets: null },
-          rightArea: { collapsed: true, currentWidget: null, widgets: null }
-        };
+        return blank;
       }
 
       let { current, left, right } = data as InstanceRestorer.IDehydratedLayout;
@@ -289,9 +291,9 @@ class InstanceRestorer implements IInstanceRestorer {
       // If any data exists, then this is not a fresh session.
       const fresh = false;
 
-      // Rehydrate main area.
-      const currentWidget = current && this._widgets.has(current) ?
-        this._widgets.get(current) : null;
+      // Rehydrate main area. Coerce type of `current` in case of bad data.
+      const currentWidget = current && this._widgets.has(`${current}`) ?
+        this._widgets.get(`${current}`) : null;
 
       // Rehydrate left area.
       const leftArea = this._rehydrateSideArea(left);
@@ -300,7 +302,7 @@ class InstanceRestorer implements IInstanceRestorer {
       const rightArea = this._rehydrateSideArea(right);
 
       return { currentWidget, fresh, leftArea, rightArea };
-    });
+    }).catch(() => blank); // Let fetch fail gracefully; return blank slate.
   }
 
   /**
@@ -371,6 +373,9 @@ class InstanceRestorer implements IInstanceRestorer {
     return this._state.save(KEY, dehydrated);
   }
 
+  /**
+   * Dehydrate a side area into a serialized description object.
+   */
   private _dehydrateSideArea(area: IInstanceRestorer.ISideArea): InstanceRestorer.ISideArea {
     let dehydrated: InstanceRestorer.ISideArea = { collapsed: area.collapsed };
     if (area.currentWidget) {
@@ -387,6 +392,13 @@ class InstanceRestorer implements IInstanceRestorer {
     return dehydrated;
   }
 
+  /**
+   * Reydrate a serialized side area description object.
+   *
+   * #### Notes
+   * This function consumes data that can become corrupted, so it uses type
+   * coercion to guarantee the dehydrated object is safely processed.
+   */
   private _rehydrateSideArea(area: InstanceRestorer.ISideArea): IInstanceRestorer.ISideArea {
     if (!area) {
       return { collapsed: true, currentWidget: null, widgets: null };
@@ -394,10 +406,11 @@ class InstanceRestorer implements IInstanceRestorer {
     let internal = this._widgets;
     const collapsed = area.hasOwnProperty('collapsed') ? !!area.collapsed
       : false;
-    const currentWidget = area.current && internal.has(area.current) ?
-      internal.get(area.current) : null;
+    const currentWidget = area.current && internal.has(`${area.current}`) ?
+      internal.get(`${area.current}`) : null;
     const widgets = !Array.isArray(area.widgets) ? null
-      : area.widgets.map(name => internal.has(name) ? internal.get(name) : null)
+      : area.widgets
+          .map(name => internal.has(`${name}`) ? internal.get(`${name}`) : null)
           .filter(widget => !!widget);
     return { collapsed, currentWidget, widgets };
   }

src/statedb/statedb.ts

@@ -44,8 +44,9 @@ class StateDB implements IStateDB {
    */
   clear(): Promise<void> {
     const prefix = `${this.namespace}:`;
-    while (window.localStorage.length) {
-      let key = window.localStorage.key(window.localStorage.length - 1);
+    let i = window.localStorage.length;
+    while (i) {
+      let key = window.localStorage.key(--i);
       if (key.indexOf(prefix) === 0) {
         window.localStorage.removeItem(key);
       }
@@ -99,8 +100,9 @@ class StateDB implements IStateDB {
     const prefix = `${this.namespace}:${namespace}:`;
     const regex = new RegExp(`^${this.namespace}\:`);
     let items: IStateItem[] = [];
-    for (let i = 0, len = window.localStorage.length; i < len; i++) {
-      let key = window.localStorage.key(i);
+    let i = window.localStorage.length;
+    while (i) {
+      let key = window.localStorage.key(--i);
       if (key.indexOf(prefix) === 0) {
         try {
           items.push({
@@ -109,6 +111,7 @@ class StateDB implements IStateDB {
           });
         } catch (error) {
           console.warn(error);
+          window.localStorage.removeItem(key);
         }
       }
     }

test/src/index.ts

@@ -79,6 +79,8 @@ import './notebook/tracker.spec';
 
 import './sanitizer/index.spec';
 
+import './statedb/statedb.spec';
+
 import './terminal/terminal.spec';
 
 import './toolbar/toolbar.spec';

test/src/statedb/statedb.spec.ts

@@ -0,0 +1,212 @@
+// Copyright (c) Jupyter Development Team.
+// Distributed under the terms of the Modified BSD License.
+
+import expect = require('expect.js');
+
+import {
+  StateDB
+} from '../../../lib/statedb/statedb';
+
+
+describe('StateDB', () => {
+
+  describe('#constructor()', () => {
+
+    it('should create a state database', () => {
+      let db = new StateDB({ namespace: 'test' });
+      expect(db).to.be.a(StateDB);
+    });
+
+  });
+
+  describe('#maxLength', () => {
+
+    it('should enforce the maximum length of a stored item', done => {
+      let db = new StateDB({ namespace: 'test' });
+      let key = 'test-key';
+      let data = { a: (new Array<string>(db.maxLength)).join('A') };
+      db.save(key, data)
+        .then(() => { done('maxLength promise should have rejected'); })
+        .catch(() => { done(); });
+    });
+
+  });
+
+  describe('#namespace', () => {
+
+    it('should be the read-only internal namespace', () => {
+      let namespace = 'test-namespace';
+      let db = new StateDB({ namespace });
+      expect(db.namespace).to.be(namespace);
+    });
+
+  });
+
+  describe('#clear()', () => {
+
+    it('should empty the items in a state database', done => {
+      let { localStorage } = window;
+      localStorage.clear();
+
+      let db = new StateDB({ namespace: 'test-namespace' });
+      let key = 'foo:bar';
+      let value = { qux: 'quux' };
+
+      expect(localStorage.length).to.be(0);
+      db.save(key, value)
+        .then(() => { expect(localStorage).to.have.length(1); })
+        .then(() => db.clear())
+        .then(() => { expect(localStorage).to.be.empty(); })
+        .then(done)
+        .catch(done);
+    });
+
+    it('should only clear its own namespace', done => {
+      let { localStorage } = window;
+      localStorage.clear();
+
+      let db1 = new StateDB({ namespace: 'test-namespace-1' });
+      let db2 = new StateDB({ namespace: 'test-namespace-2' });
+
+      expect(localStorage.length).to.be(0);
+      db1.save('foo', { bar: null })
+        .then(() => { expect(localStorage).to.have.length(1); })
+        .then(() => db2.save('baz', { qux: null }))
+        .then(() => { expect(localStorage).to.have.length(2); })
+        .then(() => db1.clear())
+        .then(() => { expect(localStorage).to.have.length(1); })
+        .then(() => db2.clear())
+        .then(() => { expect(localStorage).to.be.empty(); })
+        .then(done)
+        .catch(done);
+    });
+
+  });
+
+  describe('#fetch()', () => {
+
+    it('should fetch a stored key', done => {
+      let { localStorage } = window;
+      localStorage.clear();
+
+      let db = new StateDB({ namespace: 'test-namespace' });
+      let key = 'foo:bar';
+      let value = { baz: 'qux' };
+
+      expect(localStorage.length).to.be(0);
+      db.save(key, value)
+        .then(() => { expect(localStorage).to.have.length(1); })
+        .then(() => db.fetch(key))
+        .then(fetched => { expect(fetched).to.eql(value); })
+        .then(() => db.clear())
+        .then(done)
+        .catch(done);
+    });
+
+    it('should resolve a nonexistent key fetch with null', done => {
+      let { localStorage } = window;
+      localStorage.clear();
+
+      let db = new StateDB({ namespace: 'test-namespace' });
+      let key = 'foo:bar';
+
+      expect(localStorage.length).to.be(0);
+      db.fetch(key)
+        .then(fetched => { expect(fetched).to.be(null); })
+        .then(done)
+        .catch(done);
+    });
+
+  });
+
+  describe('#fetchNamespace()', () => {
+
+    it('should fetch a stored namespace', done => {
+      let { localStorage } = window;
+      localStorage.clear();
+
+      let db = new StateDB({ namespace: 'test-namespace' });
+      let keys = [
+        'foo:bar',
+        'foo:baz',
+        'foo:qux',
+        'abc:def',
+        'abc:ghi',
+        'abc:jkl'
+      ];
+
+      expect(localStorage.length).to.be(0);
+      let promises = keys.map(key => db.save(key, { value: key }));
+      Promise.all(promises)
+        .then(() => { expect(localStorage).to.have.length(keys.length); })
+        .then(() => db.fetchNamespace('foo'))
+        .then(fetched => {
+          expect(fetched.length).to.be(3);
+
+          let sorted = fetched.sort((a, b) => a.id.localeCompare(b.id));
+
+          expect(sorted[0].id).to.be(keys[0]);
+          expect(sorted[1].id).to.be(keys[1]);
+          expect(sorted[2].id).to.be(keys[2]);
+        })
+        .then(() => db.fetchNamespace('abc'))
+        .then(fetched => {
+          expect(fetched.length).to.be(3);
+
+          let sorted = fetched.sort((a, b) => a.id.localeCompare(b.id));
+
+          expect(sorted[0].id).to.be(keys[3]);
+          expect(sorted[1].id).to.be(keys[4]);
+          expect(sorted[2].id).to.be(keys[5]);
+        })
+        .then(() => db.clear())
+        .then(done)
+        .catch(done);
+    });
+
+  });
+
+  describe('#remove()', () => {
+
+    it('should remove a stored key', done => {
+      let { localStorage } = window;
+      localStorage.clear();
+
+      let db = new StateDB({ namespace: 'test-namespace' });
+      let key = 'foo:bar';
+      let value = { baz: 'qux' };
+
+      expect(localStorage.length).to.be(0);
+      db.save(key, value)
+        .then(() => { expect(localStorage).to.have.length(1); })
+        .then(() => db.remove(key))
+        .then(() => { expect(localStorage).to.be.empty(); })
+        .then(done)
+        .catch(done);
+    });
+
+  });
+
+  describe('#save()', () => {
+
+    it('should save a key and a value', done => {
+      let { localStorage } = window;
+      localStorage.clear();
+
+      let db = new StateDB({ namespace: 'test-namespace' });
+      let key = 'foo:bar';
+      let value = { baz: 'qux' };
+
+      expect(localStorage.length).to.be(0);
+      db.save(key, value)
+        .then(() => db.fetch(key))
+        .then(fetched => { expect(fetched).to.eql(value); })
+        .then(() => db.remove(key))
+        .then(() => { expect(localStorage).to.be.empty(); })
+        .then(done)
+        .catch(done);
+    });
+
+  });
+
+});