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 385750 7b8f1bcaf50db587e235b3f03835adb48bfeb84c
parent 385749 6c7b05c7855148adba9fb99643627cbb85a6ff92
child 385751 e92ca171760d45bc16813e6f235bb7bd2c48ca81
push id53118
push usereoger@mozilla.com
push dateThu, 12 Oct 2017 03:00:34 +0000
treeherderautoland@7b8f1bcaf50d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcsc
bugs1401700
milestone58.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 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();
 }