TabRecency: disallow operations until state is loaded from storage

This approach eliminates edge cases and race conditions, and is easier to follow.

Author: philc

background_scripts/completion.js

@@ -479,6 +479,7 @@ class DomainCompleter {
 // If the query is empty, then return a list of open tabs, sorted by recency.
 class TabCompleter {
   async filter({ queryTerms }) {
+    await BgUtils.tabRecency.init();
     // We search all tabs, not just those in the current window.
     const tabs = await chrome.tabs.query({});
     const results = tabs.filter((tab) => RankingUtils.matches(queryTerms, tab.url, tab.title));

background_scripts/main.js

@@ -316,7 +316,8 @@ const BackgroundCommands = {
     await removeTabsRelative("both", request);
   },
 
-  visitPreviousTab({ count, tab }) {
+  async visitPreviousTab({ count, tab }) {
+    await BgUtils.tabRecency.init();
     let tabIds = BgUtils.tabRecency.getTabsByRecency();
     tabIds = tabIds.filter((tabId) => tabId !== tab.id);
     if (tabIds.length > 0) {

background_scripts/tab_recency.js

@@ -5,33 +5,56 @@
 // The values are persisted to chrome.storage.session so that they're not lost when the extension's
 // background page is unloaded.
 //
+// Callers must await TabRecency.init before calling recencyScore or getTabsByRecency.
+//
 // In theory, the browser's tab.lastAccessed timestamp field should allow us to sort tabs by
 // recency, but in practice it does not work across several edge cases. See the comments on #4368.
 class TabRecency {
   constructor() {
     this.counter = 1;
     this.tabIdToCounter = {};
+    this.loaded = false;
+    this.queuedActions = [];
   }
 
   // Add listeners to chrome.tabs, and load the index from session storage.
-  // If tabs are accessed before we finish loading chrome.storage.session, the in-memory stage and
-  // the session state is merged.
-  init() {
-    chrome.tabs.onActivated.addListener((activeInfo) => this.register(activeInfo.tabId));
-    chrome.tabs.onRemoved.addListener((tabId) => this.deregister(tabId));
+  async init() {
+    if (this.initPromise) {
+      await this.initPromise;
+      return;
+    }
+    let resolveFn;
+    this.initPromise = new Promise((resolve, _reject) => {
+      resolveFn = resolve;
+    });
+
+    chrome.tabs.onActivated.addListener((activeInfo) => {
+      this.queueAction("register", activeInfo.tabId);
+    });
+    chrome.tabs.onRemoved.addListener((tabId) => {
+      this.queueAction("deregister", tabId);
+    });
 
     chrome.tabs.onReplaced.addListener((addedTabId, removedTabId) => {
-      this.deregister(removedTabId);
-      this.register(addedTabId);
+      this.queueAction("deregister", removedTabId);
+      this.queueAction("register", addedTabId);
     });
 
     chrome.windows.onFocusChanged.addListener(async (windowId) => {
       if (windowId == chrome.windows.WINDOW_ID_NONE) return;
       const tabs = await chrome.tabs.query({ windowId, active: true });
-      if (tabs[0]) this.register(tabs[0].id);
+      if (tabs[0]) {
+        this.queueAction("register", tabs[0].id);
+      }
     });
 
-    this.loadFromStorage();
+    await this.loadFromStorage();
+    while (this.queuedActions.length > 0) {
+      const [action, tabId] = this.queuedActions.shift();
+      this.handleAction(action, tabId);
+    }
+    this.loaded = true;
+    resolveFn();
   }
 
   // Loads the index from session storage.
@@ -45,9 +68,10 @@ class TabRecency {
     for (const counter of Object.values(storage.tabRecency)) {
       if (maxCounter < counter) maxCounter = counter;
     }
-    if (this.counter < maxCounter + 1) {
-      this.counter = maxCounter + 1;
+    if (this.counter < maxCounter) {
+      this.counter = maxCounter;
     }
+
     // Tabs loaded from storage should be considered accessed less recently than any tab tracked in
     // memory, so increase all of the in-memory tabs's counters by maxCounter.
     for (const [id, counter] of Object.entries(this.tabIdToCounter)) {
@@ -56,22 +80,41 @@ class TabRecency {
       if (this.counter < newCounter) this.counter = newCounter;
     }
 
-    const combined = Object.assign({}, storage.tabRecency, this.tabIdToCounter);
+    this.tabIdToCounter = Object.assign({}, storage.tabRecency);
 
-    // Remove any tab IDs which may be no longer present.
+    // Remove any tab IDs which aren't currently loaded.
     const tabIds = new Set(tabs.map((t) => t.id));
-    for (const id in combined) {
+    for (const id in this.tabIdToCounter) {
       if (!tabIds.has(parseInt(id))) {
-        delete combined[id];
+        delete this.tabIdToCounter[id];
       }
     }
-    this.tabIdToCounter = combined;
   }
 
   async saveToStorage() {
     await chrome.storage.session.set({ tabRecency: this.tabIdToCounter });
   }
 
+  // - action: "register" or "unregister".
+  queueAction(action, tabId) {
+    if (!this.loaded) {
+      this.queuedActions.push([action, tabId]);
+    } else {
+      this.handleAction(action, tabId);
+    }
+  }
+
+  // - action: "register" or "unregister".
+  handleAction(action, tabId) {
+    if (action == "register") {
+      this.register(tabId);
+    } else if (action == "deregister") {
+      this.deregister(tabId);
+    } else {
+      throw new Error(`Unexpected action type: ${action}`);
+    }
+  }
+
   register(tabId) {
     this.counter++;
     this.tabIdToCounter[tabId] = this.counter;
@@ -85,6 +128,7 @@ class TabRecency {
 
   // Recently-visited tabs get a higher score (except the current tab, which gets a low score).
   recencyScore(tabId) {
+    if (!this.loaded) throw new Error("TabRecency hasn't yet been loaded.");
     const tabCounter = this.tabIdToCounter[tabId];
     const isCurrentTab = tabCounter == this.counter;
     if (isCurrentTab) return 0;
@@ -93,6 +137,7 @@ class TabRecency {
 
   // Returns a list of tab Ids sorted by recency, most recent tab first.
   getTabsByRecency() {
+    if (!this.loaded) throw new Error("TabRecency hasn't yet been loaded.");
     const ids = Object.keys(this.tabIdToCounter);
     ids.sort((a, b) => this.tabIdToCounter[b] - this.tabIdToCounter[a]);
     return ids.map((id) => parseInt(id));

tests/unit_tests/tab_recency_test.js

@@ -7,13 +7,15 @@ context("TabRecency", () => {
   setup(() => tabRecency = new TabRecency());
 
   context("order", () => {
-    setup(() => {
-      tabRecency.register(1);
-      tabRecency.register(2);
-      tabRecency.register(3);
-      tabRecency.register(4);
-      tabRecency.deregister(4);
-      tabRecency.register(2);
+    setup(async () => {
+      stub(chrome.tabs, "query", () => Promise.resolve([]));
+      await tabRecency.init();
+      tabRecency.queueAction("register", (1));
+      tabRecency.queueAction("register", (2));
+      tabRecency.queueAction("register", (3));
+      tabRecency.queueAction("register", (4));
+      tabRecency.queueAction("deregister", (4));
+      tabRecency.queueAction("register", (2));
     });
 
     should("have the correct entries in the correct order", () => {
@@ -29,48 +31,52 @@ context("TabRecency", () => {
     });
   });
 
+  should("navigate actions are queued until state from storage is loaded", async () => {
+    let onActivated;
+    stub(chrome.tabs.onActivated, "addListener", (fn) => {
+      onActivated = fn;
+    });
+    let resolveStorage;
+    const storagePromise = new Promise((resolve, _) => resolveStorage = resolve);
+    stub(chrome.storage.session, "get", () => storagePromise);
+    tabRecency.init();
+    // Here, chrome.tabs.onActivated listeners have been added by tabrecency, but the
+    // chrome.storage.session data hasn't yet loaded.
+    onActivated({ tabId: 5 });
+    resolveStorage({});
+    await tabRecency.init();
+    assert.equal([5], tabRecency.getTabsByRecency());
+  });
+
   should("loadFromStorage handles empty values", async () => {
     stub(chrome.tabs, "query", () => Promise.resolve([{ id: 1 }]));
 
     stub(chrome.storage.session, "get", () => Promise.resolve({}));
-    await tabRecency.loadFromStorage();
+    await tabRecency.init();
     assert.equal([], tabRecency.getTabsByRecency());
 
     stub(chrome.storage.session, "get", () => Promise.resolve({ tabRecency: {} }));
     await tabRecency.loadFromStorage();
     assert.equal([], tabRecency.getTabsByRecency());
   });
 
-  should("loadFromStorage merges in-memory tabs with in-storage tabs", async () => {
+  should("loadFromStorage works", async () => {
     const tabs = [{ id: 1 }, { id: 2 }, { id: 3 }, { id: 4 }];
     stub(chrome.tabs, "query", () => Promise.resolve(tabs));
 
-    tabRecency.register(3);
-    tabRecency.register(4);
-
     const storage = { tabRecency: { 1: 5, 2: 6 } };
     stub(chrome.storage.session, "get", () => Promise.resolve(storage));
 
     // Even though the in-storage tab counters are higher than the in-memory tabs, during
     // loading, the in-memory tab counters are adjusted to be the most recent.
-    await tabRecency.loadFromStorage();
-
-    assert.equal([4, 3, 2, 1], tabRecency.getTabsByRecency());
-  });
+    await tabRecency.init();
 
-  should("loadFromStorage works if there are no in-memory tab tabs", async () => {
-    const tabs = [{ id: 1 }, { id: 2 }];
-    stub(chrome.tabs, "query", () => Promise.resolve(tabs));
-
-    const storage = { tabRecency: { 1: 4, 2: 1 } };
-    stub(chrome.storage.session, "get", () => Promise.resolve(storage));
-
-    await tabRecency.loadFromStorage();
+    assert.equal([2, 1], tabRecency.getTabsByRecency());
 
-    // This tab's counter should be the max of the tab counters loaded from storage.
-    tabRecency.register(2);
+    tabRecency.queueAction("register", (3));
+    tabRecency.queueAction("register", (1));
 
-    assert.equal([2, 1], tabRecency.getTabsByRecency());
+    assert.equal([1, 3, 2], tabRecency.getTabsByRecency());
   });
 
   should("loadFromStorage prunes out tabs which are no longer active", async () => {
@@ -79,7 +85,7 @@ context("TabRecency", () => {
 
     const storage = { tabRecency: { 1: 5, 2: 6 } };
     stub(chrome.storage.session, "get", () => Promise.resolve(storage));
-    await tabRecency.loadFromStorage();
+    await tabRecency.init();
     assert.equal([1], tabRecency.getTabsByRecency());
   });
 });