From d357497441aa269492cba08fa47402026e8b6e7c Mon Sep 17 00:00:00 2001 From: Sammy Khamis Date: Tue, 21 Jun 2022 20:17:22 +0000 Subject: [PATCH] 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 | 2 +- services/sync/modules/engines/prefs.js | 6 +- services/sync/modules/engines/tabs.js | 29 ++++++- services/sync/tests/unit/test_tab_tracker.js | 75 ++++++++++++++----- .../tests/xpcshell/test_BrowserUtils.js | 11 ++- 5 files changed, 96 insertions(+), 27 deletions(-) diff --git a/modules/libpref/init/all.js b/modules/libpref/init/all.js index 691bc522aa39..09ae694a8893 100644 --- a/modules/libpref/init/all.js +++ b/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); diff --git a/services/sync/modules/engines/prefs.js b/services/sync/modules/engines/prefs.js index 8b484c3fa921..edaccf68425b 100644 --- a/services/sync/modules/engines/prefs.js +++ b/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; diff --git a/services/sync/modules/engines/tabs.js b/services/sync/modules/engines/tabs.js index 139748ec4309..c33074b5ae72 100644 --- a/services/sync/modules/engines/tabs.js +++ b/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 element. + // Else, event target is assumed 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 ) { diff --git a/services/sync/tests/unit/test_tab_tracker.js b/services/sync/tests/unit/test_tab_tracker.js index 0f3e8beddbc5..992d577cca53 100644 --- a/services/sync/tests/unit/test_tab_tracker.js +++ b/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); diff --git a/toolkit/modules/tests/xpcshell/test_BrowserUtils.js b/toolkit/modules/tests/xpcshell/test_BrowserUtils.js index 33c39a2e3a78..66a66e20619e 100644 --- a/toolkit/modules/tests/xpcshell/test_BrowserUtils.js +++ b/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==" + ) + ) + ); });