Bug 1639727 - Fix `trackRemainingChanges` in the tabs and prefs Sync engines. r=markh
authorLina Cambridge <lina@yakshaving.ninja>
Thu, 21 May 2020 07:13:12 +0000
changeset 531388 106dc3f2e4417b54693cb4c447c4a5b50c32092a
parent 531387 d711a889379bca7f182da0756730e7ac4d054576
child 531389 8e9afbc5d9482ea18ca4b9dd4be9173eef478db0
push id37439
push userbtara@mozilla.com
push dateThu, 21 May 2020 21:49:34 +0000
treeherdermozilla-central@92c11f0bf14b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1639727
milestone78.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1639727 - Fix `trackRemainingChanges` in the tabs and prefs Sync engines. r=markh This commit fixes two issues: moves `trackRemainingChanges` to the engine, since that's where it defined (not on the store), and skips setting `modified` if all records have been successfully uploaded. Differential Revision: https://phabricator.services.mozilla.com/D76270
services/sync/modules/engines.js
services/sync/modules/engines/prefs.js
services/sync/modules/engines/tabs.js
services/sync/tests/unit/test_prefs_engine.js
services/sync/tests/unit/test_tab_engine.js
services/sync/tests/unit/xpcshell.ini
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -1991,20 +1991,16 @@ SyncEngine.prototype = {
       }
     }
     this.hasSyncedThisSession = true;
     await this._tracker.asyncObserver.promiseObserversComplete();
   },
 
   async _syncCleanup() {
     this._needWeakUpload.clear();
-    if (!this._modified) {
-      return;
-    }
-
     try {
       // Mark failed WBOs as changed again so they are reuploaded next time.
       await this.trackRemainingChanges();
     } finally {
       this._modified.clear();
     }
   },
 
--- a/services/sync/modules/engines/prefs.js
+++ b/services/sync/modules/engines/prefs.js
@@ -1,13 +1,13 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-var EXPORTED_SYMBOLS = ["PrefsEngine", "PrefRec"];
+var EXPORTED_SYMBOLS = ["PrefsEngine", "PrefRec", "PREFS_GUID"];
 
 const PREF_SYNC_PREFS_PREFIX = "services.sync.prefs.sync.";
 
 const { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
 );
 const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
 const { Preferences } = ChromeUtils.import(
@@ -122,16 +122,22 @@ PrefsEngine.prototype = {
   async _reconcile(item) {
     // Apply the incoming item if we don't care about the local data
     if (this.justWiped) {
       this.justWiped = false;
       return true;
     }
     return SyncEngine.prototype._reconcile.call(this, item);
   },
+
+  async trackRemainingChanges() {
+    if (this._modified.count() > 0) {
+      this._tracker.modified = true;
+    }
+  },
 };
 
 // We don't use services.sync.engine.tabs.filteredUrls 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;
 function isUnsyncableURLPref(prefName) {
@@ -354,20 +360,16 @@ PrefStore.prototype = {
 
     this._log.trace("Received pref updates, applying...");
     this._setAllPrefs(record.value);
   },
 
   async wipe() {
     this._log.trace("Ignoring wipe request");
   },
-
-  async trackRemainingChanges() {
-    this._tracker.modified = true;
-  },
 };
 
 function PrefTracker(name, engine) {
   Tracker.call(this, name, engine);
   this._ignoreAll = false;
   Svc.Obs.add("profile-before-change", this.asyncObserver);
 }
 PrefTracker.prototype = {
--- a/services/sync/modules/engines/tabs.js
+++ b/services/sync/modules/engines/tabs.js
@@ -103,16 +103,22 @@ TabEngine.prototype = {
       this._log.trace(
         "Ignoring incoming tab item because of its id: " + item.id
       );
       return false;
     }
 
     return SyncEngine.prototype._reconcile.call(this, item);
   },
+
+  async trackRemainingChanges() {
+    if (this._modified.count() > 0) {
+      this._tracker.modified = true;
+    }
+  },
 };
 
 function TabStore(name, engine) {
   Store.call(this, name, engine);
 }
 TabStore.prototype = {
   __proto__: Store.prototype,
 
@@ -271,20 +277,16 @@ TabStore.prototype = {
     this._remoteClients[record.id] = Object.assign({}, record.cleartext, {
       lastModified: record.modified,
     });
   },
 
   async update(record) {
     this._log.trace("Ignoring tab updates as local ones win");
   },
-
-  async trackRemainingChanges() {
-    this._tracker.modified = true;
-  },
 };
 
 function TabTracker(name, engine) {
   Tracker.call(this, name, engine);
 
   // Make sure "this" pointer is always set correctly for event listeners.
   this.onTab = Utils.bind2(this, this.onTab);
   this._unregisterListeners = Utils.bind2(this, this._unregisterListeners);
new file mode 100644
--- /dev/null
+++ b/services/sync/tests/unit/test_prefs_engine.js
@@ -0,0 +1,129 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+const { PREFS_GUID } = ChromeUtils.import(
+  "resource://services-sync/engines/prefs.js"
+);
+const { Service } = ChromeUtils.import("resource://services-sync/service.js");
+
+async function cleanup(engine, server) {
+  await engine._tracker.stop();
+  await engine.wipeClient();
+  Svc.Prefs.resetBranch("");
+  Service.recordManager.clearCache();
+  await promiseStopServer(server);
+}
+
+add_task(async function test_modified_after_fail() {
+  let engine = Service.engineManager.get("prefs");
+
+  let server = await serverForFoo(engine);
+  await SyncTestingInfrastructure(server);
+
+  try {
+    // The homepage pref is synced by default.
+    _("Set homepage before first sync");
+    Services.prefs.setStringPref("browser.startup.homepage", "about:welcome");
+
+    _("First sync; create collection and pref record on server");
+    await sync_engine_and_validate_telem(engine, false);
+
+    let collection = server.user("foo").collection("prefs");
+    equal(
+      collection.cleartext(PREFS_GUID).value["browser.startup.homepage"],
+      "about:welcome",
+      "Should upload homepage in pref record"
+    );
+    ok(
+      !engine._tracker.modified,
+      "Tracker shouldn't be modified after first sync"
+    );
+
+    // Our tracker only has a `modified` flag that's reset after a
+    // successful upload. Force it to remain set by failing the
+    // upload.
+    _("Second sync; flag tracker as modified and throw on upload");
+    Services.prefs.setStringPref("browser.startup.homepage", "about:robots");
+    engine._tracker.modified = true;
+    let oldPost = collection.post;
+    collection.post = () => {
+      throw new Error("Sync this!");
+    };
+    await Assert.rejects(
+      sync_engine_and_validate_telem(engine, true),
+      ex => ex.success === false
+    );
+    ok(
+      engine._tracker.modified,
+      "Tracker should remain modified after failed sync"
+    );
+
+    _("Third sync");
+    collection.post = oldPost;
+    await sync_engine_and_validate_telem(engine, false);
+    equal(
+      collection.cleartext(PREFS_GUID).value["browser.startup.homepage"],
+      "about:robots",
+      "Should upload new homepage on third sync"
+    );
+    ok(
+      !engine._tracker.modified,
+      "Tracker shouldn't be modified again after third sync"
+    );
+  } finally {
+    await cleanup(engine, server);
+  }
+});
+
+add_task(async function test_allow_arbitrary() {
+  let engine = Service.engineManager.get("prefs");
+
+  let server = await serverForFoo(engine);
+  await SyncTestingInfrastructure(server);
+
+  try {
+    _("Create collection and pref record on server");
+    await sync_engine_and_validate_telem(engine, false);
+
+    let collection = server.user("foo").collection("prefs");
+
+    _("Insert arbitrary pref into remote record");
+    let cleartext1 = collection.cleartext(PREFS_GUID);
+    cleartext1.value.let_viruses_take_over = true;
+    collection.insert(
+      PREFS_GUID,
+      encryptPayload(cleartext1),
+      new_timestamp() + 5
+    );
+
+    _("Sync again; client shouldn't allow pref");
+    await sync_engine_and_validate_telem(engine, false);
+    ok(
+      !Services.prefs.getBoolPref("let_viruses_take_over", false),
+      "Shouldn't allow arbitrary remote prefs without control pref"
+    );
+
+    _("Sync with control pref set; client should set new pref");
+    Services.prefs.setBoolPref(
+      "services.sync.prefs.sync.let_viruses_take_over_take_two",
+      true
+    );
+
+    let cleartext2 = collection.cleartext(PREFS_GUID);
+    cleartext2.value.let_viruses_take_over_take_two = true;
+    collection.insert(
+      PREFS_GUID,
+      encryptPayload(cleartext2),
+      new_timestamp() + 5
+    );
+    // Reset the last sync time so that the engine fetches the record again.
+    await engine.setLastSync(0);
+    await sync_engine_and_validate_telem(engine, false);
+    ok(
+      Services.prefs.getBoolPref("let_viruses_take_over_take_two"),
+      "Should set arbitrary remote pref with control pref"
+    );
+  } finally {
+    await cleanup(engine, server);
+  }
+});
--- a/services/sync/tests/unit/test_tab_engine.js
+++ b/services/sync/tests/unit/test_tab_engine.js
@@ -119,8 +119,65 @@ add_task(async function test_reconcile()
   ok(
     !(await engine._reconcile(localRecord)),
     "Skip incoming local if modified long ago"
   );
 
   localRecord.deleted = true;
   ok(!(await engine._reconcile(localRecord)), "Skip incoming local if deleted");
 });
+
+add_task(async function test_modified_after_fail() {
+  let [engine, store] = await getMocks();
+  store.getWindowEnumerator = () =>
+    mockGetWindowEnumerator("http://example.com", 1, 1);
+
+  let server = await serverForFoo(engine);
+  await SyncTestingInfrastructure(server);
+
+  try {
+    _("First sync; create collection and tabs record on server");
+    await sync_engine_and_validate_telem(engine, false);
+
+    let collection = server.user("foo").collection("tabs");
+    deepEqual(
+      collection.cleartext(engine.service.clientsEngine.localID).tabs,
+      [
+        {
+          title: "title",
+          urlHistory: ["http://example.com"],
+          icon: "",
+          lastUsed: 1,
+        },
+      ],
+      "Should upload mock local tabs on first sync"
+    );
+    ok(
+      !engine._tracker.modified,
+      "Tracker shouldn't be modified after first sync"
+    );
+
+    _("Second sync; flag tracker as modified and throw on upload");
+    engine._tracker.modified = true;
+    let oldPost = collection.post;
+    collection.post = () => {
+      throw new Error("Sync this!");
+    };
+    await Assert.rejects(
+      sync_engine_and_validate_telem(engine, true),
+      ex => ex.success === false
+    );
+    ok(
+      engine._tracker.modified,
+      "Tracker should remain modified after failed sync"
+    );
+
+    _("Third sync");
+    collection.post = oldPost;
+    await sync_engine_and_validate_telem(engine, false);
+    ok(
+      !engine._tracker.modified,
+      "Tracker shouldn't be modified again after third sync"
+    );
+  } finally {
+    await promiseStopServer(server);
+  }
+});
--- a/services/sync/tests/unit/xpcshell.ini
+++ b/services/sync/tests/unit/xpcshell.ini
@@ -155,16 +155,17 @@ run-sequentially = Frequent timeouts, bu
 [test_form_validator.js]
 [test_history_engine.js]
 [test_history_store.js]
 [test_history_tracker.js]
 [test_password_engine.js]
 [test_password_store.js]
 [test_password_validator.js]
 [test_password_tracker.js]
+[test_prefs_engine.js]
 [test_prefs_store.js]
 support-files = prefs_test_prefs_store.js
 [test_prefs_tracker.js]
 [test_tab_engine.js]
 [test_tab_store.js]
 [test_tab_tracker.js]
 
 [test_postqueue.js]