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

@@ -1996,10 +1996,6 @@ SyncEngine.prototype = {
 
   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();

services/sync/modules/engines/prefs.js

@@ -2,7 +2,7 @@
  * 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.";
 
@@ -127,6 +127,12 @@ PrefsEngine.prototype = {
     }
     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
@@ -359,10 +365,6 @@ PrefStore.prototype = {
   async wipe() {
     this._log.trace("Ignoring wipe request");
   },
-
-  async trackRemainingChanges() {
-    this._tracker.modified = true;
-  },
 };
 
 function PrefTracker(name, engine) {

services/sync/modules/engines/tabs.js

@@ -108,6 +108,12 @@ TabEngine.prototype = {
 
     return SyncEngine.prototype._reconcile.call(this, item);
   },
+
+  async trackRemainingChanges() {
+    if (this._modified.count() > 0) {
+      this._tracker.modified = true;
+    }
+  },
 };
 
 function TabStore(name, engine) {
@@ -276,10 +282,6 @@ TabStore.prototype = {
   async update(record) {
     this._log.trace("Ignoring tab updates as local ones win");
   },
-
-  async trackRemainingChanges() {
-    this._tracker.modified = true;
-  },
 };
 
 function TabTracker(name, engine) {

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);
+  }
+});

services/sync/tests/unit/test_tab_engine.js

@@ -124,3 +124,60 @@ add_task(async function test_reconcile() {
   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);
+  }
+});

services/sync/tests/unit/xpcshell.ini

@@ -160,6 +160,7 @@ run-sequentially = Frequent timeouts, bug 1395148
 [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]