Bug 1773154 - Reduce number of scheduled sync calls in sync-after-tab-change r=markh,Gijs

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

modules/libpref/init/all.js

@@ -4190,7 +4190,7 @@ pref("services.common.log.logger.tokenserverclient", "Debug");
   pref("services.sync.engine.passwords", true);
   pref("services.sync.engine.prefs", true);
   pref("services.sync.engine.tabs", true);
-  pref("services.sync.engine.tabs.filteredSchemes", "about|resource|chrome|file|blob|moz-extension");
+  pref("services.sync.engine.tabs.filteredSchemes", "about|resource|chrome|file|blob|moz-extension|data");
 
   // The addresses and CC engines might not actually be available at all.
   pref("services.sync.engine.addresses.available", false);

services/sync/modules/engines/prefs.js

@@ -139,9 +139,9 @@ PrefsEngine.prototype = {
 
 // We don't use services.sync.engine.tabs.filteredSchemes since it includes
 // about: pages and the like, which we want to be syncable in preferences.
-// Blob and moz-extension uris are never safe to sync, so we limit our check
-// to those.
-const UNSYNCABLE_URL_REGEXP = /^(moz-extension|blob):/i;
+// Blob, moz-extension, data and file uris are never safe to sync,
+// so we limit our check to those.
+const UNSYNCABLE_URL_REGEXP = /^(moz-extension|blob|data|file):/i;
 function isUnsyncableURLPref(prefName) {
   if (Services.prefs.getPrefType(prefName) != Ci.nsIPrefBranch.PREF_STRING) {
     return false;

services/sync/modules/engines/tabs.js

@@ -471,7 +471,9 @@ TabTracker.prototype = {
     this.modified = false;
   },
 
-  _topics: ["TabOpen", "TabClose", "TabSelect"],
+  // We do not track TabSelect because that almost always triggers
+  // the web progress listeners (onLocationChange), which we already track
+  _topics: ["TabOpen", "TabClose"],
 
   _registerListenersForWindow(window) {
     this._log.trace("Registering tab listeners in window");
@@ -540,9 +542,29 @@ TabTracker.prototype = {
         return;
       }
     }
-
     this._log.trace("onTab event: " + event.type);
-    this.callScheduleSync(SCORE_INCREMENT_SMALL);
+
+    switch (event.type) {
+      case "TabOpen":
+        /* We do not have a reliable way of checking the URI on the TabOpen
+         * so we will rely on the other methods (onLocationChange, getAllTabs)
+         * to filter these when going through sync
+         */
+        this.callScheduleSync(SCORE_INCREMENT_SMALL);
+        break;
+      case "TabClose":
+        // If event target has `linkedBrowser`, the event target can be assumed <tab> element.
+        // Else, event target is assumed <browser> element, use the target as it is.
+        const tab = event.target.linkedBrowser || event.target;
+
+        // TabClose means the tab has already loaded and we can check the URI
+        // and ignore if it's a scheme we don't care about
+        if (lazy.TABS_FILTERED_SCHEMES.has(tab.currentURI.scheme)) {
+          return;
+        }
+        this.callScheduleSync(SCORE_INCREMENT_SMALL);
+        break;
+    }
   },
 
   // web progress listeners.
@@ -551,6 +573,7 @@ TabTracker.prototype = {
     // document.
     if (
       flags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT ||
+      flags & Ci.nsIWebProgressListener.LOCATION_CHANGE_RELOAD ||
       !webProgress.isTopLevel ||
       !locationURI
     ) {

services/sync/tests/unit/test_tab_tracker.js

@@ -70,11 +70,16 @@ function fakeSvcWinMediator() {
   return logs;
 }
 
+function fakeGetTabState(tab) {
+  return tab;
+}
+
 add_task(async function run_test() {
   let engine = Service.engineManager.get("tabs");
 
   _("We assume that tabs have changed at startup.");
   let tracker = engine._tracker;
+  tracker.getTabState = fakeGetTabState;
 
   Assert.ok(tracker.modified);
   Assert.ok(
@@ -90,10 +95,9 @@ add_task(async function run_test() {
   tracker.start();
   Assert.equal(logs.length, 2);
   for (let log of logs) {
-    Assert.equal(log.addTopics.length, 4);
+    Assert.equal(log.addTopics.length, 3);
     Assert.ok(log.addTopics.includes("TabOpen"));
     Assert.ok(log.addTopics.includes("TabClose"));
-    Assert.ok(log.addTopics.includes("TabSelect"));
     Assert.ok(log.addTopics.includes("unload"));
     Assert.equal(log.remTopics.length, 0);
     Assert.equal(log.numAPL, 1, "Added 1 progress listener");
@@ -106,23 +110,26 @@ add_task(async function run_test() {
   Assert.equal(logs.length, 2);
   for (let log of logs) {
     Assert.equal(log.addTopics.length, 0);
-    Assert.equal(log.remTopics.length, 4);
+    Assert.equal(log.remTopics.length, 3);
     Assert.ok(log.remTopics.includes("TabOpen"));
     Assert.ok(log.remTopics.includes("TabClose"));
-    Assert.ok(log.remTopics.includes("TabSelect"));
     Assert.ok(log.remTopics.includes("unload"));
     Assert.equal(log.numAPL, 0, "Didn't add a progress listener");
     Assert.equal(log.numRPL, 1, "Removed 1 progress listener");
   }
 
   _("Test tab listener");
-  for (let evttype of ["TabOpen", "TabClose", "TabSelect"]) {
+  for (let evttype of ["TabOpen", "TabClose"]) {
     // Pretend we just synced.
     await tracker.clearChangedIDs();
     Assert.ok(!tracker.modified);
 
     // Send a fake tab event
-    tracker.onTab({ type: evttype, originalTarget: evttype });
+    tracker.onTab({
+      type: evttype,
+      originalTarget: evttype,
+      target: { entries: [], currentURI: "about:config" },
+    });
     Assert.ok(tracker.modified);
     Assert.ok(
       Utils.deepEquals(Object.keys(await engine.getChangedIDs()), [
@@ -135,7 +142,11 @@ add_task(async function run_test() {
   await tracker.clearChangedIDs();
   Assert.ok(!tracker.modified);
 
-  tracker.onTab({ type: "TabOpen", originalTarget: "TabOpen" });
+  tracker.onTab({
+    type: "TabOpen",
+    originalTarget: "TabOpen",
+    target: { entries: [], currentURI: "about:config" },
+  });
   Assert.ok(
     Utils.deepEquals(Object.keys(await engine.getChangedIDs()), [
       clientsEngine.localID,
@@ -162,8 +173,7 @@ add_task(async function run_test() {
   tracker.onLocationChange(
     { isTopLevel: true },
     undefined,
-    Services.io.newURI("https://www.mozilla.org"),
-    Ci.nsIWebProgressListener.LOCATION_CHANGE_RELOAD
+    Services.io.newURI("https://www.mozilla.org")
   );
   Assert.ok(
     tracker.modified,
@@ -211,6 +221,7 @@ add_task(async function run_sync_on_tab_change_test() {
 
   _("We assume that tabs have changed at startup.");
   let tracker = engine._tracker;
+  tracker.getTabState = fakeGetTabState;
 
   Assert.ok(tracker.modified);
   Assert.ok(
@@ -222,16 +233,35 @@ add_task(async function run_sync_on_tab_change_test() {
   _("Test sync is scheduled after a tab change if experiment is enabled");
   for (let evttype of ["TabOpen", "TabClose", "TabSelect"]) {
     // Send a fake tab event
-    tracker.onTab({ type: evttype, originalTarget: evttype });
+    tracker.onTab({
+      type: evttype,
+      originalTarget: evttype,
+      target: { entries: [], currentURI: "about:config" },
+    });
     // Ensure the tracker fired
     Assert.ok(tracker.modified);
     // We should be scheduling <= experiment value
     Assert.ok(scheduler.nextSync - Date.now() <= testExperimentDelay);
   }
 
-  _("Test navigating within the same tab triggers a sync");
+  _("Test sync is NOT scheduled after an unsupported tab open");
+  for (let evttype of ["TabOpen"]) {
+    // Send a fake tab event
+    tracker.onTab({
+      type: evttype,
+      originalTarget: evttype,
+      target: { entries: ["about:newtab"], currentURI: null },
+    });
+    // Ensure the tracker fired
+    Assert.ok(tracker.modified);
+    // We should be scheduling <= experiment value
+    Assert.ok(scheduler.nextSync - Date.now() <= testExperimentDelay);
+  }
+
+  _("Test navigating within the same tab does NOT trigger a sync");
   // Pretend we just synced
   await tracker.clearChangedIDs();
+  scheduler.nextSync = Date.now() + scheduler.idleInterval;
 
   tracker.onLocationChange(
     { isTopLevel: true },
@@ -240,12 +270,12 @@ add_task(async function run_sync_on_tab_change_test() {
     Ci.nsIWebProgressListener.LOCATION_CHANGE_RELOAD
   );
   Assert.ok(
-    tracker.modified,
-    "location change for a new top-level document flagged as modified"
+    !tracker.modified,
+    "location change for reloading doesn't trigger a sync"
   );
   Assert.ok(
-    scheduler.nextSync - Date.now() <= testExperimentDelay,
-    "top level document change triggers a sync when experiment is enabled"
+    scheduler.nextSync - Date.now() > testExperimentDelay,
+    "reload does not trigger a sync"
   );
 
   // Pretend we just synced
@@ -301,8 +331,7 @@ add_task(async function run_sync_on_tab_change_test() {
   tracker.onLocationChange(
     { isTopLevel: true },
     undefined,
-    Services.io.newURI("https://www.mozilla.org"),
-    Ci.nsIWebProgressListener.LOCATION_CHANGE_RELOAD
+    Services.io.newURI("https://www.mozilla.org")
   );
   Assert.ok(
     tracker.modified,
@@ -325,7 +354,11 @@ add_task(async function run_sync_on_tab_change_test() {
   scheduler.updateClientMode();
   Assert.equal(scheduler.numClients, 2);
   // Fire ontab event
-  tracker.onTab({ type: evttype, originalTarget: evttype });
+  tracker.onTab({
+    type: evttype,
+    originalTarget: evttype,
+    target: { entries: [], currentURI: "about:config" },
+  });
 
   // Ensure the tracker fired
   Assert.ok(tracker.modified);
@@ -350,7 +383,11 @@ add_task(async function run_sync_on_tab_change_test() {
 
   // Fire ontab event
   evttype = "TabOpen";
-  tracker.onTab({ type: evttype, originalTarget: evttype });
+  tracker.onTab({
+    type: evttype,
+    originalTarget: evttype,
+    target: { entries: [], currentURI: "about:config" },
+  });
   // Ensure the tracker fired
   Assert.ok(tracker.modified);
 

toolkit/modules/tests/xpcshell/test_BrowserUtils.js

@@ -192,7 +192,7 @@ add_task(function test_isShareableURL() {
   if (!Preferences.get("services.sync.engine.tabs.filteredSchemes")) {
     Preferences.set(
       "services.sync.engine.tabs.filteredSchemes",
-      "about|resource|chrome|file|blob|moz-extension"
+      "about|resource|chrome|file|blob|moz-extension|data"
     );
   }
   // Empty shouldn't be sendable
@@ -205,4 +205,13 @@ add_task(function test_isShareableURL() {
   Assert.ok(
     !BrowserUtils.isShareableURL(Services.io.newURI("file://path/to/pdf.pdf"))
   );
+
+  // Invalid
+  Assert.ok(
+    !BrowserUtils.isShareableURL(
+      Services.io.newURI(
+        "data:application/json;base64,ewogICJ0eXBlIjogIm1haW4i=="
+      )
+    )
+  );
 });