Bug 1401700 - Prevent incoming tabs from opening multiple times if client sync fails. r=tcsc
☠☠ backed out by b9dd71f3f20f ☠ ☠
authorEdouard Oger <eoger@fastmail.com>
Tue, 03 Oct 2017 14:45:11 -0400
changeset 679110 8d5cc47b248f5b6c995fc836b076a3c0d1655313
parent 679109 aca965f290d3a516d1808f7fbf64e98e08bdb967
child 679111 e029590936c56ac269d68610ab389ab25887b6e1
push id84141
push userbmo:schien@mozilla.com
push dateThu, 12 Oct 2017 11:13:04 +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_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,21 @@ 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) {
         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_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();
 }