Bug 1783991 - ensure newly opened tabs are synced. r=Gijs

This patch takes a bit of a nuclear option: it removes all
dependencies on SessionStore, and instead just inspects the
tab itself. This means that we no longer store the full
"url history", but no sync clients actually leverage that.

A nice side-effect of this is that we now can store more tabs
in the payload, so heavy tab users are likely to see even more
of their tabs synced.

Differential Revision: https://phabricator.services.mozilla.com/D154192

services/sync/modules/engines/tabs.js

@@ -10,9 +10,6 @@ const TAB_ENTRIES_LIMIT = 5; // How many URLs to include in tab history.
 const { XPCOMUtils } = ChromeUtils.importESModule(
   "resource://gre/modules/XPCOMUtils.sys.mjs"
 );
-const { TabStateFlusher } = ChromeUtils.import(
-  "resource:///modules/sessionstore/TabStateFlusher.jsm"
-);
 const { Log } = ChromeUtils.import("resource://gre/modules/Log.jsm");
 const { Store, SyncEngine, Tracker } = ChromeUtils.import(
   "resource://services-sync/engines.js"
@@ -45,11 +42,6 @@ ChromeUtils.defineModuleGetter(
   "PrivateBrowsingUtils",
   "resource://gre/modules/PrivateBrowsingUtils.jsm"
 );
-ChromeUtils.defineModuleGetter(
-  lazy,
-  "SessionStore",
-  "resource:///modules/sessionstore/SessionStore.jsm"
-);
 
 ChromeUtils.defineESModuleGetters(lazy, {
   PlacesUtils: "resource://gre/modules/PlacesUtils.sys.mjs",
@@ -299,12 +291,9 @@ TabStore.prototype = {
     return win.closed || lazy.PrivateBrowsingUtils.isWindowPrivate(win);
   },
 
-  getTabState(tab) {
-    return JSON.parse(lazy.SessionStore.getTabState(tab));
-  },
-
   async getAllTabs(filter) {
     let allTabs = [];
+    let iconPromises = [];
 
     for (let win of this.getWindowEnumerator()) {
       if (this.shouldSkipWindow(win)) {
@@ -312,76 +301,55 @@ TabStore.prototype = {
       }
 
       for (let tab of win.gBrowser.tabs) {
-        let tabState = this.getTabState(tab);
-
-        // Make sure there are history entries to look at.
-        if (!tabState || !tabState.entries.length) {
-          // If we detected a tab but no entries we should
-          // flush the window so SessionState properly updates
-          await TabStateFlusher.flushWindow(win);
-          tabState = this.getTabState(tab);
-
-          // We failed to get entries even after a flush
-          // safe to skip this tab
-          if (!tabState || !tabState.entries.length) {
-            continue;
-          }
+        // Note that we used to sync "tab history" (ie, the "back button") state,
+        // but in practice this hasn't been used - only the current URI is of
+        // interest to clients.
+        // We stopped recording this in bug 1783991.
+        if (!tab?.linkedBrowser) {
+          continue;
         }
-
         let acceptable = !filter
           ? url => url
           : url =>
               url &&
               !lazy.TABS_FILTERED_SCHEMES.has(Services.io.extractScheme(url));
 
-        let entries = tabState.entries;
-        let index = tabState.index;
-        let current = entries[index - 1];
-
+        let url = tab.linkedBrowser.currentURI?.spec;
         // We ignore the tab completely if the current entry url is
         // not acceptable (we need something accurate to open).
-        if (!acceptable(current.url)) {
+        if (!acceptable(url)) {
           continue;
         }
 
-        if (current.url.length > URI_LENGTH_MAX) {
+        if (url.length > URI_LENGTH_MAX) {
           this._log.trace("Skipping over-long URL.");
           continue;
         }
 
-        // The element at `index` is the current page. Previous URLs were
-        // previously visited URLs; subsequent URLs are in the 'forward' stack,
-        // which we can't represent in Sync, so we truncate here.
-        let candidates =
-          entries.length == index ? entries : entries.slice(0, index);
-
-        let urls = candidates
-          .map(entry => entry.url)
-          .filter(acceptable)
-          .reverse(); // Because Sync puts current at index 0, and history after.
-
-        // Truncate if necessary.
-        if (urls.length > TAB_ENTRIES_LIMIT) {
-          urls.length = TAB_ENTRIES_LIMIT;
-        }
-
-        // tabState has .image, but it's a large data: url. So we ask the favicon service for the url.
-        let icon = "";
-        try {
-          let iconData = await lazy.PlacesUtils.promiseFaviconData(urls[0]);
-          icon = iconData.uri.spec;
-        } catch (ex) {
-          this._log.trace(`Failed to fetch favicon for ${urls[0]}`, ex);
-        }
-        allTabs.push({
-          title: current.title || "",
-          urlHistory: urls,
-          icon,
-          lastUsed: Math.floor((tabState.lastAccessed || 0) / 1000),
-        });
+        let thisTab = {
+          title: tab.linkedBrowser.contentTitle || "",
+          urlHistory: [url],
+          icon: "",
+          lastUsed: Math.floor((tab.lastAccessed || 0) / 1000),
+        };
+        allTabs.push(thisTab);
+        // Use the favicon service for the icon url - we can wait for the promises at the end.
+        let iconPromise = lazy.PlacesUtils.promiseFaviconData(url)
+          .then(iconData => {
+            thisTab.icon = iconData.uri.spec;
+          })
+          .catch(ex => {
+            this._log.trace(
+              `Failed to fetch favicon for ${url}`,
+              thisTab.urlHistory[0]
+            );
+          });
+        iconPromises.push(iconPromise);
       }
     }
 
+    await Promise.allSettled(iconPromises);
+
     return allTabs;
   },
 

services/sync/tests/unit/head_helpers.js

@@ -208,16 +208,10 @@ function mockGetTabState(tab) {
   return tab;
 }
 
-function mockGetWindowEnumerator(url, numWindows, numTabs, indexes, moreURLs) {
+function mockGetWindowEnumerator(urls) {
   let elements = [];
 
-  function url2entry(urlToConvert) {
-    return {
-      url: typeof urlToConvert == "function" ? urlToConvert() : urlToConvert,
-      title: "title",
-    };
-  }
-
+  const numWindows = 1;
   for (let w = 0; w < numWindows; ++w) {
     let tabs = [];
     let win = {
@@ -229,22 +223,16 @@ function mockGetWindowEnumerator(url, numWindows, numTabs, indexes, moreURLs) {
     };
     elements.push(win);
 
-    for (let t = 0; t < numTabs; ++t) {
-      tabs.push(
-        Cu.cloneInto(
-          {
-            index: indexes ? indexes() : 1,
-            entries: (moreURLs ? [url].concat(moreURLs()) : [url]).map(
-              url2entry
-            ),
-            attributes: {
-              image: "image",
-            },
-            lastAccessed: 1499,
-          },
-          {}
-        )
-      );
+    let lastAccessed = 2000;
+    for (let url of urls) {
+      tabs.push({
+        linkedBrowser: {
+          currentURI: Services.io.newURI(url),
+          contentTitle: "title",
+        },
+        lastAccessed,
+      });
+      lastAccessed += 1000;
     }
   }
 

services/sync/tests/unit/test_tab_engine.js

@@ -128,7 +128,7 @@ add_task(async function test_reconcile() {
 add_task(async function test_modified_after_fail() {
   let [engine, store] = await getMocks();
   store.getWindowEnumerator = () =>
-    mockGetWindowEnumerator("http://example.com", 1, 1);
+    mockGetWindowEnumerator(["http://example.com"]);
 
   let server = await serverForFoo(engine);
   await SyncTestingInfrastructure(server);
@@ -143,9 +143,9 @@ add_task(async function test_modified_after_fail() {
       [
         {
           title: "title",
-          urlHistory: ["http://example.com"],
+          urlHistory: ["http://example.com/"],
           icon: "",
-          lastUsed: 1,
+          lastUsed: 2,
         },
       ],
       "Should upload mock local tabs on first sync"

services/sync/tests/unit/test_tab_store.js

@@ -55,122 +55,68 @@ add_task(async function test_getAllTabs() {
   let store = await getMockStore();
   let tabs;
 
-  let threeUrls = ["http://foo.com", "http://fuubar.com", "http://barbar.com"];
-
-  store.getWindowEnumerator = mockGetWindowEnumerator.bind(
-    this,
+  store.getWindowEnumerator = mockGetWindowEnumerator.bind(this, [
     "http://bar.com",
-    1,
-    1,
-    () => 2,
-    () => threeUrls
-  );
+  ]);
 
   _("Get all tabs.");
   tabs = await store.getAllTabs();
   _("Tabs: " + JSON.stringify(tabs));
   equal(tabs.length, 1);
   equal(tabs[0].title, "title");
-  equal(tabs[0].urlHistory.length, 2);
-  equal(tabs[0].urlHistory[0], "http://foo.com");
-  equal(tabs[0].urlHistory[1], "http://bar.com");
+  equal(tabs[0].urlHistory.length, 1);
+  equal(tabs[0].urlHistory[0], "http://bar.com/");
   equal(tabs[0].icon, "");
-  equal(tabs[0].lastUsed, 1);
+  equal(tabs[0].lastUsed, 2);
 
   _("Get all tabs, and check that filtering works.");
-  let twoUrls = ["about:foo", "http://fuubar.com"];
-  store.getWindowEnumerator = mockGetWindowEnumerator.bind(
-    this,
+  store.getWindowEnumerator = mockGetWindowEnumerator.bind(this, [
     "http://foo.com",
-    1,
-    1,
-    () => 2,
-    () => twoUrls
-  );
+    "about:foo",
+  ]);
   tabs = await store.getAllTabs(true);
   _("Filtered: " + JSON.stringify(tabs));
-  equal(tabs.length, 0);
-
-  _("Get all tabs, and check that the entries safety limit works.");
-  let allURLs = [];
-  for (let i = 0; i < 50; i++) {
-    allURLs.push("http://foo" + i + ".bar");
-  }
-  allURLs.splice(35, 0, "about:foo", "about:bar", "about:foobar");
-
-  store.getWindowEnumerator = mockGetWindowEnumerator.bind(
-    this,
-    "http://bar.com",
-    1,
-    1,
-    () => 45,
-    () => allURLs
-  );
-  tabs = await store.getAllTabs(url => url.startsWith("about"));
-
-  _("Sliced: " + JSON.stringify(tabs));
   equal(tabs.length, 1);
-  equal(tabs[0].urlHistory.length, 5);
-  equal(tabs[0].urlHistory[0], "http://foo40.bar");
-  equal(tabs[0].urlHistory[4], "http://foo36.bar");
 });
 
 add_task(async function test_createRecord() {
   let store = await getMockStore();
   let record;
 
-  store.getTabState = mockGetTabState;
-  store.shouldSkipWindow = mockShouldSkipWindow;
-  store.getWindowEnumerator = mockGetWindowEnumerator.bind(
-    this,
-    "http://foo.com",
-    1,
-    1
-  );
   // This number is sensitive to our hard-coded default max record payload size
   // in service.js (256 * 1024).
   // It should be larger than how many records we can fit in a single payload.
-  let numtabs = 2700;
+  let numTabs = 2700;
+  let urls = [];
+  for (let i = 0; i < numTabs; i++) {
+    urls.push("http://foo.com" + i + ".bar");
+  }
 
-  store.getWindowEnumerator = mockGetWindowEnumerator.bind(
-    this,
-    "http://foo.com",
-    1,
-    1
-  );
-  record = await store.createRecord("fake-guid");
-  ok(record instanceof TabSetRecord);
-  equal(record.tabs.length, 1);
+  store.getWindowEnumerator = mockGetWindowEnumerator.bind(this, urls, 1, 1);
 
-  _("create a big record");
-  store.getWindowEnumerator = mockGetWindowEnumerator.bind(
-    this,
-    "http://foo.com",
-    1,
-    numtabs
-  );
   record = await store.createRecord("fake-guid");
   ok(record instanceof TabSetRecord);
   // This number is sensitive to our hard-coded default max record payload size
   // in service.js (256 * 1024). Given our mock session-store etc, it is the
   // actual max we can fit.
-  equal(record.tabs.length, 2672);
+  equal(record.tabs.length, 2309);
 
+  // Now increase the max-payload size - the number of tabs we can store should be larger.
   let maxSizeStub = sinon
     .stub(Service, "getMemcacheMaxRecordPayloadSize")
     .callsFake(() => 512 * 1024);
   try {
-    numtabs = 5400;
+    numTabs = 5400;
     _("Modify the max record payload size and create a big record");
-    store.getWindowEnumerator = mockGetWindowEnumerator.bind(
-      this,
-      "http://foo.com",
-      1,
-      numtabs
-    );
+    urls = [];
+    for (let i = 0; i < numTabs; i++) {
+      urls.push("http://foo.com/" + i + ".bar");
+    }
+    store.getWindowEnumerator = mockGetWindowEnumerator.bind(this, urls);
     record = await store.createRecord("fake-guid");
     ok(record instanceof TabSetRecord);
-    equal(record.tabs.length, 5365);
+    // should now be roughly twice what we could fit before.
+    equal(record.tabs.length, 4613);
   } finally {
     maxSizeStub.restore();
   }