Bug 1448929 - Fix first sync check in `gSync.syncConfiguredAndLoading`. r=eoger draft
authorKit Cambridge <kit@yakshaving.ninja>
Mon, 26 Mar 2018 09:35:29 -0700
changeset 772728 eba433ff16f9b3088958878cdc7223ec68f68e18
parent 772310 b99844d179cacf74a5d39ad23429be91e989c331
push id104036
push userbmo:kit@mozilla.com
push dateMon, 26 Mar 2018 22:18:25 +0000
reviewerseoger
bugs1448929
milestone61.0a1
Bug 1448929 - Fix first sync check in `gSync.syncConfiguredAndLoading`. r=eoger MozReview-Commit-ID: 8Xk6DMHroTm
browser/base/content/browser-sync.js
browser/base/content/test/sync/head.js
browser/base/content/test/urlbar/browser_page_action_menu.js
services/sync/modules/engines/clients.js
services/sync/tests/unit/test_clients_engine.js
--- a/browser/base/content/browser-sync.js
+++ b/browser/base/content/browser-sync.js
@@ -48,19 +48,17 @@ var gSync = {
   get syncReady() {
     return Cc["@mozilla.org/weave/service;1"].getService().wrappedJSObject.ready;
   },
 
   // Returns true if sync is configured but hasn't loaded or is yet to determine
   // if any remote clients exist.
   get syncConfiguredAndLoading() {
     return UIState.get().status == UIState.STATUS_SIGNED_IN &&
-           (!this.syncReady ||
-           // lastSync will be non-zero after the first sync
-           Weave.Service.clientsEngine.lastSync == 0);
+           (!this.syncReady || Weave.Service.clientsEngine.isFirstSync);
   },
 
   get isSignedIn() {
     return UIState.get().status == UIState.STATUS_SIGNED_IN;
   },
 
   get remoteClients() {
     return Weave.Service.clientsEngine.remoteClients
--- a/browser/base/content/test/sync/head.js
+++ b/browser/base/content/test/sync/head.js
@@ -12,14 +12,14 @@ function promiseSyncReady() {
                   .getService(Ci.nsISupports)
                   .wrappedJSObject;
   return service.whenLoaded();
 }
 
 function setupSendTabMocks({ syncReady, clientsSynced, remoteClients, state, isSendableURI }) {
   const sandbox = sinon.sandbox.create();
   sandbox.stub(gSync, "syncReady").get(() => syncReady);
-  sandbox.stub(Weave.Service.clientsEngine, "lastSync").get(() => clientsSynced ? Date.now() : 0);
+  sandbox.stub(Weave.Service.clientsEngine, "isFirstSync").get(() => !clientsSynced);
   sandbox.stub(gSync, "remoteClients").get(() => remoteClients);
   sandbox.stub(UIState, "get").returns({ status: state });
   sandbox.stub(gSync, "isSendableURI").returns(isSendableURI);
   return sandbox;
 }
--- a/browser/base/content/test/urlbar/browser_page_action_menu.js
+++ b/browser/base/content/test/urlbar/browser_page_action_menu.js
@@ -208,17 +208,17 @@ add_task(async function sendToDevice_non
 });
 
 add_task(async function sendToDevice_syncNotReady_other_states() {
   // Open a tab that's sendable.
   await BrowserTestUtils.withNewTab("http://example.com/", async () => {
     await promiseSyncReady();
     const sandbox = sinon.sandbox.create();
     sandbox.stub(gSync, "syncReady").get(() => false);
-    sandbox.stub(Weave.Service.clientsEngine, "lastSync").get(() => 0);
+    sandbox.stub(Weave.Service.clientsEngine, "isFirstSync").get(() => true);
     sandbox.stub(UIState, "get").returns({ status: UIState.STATUS_NOT_VERIFIED });
     sandbox.stub(gSync, "isSendableURI").returns(true);
 
     let cleanUp = () => {
       sandbox.restore();
     };
     registerCleanupFunction(cleanUp);
 
@@ -265,17 +265,17 @@ add_task(async function sendToDevice_syn
 });
 
 add_task(async function sendToDevice_syncNotReady_configured() {
   // Open a tab that's sendable.
   await BrowserTestUtils.withNewTab("http://example.com/", async () => {
     await promiseSyncReady();
     const sandbox = sinon.sandbox.create();
     const syncReady = sandbox.stub(gSync, "syncReady").get(() => false);
-    const lastSync = sandbox.stub(Weave.Service.clientsEngine, "lastSync").get(() => 0);
+    const lastSync = sandbox.stub(Weave.Service.clientsEngine, "isFirstSync").get(() => true);
     sandbox.stub(UIState, "get").returns({ status: UIState.STATUS_SIGNED_IN });
     sandbox.stub(gSync, "isSendableURI").returns(true);
 
     sandbox.stub(Weave.Service, "sync").callsFake(() => {
       syncReady.get(() => true);
       lastSync.get(() => Date.now());
       sandbox.stub(gSync, "remoteClients").get(() => mockRemoteClients);
     });
@@ -405,17 +405,17 @@ add_task(async function sendToDevice_not
 });
 
 add_task(async function sendToDevice_noDevices() {
   // Open a tab that's sendable.
   await BrowserTestUtils.withNewTab("http://example.com/", async () => {
     await promiseSyncReady();
     const sandbox = sinon.sandbox.create();
     sandbox.stub(gSync, "syncReady").get(() => true);
-    sandbox.stub(Weave.Service.clientsEngine, "lastSync").get(() => Date.now());
+    sandbox.stub(Weave.Service.clientsEngine, "isFirstSync").get(() => false);
     sandbox.stub(UIState, "get").returns({ status: UIState.STATUS_SIGNED_IN });
     sandbox.stub(gSync, "isSendableURI").returns(true);
     sandbox.stub(gSync, "remoteClients").get(() => []);
 
     let cleanUp = () => {
       sandbox.restore();
     };
     registerCleanupFunction(cleanUp);
@@ -470,17 +470,17 @@ add_task(async function sendToDevice_noD
 });
 
 add_task(async function sendToDevice_devices() {
   // Open a tab that's sendable.
   await BrowserTestUtils.withNewTab("http://example.com/", async () => {
     await promiseSyncReady();
     const sandbox = sinon.sandbox.create();
     sandbox.stub(gSync, "syncReady").get(() => true);
-    sandbox.stub(Weave.Service.clientsEngine, "lastSync").get(() => Date.now());
+    sandbox.stub(Weave.Service.clientsEngine, "isFirstSync").get(() => false);
     sandbox.stub(UIState, "get").returns({ status: UIState.STATUS_SIGNED_IN });
     sandbox.stub(gSync, "isSendableURI").returns(true);
     sandbox.stub(gSync, "remoteClients").get(() => mockRemoteClients);
 
     let cleanUp = () => {
       sandbox.restore();
     };
     registerCleanupFunction(cleanUp);
@@ -534,17 +534,17 @@ add_task(async function sendToDevice_dev
 });
 
 add_task(async function sendToDevice_inUrlbar() {
   // Open a tab that's sendable.
   await BrowserTestUtils.withNewTab("http://example.com/", async () => {
     await promiseSyncReady();
     const sandbox = sinon.sandbox.create();
     sandbox.stub(gSync, "syncReady").get(() => true);
-    sandbox.stub(Weave.Service.clientsEngine, "lastSync").get(() => Date.now());
+    sandbox.stub(Weave.Service.clientsEngine, "isFirstSync").get(() => false);
     sandbox.stub(UIState, "get").returns({ status: UIState.STATUS_SIGNED_IN });
     sandbox.stub(gSync, "isSendableURI").returns(true);
     sandbox.stub(gSync, "remoteClients").get(() => mockRemoteClients);
 
     let cleanUp = () => {
       sandbox.restore();
     };
     registerCleanupFunction(cleanUp);
--- a/services/sync/modules/engines/clients.js
+++ b/services/sync/modules/engines/clients.js
@@ -109,16 +109,20 @@ ClientEngine.prototype = {
   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);
   },
 
+  get isFirstSync() {
+    return !this.lastRecordUpload;
+  },
+
   // Always sync client data as it controls other sync behavior
   get enabled() {
     return true;
   },
 
   get lastRecordUpload() {
     return Svc.Prefs.get(this.name + ".lastRecordUpload", 0);
   },
@@ -358,28 +362,26 @@ ClientEngine.prototype = {
       this._log.error("Could not retrieve the FxA device list", ex);
       this._knownStaleFxADeviceIds = [];
       return;
     }
     this._knownStaleFxADeviceIds = Utils.arraySub(localClients, fxaClients);
   },
 
   async _syncStartup() {
-    this.isFirstSync = !this.lastRecordUpload;
     // Reupload new client record periodically.
     if (Date.now() / 1000 - this.lastRecordUpload > CLIENTS_TTL_REFRESH) {
       await this._tracker.addChangedID(this.localID);
-      this.lastRecordUpload = Date.now() / 1000;
     }
     return SyncEngine.prototype._syncStartup.call(this);
   },
 
   async _processIncoming() {
     // Fetch all records from the server.
-    await this.setLastSync(0);
+    await this.resetLastSync();
     this._incomingClients = {};
     try {
       await SyncEngine.prototype._processIncoming.call(this);
       // Refresh the known stale clients list at startup and when we receive
       // "device connected/disconnected" push notifications.
       if (!this._knownStaleFxADeviceIds) {
         await this._refreshKnownStaleClients();
       }
@@ -448,17 +450,19 @@ ClientEngine.prototype = {
         this._modified.set(clientId, 0);
       }
     }
     let updatedIDs = this._modified.ids();
     await SyncEngine.prototype._uploadOutgoing.call(this);
     // Record the response time as the server time for each item we uploaded.
     let lastSync = await this.getLastSync();
     for (let id of updatedIDs) {
-      if (id != this.localID) {
+      if (id == this.localID) {
+        this.lastRecordUpload = lastSync;
+      } else {
         this._store._remoteClients[id].serverLastModified = lastSync;
       }
     }
   },
 
   async _onRecordsWritten(succeeded, failed) {
     // Reconcile the status of the local records with what we just wrote on the
     // server
--- a/services/sync/tests/unit/test_clients_engine.js
+++ b/services/sync/tests/unit/test_clients_engine.js
@@ -107,20 +107,22 @@ add_task(async function test_bad_hmac() 
   try {
     await configureIdentity({username: "foo"}, server);
     await Service.login();
 
     await generateNewKeys(Service.collectionKeys);
 
     _("First sync, client record is uploaded");
     equal(engine.lastRecordUpload, 0);
+    ok(engine.isFirstSync);
     check_clients_count(0);
     await syncClientsEngine(server);
     check_clients_count(1);
     ok(engine.lastRecordUpload > 0);
+    ok(!engine.isFirstSync);
 
     // Our uploaded record has a version.
     await check_record_version(user, engine.localID);
 
     // Initial setup can wipe the server, so clean up.
     deletedCollections = [];
     deletedItems       = [];
 
@@ -243,18 +245,20 @@ add_task(async function test_full_sync()
     protocols: ["1.5"],
   }, now - 10);
 
   try {
     let store = engine._store;
 
     _("First sync. 2 records downloaded; our record uploaded.");
     strictEqual(engine.lastRecordUpload, 0);
+    ok(engine.isFirstSync);
     await syncClientsEngine(server);
     ok(engine.lastRecordUpload > 0);
+    ok(!engine.isFirstSync);
     deepEqual(user.collection("clients").keys().sort(),
               [activeID, deletedID, engine.localID].sort(),
               "Our record should be uploaded on first sync");
     let ids = await store.getAllIDs();
     deepEqual(Object.keys(ids).sort(),
               [activeID, deletedID, engine.localID].sort(),
               "Other clients should be downloaded on first sync");
 
@@ -293,38 +297,42 @@ add_task(async function test_sync() {
     return user.collection("clients").wbo(engine.localID);
   }
 
   try {
 
     _("First sync. Client record is uploaded.");
     equal(clientWBO(), undefined);
     equal(engine.lastRecordUpload, 0);
+    ok(engine.isFirstSync);
     await syncClientsEngine(server);
     ok(!!clientWBO().payload);
     ok(engine.lastRecordUpload > 0);
+    ok(!engine.isFirstSync);
 
     _("Let's time travel more than a week back, new record should've been uploaded.");
     engine.lastRecordUpload -= MORE_THAN_CLIENTS_TTL_REFRESH;
     let lastweek = engine.lastRecordUpload;
     clientWBO().payload = undefined;
     await syncClientsEngine(server);
     ok(!!clientWBO().payload);
     ok(engine.lastRecordUpload > lastweek);
+    ok(!engine.isFirstSync);
 
     _("Remove client record.");
     await engine.removeClientData();
     equal(clientWBO().payload, undefined);
 
     _("Time travel one day back, no record uploaded.");
     engine.lastRecordUpload -= LESS_THAN_CLIENTS_TTL_REFRESH;
     let yesterday = engine.lastRecordUpload;
     await syncClientsEngine(server);
     equal(clientWBO().payload, undefined);
     equal(engine.lastRecordUpload, yesterday);
+    ok(!engine.isFirstSync);
 
   } finally {
     await cleanup();
     await promiseStopServer(server);
   }
 });
 
 add_task(async function test_client_name_change() {
@@ -640,18 +648,20 @@ add_task(async function test_filter_dupl
     protocols: ["1.5"],
   }, now - 604820);
 
   try {
     let store = engine._store;
 
     _("First sync");
     strictEqual(engine.lastRecordUpload, 0);
+    ok(engine.isFirstSync);
     await syncClientsEngine(server);
     ok(engine.lastRecordUpload > 0);
+    ok(!engine.isFirstSync);
     deepEqual(user.collection("clients").keys().sort(),
               [recentID, dupeID, oldID, engine.localID].sort(),
               "Our record should be uploaded on first sync");
 
     let ids = await store.getAllIDs();
     deepEqual(Object.keys(ids).sort(),
               [recentID, dupeID, oldID, engine.localID].sort(),
               "Duplicate ID should remain in getAllIDs");
@@ -787,16 +797,17 @@ add_task(async function test_command_syn
     await engine.sendCommand("wipeAll", []);
     let clientCommands = (await engine._readCommands())[remoteId];
     equal(clientCommands.length, 1);
     await syncClientsEngine(server);
 
     _("Checking record was uploaded.");
     notEqual(clientWBO(engine.localID).payload, undefined);
     ok(engine.lastRecordUpload > 0);
+    ok(!engine.isFirstSync);
 
     notEqual(clientWBO(remoteId).payload, undefined);
 
     Svc.Prefs.set("client.GUID", remoteId);
     await engine._resetClient();
     equal(engine.localID, remoteId);
     _("Performing sync on resetted client.");
     await syncClientsEngine(server);
@@ -1097,16 +1108,17 @@ add_task(async function test_merge_comma
     }],
     version: "48",
     protocols: ["1.5"],
   }, now - 10);
 
   try {
     _("First sync. 2 records downloaded.");
     strictEqual(engine.lastRecordUpload, 0);
+    ok(engine.isFirstSync);
     await syncClientsEngine(server);
 
     _("Broadcast logout to all clients");
     await engine.sendCommand("logout", []);
     await syncClientsEngine(server);
 
     let desktopPayload = collection.cleartext(desktopID);
     compareCommands(desktopPayload.commands, [{
@@ -1150,16 +1162,17 @@ add_task(async function test_duplicate_r
     commands: [],
     version: "48",
     protocols: ["1.5"],
   }, now - 10);
 
   try {
     _("First sync. 1 record downloaded.");
     strictEqual(engine.lastRecordUpload, 0);
+    ok(engine.isFirstSync);
     await syncClientsEngine(server);
 
     _("Send tab to client");
     await engine.sendCommand("displayURI", ["https://example.com", engine.localID, "Yak Herders Anonymous"]);
     await syncClientsEngine(server);
 
     _("Simulate the desktop client consuming the command and syncing to the server");
     collection.insertRecord({
@@ -1223,16 +1236,17 @@ add_task(async function test_upload_afte
     commands: [],
     version: "48",
     protocols: ["1.5"],
   }, now - 10);
 
   try {
     _("First sync. 2 records downloaded.");
     strictEqual(engine.lastRecordUpload, 0);
+    ok(engine.isFirstSync);
     await syncClientsEngine(server);
 
     _("Send tab to client");
     await engine.sendCommand("displayURI", ["https://example.com", engine.localID, "Yak Herders Anonymous"], deviceBID);
 
     const oldUploadOutgoing = SyncEngine.prototype._uploadOutgoing;
     SyncEngine.prototype._uploadOutgoing = async () => engine._onRecordsWritten([], [deviceBID]);
     await syncClientsEngine(server);
@@ -1320,16 +1334,17 @@ add_task(async function test_keep_cleare
     commands: [],
     version: "48",
     protocols: ["1.5"],
   }, now - 10);
 
   try {
     _("First sync. Download remote and our record.");
     strictEqual(engine.lastRecordUpload, 0);
+    ok(engine.isFirstSync);
 
     const oldUploadOutgoing = SyncEngine.prototype._uploadOutgoing;
     SyncEngine.prototype._uploadOutgoing = async () => engine._onRecordsWritten([], [deviceBID]);
     let commandsProcessed = 0;
     engine._handleDisplayURIs = (uris) => { commandsProcessed = uris.length; };
 
     await syncClientsEngine(server);
     await engine.processIncomingCommands(); // Not called by the engine.sync(), gotta call it ourselves