Bug 1594704 - ensure sync data choices made before connecting sync are honored. r=eoger
authorMark Hammond <mhammond@skippinet.com.au>
Fri, 15 Nov 2019 19:12:16 +0000
changeset 502310 bf1d04bbfc0be1fc01828bf6c0a32a98473513d6
parent 502309 5258157edfeea095a4d96b65f819e2b6143af6c0
child 502311 1d6c106c86d52e7e6552f9fc73f5d349c1db529f
push id114172
push userdluca@mozilla.com
push dateTue, 19 Nov 2019 11:31:10 +0000
treeherdermozilla-inbound@b5c5ba07d3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerseoger
bugs1594704
milestone72.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 1594704 - ensure sync data choices made before connecting sync are honored. r=eoger Differential Revision: https://phabricator.services.mozilla.com/D53138
services/sync/modules/service.js
services/sync/tests/unit/test_service_sync_updateEnabledEngines.js
--- a/services/sync/modules/service.js
+++ b/services/sync/modules/service.js
@@ -527,18 +527,25 @@ Sync11Service.prototype = {
     this.recordManager.clearCache();
     let meta = await this.recordManager.get(this.metaURL);
     if (!meta) {
       this._log.info("Meta record is null, aborting engine state update.");
       return;
     }
     const declinedEngines = meta.payload.declined;
     const allEngines = this.engineManager.getAll().map(e => e.name);
-    for (const engine of allEngines) {
-      Svc.Prefs.set(`engine.${engine}`, !declinedEngines.includes(engine));
+    // We don't want our observer of the enabled prefs to treat the change as
+    // a user-change, otherwise we will do the wrong thing with declined etc.
+    this._ignorePrefObserver = true;
+    try {
+      for (const engine of allEngines) {
+        Svc.Prefs.set(`engine.${engine}`, !declinedEngines.includes(engine));
+      }
+    } finally {
+      this._ignorePrefObserver = false;
     }
   },
 
   QueryInterface: ChromeUtils.generateQI([
     Ci.nsIObserver,
     Ci.nsISupportsWeakReference,
   ]),
 
@@ -1357,44 +1364,50 @@ Sync11Service.prototype = {
         // Check if the identity wants to pre-fetch a migration sentinel from
         // the server.
         // If we have no clusterURL, we are probably doing a node reassignment
         // so don't attempt to get it in that case.
         if (this.clusterURL) {
           this.identity.prefetchMigrationSentinel(this);
         }
 
-        // Now let's update our declined engines (but only if we have a metaURL;
-        // if Sync failed due to no node we will not have one)
-        if (this.metaURL) {
-          let meta = await this.recordManager.get(this.metaURL);
-          if (!meta) {
-            this._log.warn("No meta/global; can't update declined state.");
-            return;
-          }
-
-          let declinedEngines = new DeclinedEngines(this);
-          let didChange = declinedEngines.updateDeclined(
-            meta,
-            this.engineManager
-          );
-          if (!didChange) {
-            this._log.info(
-              "No change to declined engines. Not reuploading meta/global."
-            );
-            return;
-          }
-
-          await this.uploadMetaGlobal(meta);
-        }
+        // Now let's update our declined engines
+        await this._maybeUpdateDeclined();
       })
     )();
   },
 
   /**
+   * Update the "declined" information in meta/global if necessary.
+   */
+  async _maybeUpdateDeclined() {
+    // if Sync failed due to no node we will not have a meta URL, so can't
+    // update anything.
+    if (!this.metaURL) {
+      return;
+    }
+    let meta = await this.recordManager.get(this.metaURL);
+    if (!meta) {
+      this._log.warn("No meta/global; can't update declined state.");
+      return;
+    }
+
+    let declinedEngines = new DeclinedEngines(this);
+    let didChange = declinedEngines.updateDeclined(meta, this.engineManager);
+    if (!didChange) {
+      this._log.info(
+        "No change to declined engines. Not reuploading meta/global."
+      );
+      return;
+    }
+
+    await this.uploadMetaGlobal(meta);
+  },
+
+  /**
    * Upload a fresh meta/global record
    * @throws the response object if the upload request was not a success
    */
   async _uploadNewMetaGlobal() {
     let meta = new WBORecord("meta", "global");
     meta.payload.syncID = this.syncID;
     meta.payload.storageVersion = STORAGE_VERSION;
     meta.payload.declined = this.engineManager.getDeclined();
--- a/services/sync/tests/unit/test_service_sync_updateEnabledEngines.js
+++ b/services/sync/tests/unit/test_service_sync_updateEnabledEngines.js
@@ -1,13 +1,17 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 const { Service } = ChromeUtils.import("resource://services-sync/service.js");
 
+const { EngineSynchronizer } = ChromeUtils.import(
+  "resource://services-sync/stages/enginesync.js"
+);
+
 function QuietStore() {
   Store.call("Quiet");
 }
 QuietStore.prototype = {
   async getAllIDs() {
     return [];
   },
 };
@@ -482,16 +486,88 @@ add_task(async function test_service_upd
   Service.identity._findCluster = () => server.baseURI + "/1.1/johndoe/";
 
   // Update engine state from the server.
   await Service.updateLocalEnginesState();
   // Now disabled.
   Assert.ok(!engine.enabled);
 });
 
+add_task(async function test_service_enableAfterUpdateState() {
+  Service.syncID = "abcdefghij";
+  const engine = Service.engineManager.get("steam");
+  const metaWBO = new ServerWBO("global", {
+    syncID: Service.syncID,
+    storageVersion: STORAGE_VERSION,
+    declined: ["steam"],
+    engines: { someengine: {} },
+  });
+  const server = httpd_setup({
+    "/1.1/johndoe/storage/meta/global": metaWBO.handler(),
+  });
+  await SyncTestingInfrastructure(server, "johndoe");
+
+  // Disconnect sync.
+  await Service.startOver();
+  Service.identity._findCluster = () => server.baseURI + "/1.1/johndoe/";
+
+  // Update engine state from the server.
+  await Service.updateLocalEnginesState();
+  // Now disabled, reflecting what's on the server.
+  Assert.ok(!engine.enabled);
+  // Enable the engine, as though the user selected it via CWTS.
+  engine.enabled = true;
+
+  // Do the "reconcile local and remote states" dance.
+  let engineSync = new EngineSynchronizer(Service);
+  await engineSync._updateEnabledEngines();
+  await Service._maybeUpdateDeclined();
+  // engine should remain enabled.
+  Assert.ok(engine.enabled);
+  // engine should no longer appear in declined on the server.
+  Assert.deepEqual(metaWBO.data.declined, []);
+});
+
+add_task(async function test_service_disableAfterUpdateState() {
+  Service.syncID = "abcdefghij";
+  const engine = Service.engineManager.get("steam");
+  const metaWBO = new ServerWBO("global", {
+    syncID: Service.syncID,
+    storageVersion: STORAGE_VERSION,
+    declined: [],
+    engines: { steam: {} },
+  });
+  const server = httpd_setup({
+    "/1.1/johndoe/storage/meta/global": metaWBO.handler(),
+  });
+  await SyncTestingInfrastructure(server, "johndoe");
+
+  // Disconnect sync.
+  await Service.startOver();
+  Service.identity._findCluster = () => server.baseURI + "/1.1/johndoe/";
+
+  // Update engine state from the server.
+  await Service.updateLocalEnginesState();
+  // Now enabled, reflecting what's on the server.
+  Assert.ok(engine.enabled);
+  // Disable the engine, as though via CWTS.
+  engine.enabled = false;
+
+  // Do the "reconcile local and remote states" dance.
+  let engineSync = new EngineSynchronizer(Service);
+  await engineSync._updateEnabledEngines();
+  await Service._maybeUpdateDeclined();
+  // engine should remain disabled.
+  Assert.ok(!engine.enabled);
+  // engine should now appear in declined on the server.
+  Assert.deepEqual(metaWBO.data.declined, ["steam"]);
+  // and should have been removed from engines.
+  Assert.deepEqual(metaWBO.data.engines, {});
+});
+
 add_task(async function test_service_updateLocalEnginesState_no_meta_global() {
   Service.syncID = "abcdefghij";
   const engine = Service.engineManager.get("steam");
   // The server doesn't contain /meta/global (sync was never enabled).
   const server = httpd_setup({});
   await SyncTestingInfrastructure(server, "johndoe");
 
   // Disconnect sync.