From d8c3cc3c0b4527aea0531920e7acd7c264cbf30e Mon Sep 17 00:00:00 2001
From: Edouard Oger <eoger@fastmail.com>
Date: Tue, 3 Oct 2017 14:45:11 -0400
Subject: [PATCH] Bug 1401700 - Prevent incoming tabs from opening multiple
 times if client sync fails. r=tcsc

MozReview-Commit-ID: DhrZ1oFY2WG

--HG--
extra : rebase_source : d8d35bdb08edbad9412d6953a671c3e38e75711c
---
 services/sync/modules/engines/clients.js      | 18 ++++++++++++++++-
 .../sync/tests/unit/test_bookmark_repair.js   |  3 +++
 .../sync/tests/unit/test_clients_engine.js    | 20 ++++++++++++++++++-
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/services/sync/modules/engines/clients.js b/services/sync/modules/engines/clients.js
index fe167b16fa48..614c410a8cef 100644
--- a/services/sync/modules/engines/clients.js
+++ b/services/sync/modules/engines/clients.js
@@ -50,6 +50,7 @@ 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) {
@@ -92,6 +93,17 @@ ClientEngine.prototype = {
   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;
@@ -392,6 +404,7 @@ ClientEngine.prototype = {
       // 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]);
@@ -670,9 +683,12 @@ ClientEngine.prototype = {
    */
   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));
diff --git a/services/sync/tests/unit/test_bookmark_repair.js b/services/sync/tests/unit/test_bookmark_repair.js
index 0538ebc54558..282afe75d54f 100644
--- a/services/sync/tests/unit/test_bookmark_repair.js
+++ b/services/sync/tests/unit/test_bookmark_repair.js
@@ -38,6 +38,7 @@ var recordedEvents = [];
 
 add_task(async function setup() {
   clientsEngine = Service.clientsEngine;
+  clientsEngine.ignoreLastModifiedOnProcessCommands = true;
   bookmarksEngine = Service.engineManager.get("bookmarks");
 
   await generateNewKeys(Service.collectionKeys);
@@ -200,6 +201,7 @@ add_task(async function test_bookmark_repair_integration() {
     // 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;
 
@@ -307,6 +309,7 @@ add_task(async function test_bookmark_repair_integration() {
   } finally {
     await cleanup(server);
     clientsEngine = Service.clientsEngine = new ClientEngine(Service);
+    clientsEngine.ignoreLastModifiedOnProcessCommands = true;
     clientsEngine.initialize();
   }
 });
diff --git a/services/sync/tests/unit/test_clients_engine.js b/services/sync/tests/unit/test_clients_engine.js
index 5b4a8f4d00de..ea0378dab764 100644
--- a/services/sync/tests/unit/test_clients_engine.js
+++ b/services/sync/tests/unit/test_clients_engine.js
@@ -1352,7 +1352,7 @@ add_task(async function test_keep_cleared_commands_after_reboot() {
       }],
       version: "48",
       protocols: ["1.5"],
-    }), now - 10));
+    }), now - 5));
 
     // Simulate reboot
     SyncEngine.prototype._uploadOutgoing = oldUploadOutgoing;
@@ -1794,6 +1794,24 @@ add_task(async function process_incoming_refreshes_known_stale_clients() {
   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;