Bug 1401700 - Prevent incoming tabs from opening multiple times if client sync fails. r=tcsc
authorEdouard Oger <eoger@fastmail.com>
Tue, 03 Oct 2017 14:45:11 -0400
changeset 428391 7b8f1bcaf50db587e235b3f03835adb48bfeb84c
parent 428390 6c7b05c7855148adba9fb99643627cbb85a6ff92
child 428392 e92ca171760d45bc16813e6f235bb7bd2c48ca81
push id97
push userfmarier@mozilla.com
push dateSat, 14 Oct 2017 01:12:59 +0000
reviewerstcsc
bugs1401700
milestone58.0a1
Bug 1401700 - Prevent incoming tabs from opening multiple times if client sync fails. r=tcsc MozReview-Commit-ID: DhrZ1oFY2WG
services/sync/modules/engines/clients.js
services/sync/tests/unit/test_bookmark_repair.js
services/sync/tests/unit/test_clients_engine.js
--- a/services/sync/modules/engines/clients.js
+++ b/services/sync/modules/engines/clients.js
@@ -45,16 +45,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyModuleGetter(this, "getRepairResponder",
   "resource://services-sync/collection_repair.js");
 
 const CLIENTS_TTL = 1814400; // 21 days
 const CLIENTS_TTL_REFRESH = 604800; // 7 days
 const STALE_CLIENT_REMOTE_AGE = 604800; // 7 days
 
 const SUPPORTED_PROTOCOL_VERSIONS = [SYNC_API_VERSION];
+const LAST_MODIFIED_ON_PROCESS_COMMAND_PREF = "services.sync.clients.lastModifiedOnProcessCommands";
 
 function hasDupeCommand(commands, action) {
   if (!commands) {
     return false;
   }
   return commands.some(other => other.command == action.command &&
     Utils.deepEquals(other.args, action.args));
 }
@@ -87,16 +88,27 @@ this.ClientEngine = function ClientEngin
 ClientEngine.prototype = {
   __proto__: SyncEngine.prototype,
   _storeObj: ClientStore,
   _recordObj: ClientsRec,
   _trackerObj: ClientsTracker,
   allowSkippedRecord: false,
   _knownStaleFxADeviceIds: null,
 
+  // These two properties allow us to avoid replaying the same commands
+  // continuously if we cannot manage to upload our own record.
+  _localClientLastModified: 0,
+  get _lastModifiedOnProcessCommands() {
+    return Services.prefs.getIntPref(LAST_MODIFIED_ON_PROCESS_COMMAND_PREF, -1);
+  },
+
+  set _lastModifiedOnProcessCommands(value) {
+    Services.prefs.setIntPref(LAST_MODIFIED_ON_PROCESS_COMMAND_PREF, value);
+  },
+
   // Always sync client data as it controls other sync behavior
   get enabled() {
     return true;
   },
 
   get lastRecordUpload() {
     return Svc.Prefs.get(this.name + ".lastRecordUpload", 0);
   },
@@ -387,16 +399,17 @@ ClientEngine.prototype = {
         }
       }
       let localFxADeviceId = await fxAccounts.getDeviceId();
       // Bug 1264498: Mobile clients don't remove themselves from the clients
       // collection when the user disconnects Sync, so we mark as stale clients
       // with the same name that haven't synced in over a week.
       // (Note we can't simply delete them, or we re-apply them next sync - see
       // bug 1287687)
+      this._localClientLastModified = Math.round(this._incomingClients[this.localID]);
       delete this._incomingClients[this.localID];
       let names = new Set([this.localName]);
       let seenDeviceIds = new Set([localFxADeviceId]);
       let idToLastModifiedList = Object.entries(this._incomingClients)
                                  .sort((a, b) => b[1] - a[1]);
       for (let [id, serverLastModified] of idToLastModifiedList) {
         let record = this._store._remoteClients[id];
         // stash the server last-modified time on the record.
@@ -665,19 +678,22 @@ ClientEngine.prototype = {
 
   /**
    * Check if the local client has any remote commands and perform them.
    *
    * @return false to abort sync
    */
   async processIncomingCommands() {
     return this._notify("clients:process-commands", "", async function() {
-      if (!this.localCommands) {
+      if (!this.localCommands ||
+          (this._lastModifiedOnProcessCommands == this._localClientLastModified
+           && !this.ignoreLastModifiedOnProcessCommands)) {
         return true;
       }
+      this._lastModifiedOnProcessCommands = this._localClientLastModified;
 
       const clearedCommands = await this._readCommands()[this.localID];
       const commands = this.localCommands.filter(command => !hasDupeCommand(clearedCommands, command));
       let didRemoveCommand = false;
       let URIsToDisplay = [];
       // Process each command in order.
       for (let rawCommand of commands) {
         let shouldRemoveCommand = true; // most commands are auto-removed.
--- a/services/sync/tests/unit/test_bookmark_repair.js
+++ b/services/sync/tests/unit/test_bookmark_repair.js
@@ -33,16 +33,17 @@ const BOOKMARK_REPAIR_STATE_PREFS = [
 ];
 
 let clientsEngine;
 let bookmarksEngine;
 var recordedEvents = [];
 
 add_task(async function setup() {
   clientsEngine = Service.clientsEngine;
+  clientsEngine.ignoreLastModifiedOnProcessCommands = true;
   bookmarksEngine = Service.engineManager.get("bookmarks");
 
   await generateNewKeys(Service.collectionKeys);
 
   Service.recordTelemetryEvent = (object, method, value, extra = undefined) => {
     recordedEvents.push({ object, method, value, extra });
   };
 });
@@ -195,16 +196,17 @@ add_task(async function test_bookmark_re
       "Should not record repair telemetry after sending repair request");
 
     _("Back up repair state to restore later");
     let restoreInitialRepairState = backupPrefs(BOOKMARK_REPAIR_STATE_PREFS);
 
     // so now let's take over the role of that other client!
     _("Create new clients engine pretending to be remote client");
     let remoteClientsEngine = Service.clientsEngine = new ClientEngine(Service);
+    remoteClientsEngine.ignoreLastModifiedOnProcessCommands = true;
     await remoteClientsEngine.initialize();
     remoteClientsEngine.localID = remoteID;
 
     _("Restore missing bookmark");
     // Pretend Sync wrote the bookmark, so that we upload it as part of the
     // repair instead of the sync.
     bookmarkInfo.source = PlacesUtils.bookmarks.SOURCE_SYNC;
     await PlacesUtils.bookmarks.insert(bookmarkInfo);
@@ -302,16 +304,17 @@ add_task(async function test_bookmark_re
       },
     }]);
     await revalidationPromise;
     ok(!Services.prefs.prefHasUserValue("services.sync.repairs.bookmarks.state"),
       "Should clear repair pref after successfully completing repair");
   } finally {
     await cleanup(server);
     clientsEngine = Service.clientsEngine = new ClientEngine(Service);
+    clientsEngine.ignoreLastModifiedOnProcessCommands = true;
     clientsEngine.initialize();
   }
 });
 
 add_task(async function test_repair_client_missing() {
   enableValidationPrefs();
 
   _("Ensure that a record missing from the client only will get re-downloaded from the server");
--- a/services/sync/tests/unit/test_clients_engine.js
+++ b/services/sync/tests/unit/test_clients_engine.js
@@ -1347,17 +1347,17 @@ add_task(async function test_keep_cleare
       },
       {
         command: "displayURI",
         args: ["https://deviceclink2.com", deviceCID, "Device C link 2"],
         flowID: Utils.makeGUID(),
       }],
       version: "48",
       protocols: ["1.5"],
-    }), now - 10));
+    }), now - 5));
 
     // Simulate reboot
     SyncEngine.prototype._uploadOutgoing = oldUploadOutgoing;
     engine = Service.clientsEngine = new ClientEngine(Service);
     await engine.initialize();
 
     commandsProcessed = 0;
     engine._handleDisplayURIs = (uris) => { commandsProcessed = uris.length };
@@ -1789,13 +1789,31 @@ add_task(async function process_incoming
 
   await engine._processIncoming();
   ok(stubRefresh.notCalled, "Should not refresh the known stale clients since it's already populated");
 
   stubProcessIncoming.restore();
   stubRefresh.restore();
 });
 
+add_task(async function process_incoming_refreshes_known_stale_clients() {
+  Services.prefs.clearUserPref("services.sync.clients.lastModifiedOnProcessCommands");
+  engine._localClientLastModified = Math.round(Date.now() / 1000);
+
+  const stubRemoveLocalCommand = sinon.stub(engine, "removeLocalCommand");
+  const tabProcessedSpy = sinon.spy(engine, "_handleDisplayURIs");
+  engine.localCommands = [{ command: "displayURI", args: ["https://foo.bar", "fxaid1", "foo"] }];
+
+  await engine.processIncomingCommands();
+  ok(tabProcessedSpy.calledOnce);
+  // Let's say we failed to upload and we end up calling processIncomingCommands again
+  await engine.processIncomingCommands();
+  ok(tabProcessedSpy.calledOnce);
+
+  tabProcessedSpy.restore();
+  stubRemoveLocalCommand.restore();
+});
+
 function run_test() {
   initTestLogging("Trace");
   Log.repository.getLogger("Sync.Engine.Clients").level = Log.Level.Trace;
   run_next_test();
 }