Bug 1206571 - ensure Sync UI is in correct state before Sync initialized. r=adw, a=lizzard
authorMark Hammond <mhammond@skippinet.com.au>
Wed, 23 Sep 2015 11:00:40 +1000
changeset 296182 29a1298aad3d6b3f08f191c532527dd3b2ffc74f
parent 296181 2f4c59d36486b729ee541c5ab3cc09645e0158f9
child 296183 ccf87ddfb974518a04372d4e8ab02b189eabfbbf
push id5245
push userraliiev@mozilla.com
push dateThu, 29 Oct 2015 11:30:51 +0000
treeherdermozilla-beta@dac831dc1bd0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw, lizzard
bugs1206571
milestone43.0a2
Bug 1206571 - ensure Sync UI is in correct state before Sync initialized. r=adw, a=lizzard
browser/base/content/browser-syncui.js
browser/base/content/test/general/browser_syncui.js
browser/components/customizableui/test/browser_987185_syncButton.js
--- a/browser/base/content/browser-syncui.js
+++ b/browser/base/content/browser-syncui.js
@@ -6,16 +6,19 @@ Cu.import("resource://gre/modules/XPCOMU
 
 #ifdef MOZ_SERVICES_CLOUDSYNC
 XPCOMUtils.defineLazyModuleGetter(this, "CloudSync",
                                   "resource://gre/modules/CloudSync.jsm");
 #else
 var CloudSync = null;
 #endif
 
+XPCOMUtils.defineLazyModuleGetter(this, "fxAccounts",
+                                  "resource://gre/modules/FxAccounts.jsm");
+
 // gSyncUI handles updating the tools menu and displaying notifications.
 var gSyncUI = {
   _obs: ["weave:service:sync:start",
          "weave:service:sync:finish",
          "weave:service:sync:error",
          "weave:service:setup-complete",
          "weave:service:login:start",
          "weave:service:login:finish",
@@ -33,20 +36,17 @@ var gSyncUI = {
   // The number of "active" syncs - while this is non-zero, our button will spin
   _numActiveSyncTasks: 0,
 
   init: function () {
     Cu.import("resource://services-common/stringbundle.js");
 
     // Proceed to set up the UI if Sync has already started up.
     // Otherwise we'll do it when Sync is firing up.
-    let xps = Components.classes["@mozilla.org/weave/service;1"]
-                                .getService(Components.interfaces.nsISupports)
-                                .wrappedJSObject;
-    if (xps.ready) {
+    if (this.weaveService.ready) {
       this.initUI();
       return;
     }
 
     Services.obs.addObserver(this, "weave:service:ready", true);
 
     // Remove the observer if the window is closed before the observer
     // was triggered.
@@ -94,87 +94,95 @@ var gSyncUI = {
     // notificationbox will listen to observers from now on.
     Services.obs.removeObserver(this, "weave:notification:added");
     let idx = this._obs.indexOf("weave:notification:added");
     if (idx >= 0) {
       this._obs.splice(idx, 1);
     }
   },
 
+  // Returns a promise that resolves with true if Sync needs to be configured,
+  // false otherwise.
   _needsSetup() {
-    // We want to treat "account needs verification" as "needs setup". So
-    // "reach in" to Weave.Status._authManager to check whether we the signed-in
-    // user is verified.
-    // NOTE: We used to have this _authManager hack to avoid a nested
-    // event-loop from querying Weave.Status.checkSetup() - while that's no
-    // longer true, we do still have the FxA-specific requirement of checking
-    // the verified state - so the hack remains. We should consider refactoring
-    // Sync's "check setup" capabilities to take this into account at some point...
-    if (Weave.Status._authManager._signedInUser !== undefined) {
-      // If we have a signed in user already, and that user is not verified,
-      // revert to the "needs setup" state.
-      return !Weave.Status._authManager._signedInUser ||
-             !Weave.Status._authManager._signedInUser.verified;
+    // If Sync is configured for FxAccounts then we do that promise-dance.
+    if (this.weaveService.fxAccountsEnabled) {
+      return fxAccounts.getSignedInUser().then(user => {
+        // We want to treat "account needs verification" as "needs setup".
+        return !(user && user.verified);
+      });
     }
-
     // We are using legacy sync - check that.
     let firstSync = "";
     try {
       firstSync = Services.prefs.getCharPref("services.sync.firstSync");
     } catch (e) { }
 
-    return Weave.Status.checkSetup() == Weave.CLIENT_NOT_CONFIGURED ||
-           firstSync == "notReady";
+    return Promise.resolve(Weave.Status.checkSetup() == Weave.CLIENT_NOT_CONFIGURED ||
+                           firstSync == "notReady");
   },
 
+  // Returns a promise that resolves with true if the user currently signed in
+  // to Sync needs to be verified, false otherwise.
   _needsVerification() {
     // For callers who care about the distinction between "needs setup" and
-    // "setup but needs verification"
-    // See _needsSetup for the subtleties here.
-    if (Weave.Status._authManager._signedInUser === undefined) {
-      // a legacy sync user - no "verified" concept there.
-      return false;
+    // "needs verification"
+    if (this.weaveService.fxAccountsEnabled) {
+      return fxAccounts.getSignedInUser().then(user => {
+        // If there is no user, they can't be in a "needs verification" state.
+        if (!user) {
+          return false;
+        }
+        return !user.verified;
+      });
     }
-    if (!Weave.Status._authManager._signedInUser) {
-      // no user configured at all, so not in a "need verification" state.
-      return false;
-    }
-    return !Weave.Status._authManager._signedInUser.verified;
+
+    // Otherwise we are configured for legacy Sync, which has no verification
+    // concept.
+    return Promise.resolve(false);
   },
 
   // Note that we don't show login errors in a notification bar here, but do
   // still need to track a login-failed state so the "Tools" menu updates
   // with the correct state.
   _loginFailed: function () {
     this.log.debug("_loginFailed has sync state=${sync}",
                    { sync: Weave.Status.login});
     return Weave.Status.login == Weave.LOGIN_FAILED_LOGIN_REJECTED;
   },
 
-  updateUI: function SUI_updateUI() {
-    let needsSetup = this._needsSetup();
-    let loginFailed = this._loginFailed();
+  // Kick off an update of the UI - does *not* return a promise.
+  updateUI() {
+    this._promiseUpdateUI().catch(err => {
+      this.log.error("updateUI failed", err);
+    })
+  },
 
-    // Start off with a clean slate
-    document.getElementById("sync-reauth-state").hidden = true;
-    document.getElementById("sync-setup-state").hidden = true;
-    document.getElementById("sync-syncnow-state").hidden = true;
+  // Updates the UI - returns a promise.
+  _promiseUpdateUI() {
+    return this._needsSetup().then(needsSetup => {
+      let loginFailed = this._loginFailed();
 
-    if (CloudSync && CloudSync.ready && CloudSync().adapters.count) {
-      document.getElementById("sync-syncnow-state").hidden = false;
-    } else if (loginFailed) {
-      // unhiding this element makes the menubar show the login failure state.
-      document.getElementById("sync-reauth-state").hidden = false;
-    } else if (needsSetup) {
-      document.getElementById("sync-setup-state").hidden = false;
-    } else {
-      document.getElementById("sync-syncnow-state").hidden = false;
-    }
+      // Start off with a clean slate
+      document.getElementById("sync-reauth-state").hidden = true;
+      document.getElementById("sync-setup-state").hidden = true;
+      document.getElementById("sync-syncnow-state").hidden = true;
 
-    this._updateSyncButtonsTooltip();
+      if (CloudSync && CloudSync.ready && CloudSync().adapters.count) {
+        document.getElementById("sync-syncnow-state").hidden = false;
+      } else if (loginFailed) {
+        // unhiding this element makes the menubar show the login failure state.
+        document.getElementById("sync-reauth-state").hidden = false;
+      } else if (needsSetup) {
+        document.getElementById("sync-setup-state").hidden = false;
+      } else {
+        document.getElementById("sync-syncnow-state").hidden = false;
+      }
+
+      return this._updateSyncButtonsTooltip();
+    });
   },
 
   // Functions called by observers
   onActivityStart() {
     if (!gBrowser)
       return;
 
     this.log.debug("onActivityStart with numActive", this._numActiveSyncTasks);
@@ -235,50 +243,58 @@ var gSyncUI = {
   },
 
   _getAppName: function () {
     let brand = new StringBundle("chrome://branding/locale/brand.properties");
     return brand.get("brandShortName");
   },
 
   // Commands
-  doSync: function SUI_doSync() {
-    let needsSetup = this._needsSetup();
-
-    if (!needsSetup) {
-      setTimeout(function () Weave.Service.errorHandler.syncAndReportErrors(), 0);
-    }
-
-    Services.obs.notifyObservers(null, "cloudsync:user-sync", null);
+  // doSync forces a sync - it *does not* return a promise as it is called
+  // via the various UI components.
+  doSync() {
+    this._needsSetup().then(needsSetup => {
+      if (!needsSetup) {
+        setTimeout(function () Weave.Service.errorHandler.syncAndReportErrors(), 0);
+      }
+      Services.obs.notifyObservers(null, "cloudsync:user-sync", null);
+    }).catch(err => {
+      this.log.error("Failed to force a sync", err);
+    });
   },
 
-  handleToolbarButton: function SUI_handleStatusbarButton() {
-    if (this._needsSetup() || this._loginFailed())
-      this.openSetup();
-    else
-      this.doSync();
+  // Handle clicking the toolbar button - which either opens the Sync setup
+  // pages or forces a sync now. Does *not* return a promise as it is called
+  // via the UI.
+  handleToolbarButton() {
+    this._needsSetup().then(needsSetup => {
+      if (needsSetup || this._loginFailed()) {
+        this.openSetup();
+      } else {
+        return this.doSync();
+      }
+    }).catch(err => {
+      this.log.error("Failed to handle toolbar button command", err);
+    });
   },
 
   /**
    * Invoke the Sync setup wizard.
    *
    * @param wizardType
    *        Indicates type of wizard to launch:
    *          null    -- regular set up wizard
    *          "pair"  -- pair a device first
    *          "reset" -- reset sync
    * @param entryPoint
    *        Indicates the entrypoint from where this method was called.
    */
 
   openSetup: function SUI_openSetup(wizardType, entryPoint = "syncbutton") {
-    let xps = Components.classes["@mozilla.org/weave/service;1"]
-                                .getService(Components.interfaces.nsISupports)
-                                .wrappedJSObject;
-    if (xps.fxAccountsEnabled) {
+    if (this.weaveService.fxAccountsEnabled) {
       // If the user is also in an uitour, set the entrypoint to `uitour`
       if (UITour.tourBrowsersByWindow.get(window) &&
           UITour.tourBrowsersByWindow.get(window).has(gBrowser.selectedBrowser)) {
         entryPoint = "uitour";
       }
       this.openPrefs(entryPoint);
     } else {
       let win = Services.wm.getMostRecentWindow("Weave:AccountSetup");
@@ -287,16 +303,17 @@ var gSyncUI = {
       else {
         window.openDialog("chrome://browser/content/sync/setup.xul",
                           "weaveSetup", "centerscreen,chrome,resizable=no",
                           wizardType);
       }
     }
   },
 
+  // Open the legacy-sync device pairing UI. Note used for FxA Sync.
   openAddDevice: function () {
     if (!Weave.Utils.ensureMPUnlocked())
       return;
 
     let win = Services.wm.getMostRecentWindow("Sync:AddDevice");
     if (win)
       win.focus();
     else
@@ -313,39 +330,39 @@ var gSyncUI = {
   },
 
   /* Update the tooltip for the Sync Toolbar button and the Sync spinner in the
      FxA hamburger area.
      If Sync is configured, the tooltip is when the last sync occurred,
      otherwise the tooltip reflects the fact that Sync needs to be
      (re-)configured.
   */
-  _updateSyncButtonsTooltip: function() {
+  _updateSyncButtonsTooltip: Task.async(function* () {
     if (!gBrowser)
       return;
 
-    let syncButton = document.getElementById("sync-button");
-    let statusButton = document.getElementById("PanelUI-fxa-icon");
-
     let email;
     try {
       email = Services.prefs.getCharPref("services.sync.username");
     } catch (ex) {}
 
+    let needsSetup = yield this._needsSetup();
+    let needsVerification = yield this._needsVerification();
+    let loginFailed = this._loginFailed();
     // This is a little messy as the Sync buttons are 1/2 Sync related and
     // 1/2 FxA related - so for some strings we use Sync strings, but for
     // others we reach into gFxAccounts for strings.
     let tooltiptext;
-    if (this._needsVerification()) {
+    if (needsVerification) {
       // "needs verification"
       tooltiptext = gFxAccounts.strings.formatStringFromName("verifyDescription", [email], 1);
-    } else if (this._needsSetup()) {
+    } else if (needsSetup) {
       // "needs setup".
       tooltiptext = this._stringBundle.GetStringFromName("signInToSync.description");
-    } else if (this._loginFailed()) {
+    } else if (loginFailed) {
       // "need to reconnect/re-enter your password"
       tooltiptext = gFxAccounts.strings.formatStringFromName("reconnectDescription", [email], 1);
     } else {
       // Sync appears configured - format the "last synced at" time.
       try {
         let lastSync = new Date(Services.prefs.getCharPref("services.sync.lastSync"));
         // Show the day-of-week and time (HH:MM) of last sync
         let lastSyncDateString = lastSync.toLocaleFormat("%a %H:%M");
@@ -353,26 +370,34 @@ var gSyncUI = {
       }
       catch (e) {
         // pref doesn't exist (which will be the case until we've seen the
         // first successful sync) or is invalid (which should be impossible!)
         // Just leave tooltiptext as the empty string in these cases, which
         // will cause the tooltip to be removed below.
       }
     }
+
+    // We've done all our promise-y work and ready to update the UI - make
+    // sure it hasn't been torn down since we started.
+    if (!gBrowser)
+      return;
+    let syncButton = document.getElementById("sync-button");
+    let statusButton = document.getElementById("PanelUI-fxa-icon");
+
     for (let button of [syncButton, statusButton]) {
       if (button) {
         if (tooltiptext) {
           button.setAttribute("tooltiptext", tooltiptext);
         } else {
           button.removeAttribute("tooltiptext");
         }
       }
     }
-  },
+  }),
 
   clearError: function SUI_clearError(errorString) {
     Weave.Notifications.removeAll(errorString);
     this.updateUI();
   },
 
   onSyncFinish: function SUI_onSyncFinish() {
     let title = this._stringBundle.GetStringFromName("error.sync.title");
@@ -454,8 +479,14 @@ XPCOMUtils.defineLazyGetter(gSyncUI, "_s
   return Cc["@mozilla.org/intl/stringbundle;1"].
          getService(Ci.nsIStringBundleService).
          createBundle("chrome://weave/locale/services/sync.properties");
 });
 
 XPCOMUtils.defineLazyGetter(gSyncUI, "log", function() {
   return Log.repository.getLogger("browserwindow.syncui");
 });
+
+XPCOMUtils.defineLazyGetter(gSyncUI, "weaveService", function() {
+  return Components.classes["@mozilla.org/weave/service;1"]
+                   .getService(Components.interfaces.nsISupports)
+                   .wrappedJSObject;
+});
--- a/browser/base/content/test/general/browser_syncui.js
+++ b/browser/base/content/test/general/browser_syncui.js
@@ -7,16 +7,35 @@ var {Notifications} = Cu.import("resourc
 
 var stringBundle = Cc["@mozilla.org/intl/stringbundle;1"]
                    .getService(Ci.nsIStringBundleService)
                    .createBundle("chrome://weave/locale/services/sync.properties");
 
 // ensure test output sees log messages.
 Log.repository.getLogger("browserwindow.syncui").addAppender(new Log.DumpAppender());
 
+// Send the specified sync-related notification and return a promise that
+// resolves once gSyncUI._promiseUpateUI is complete and the UI is ready to check.
+function notifyAndPromiseUIUpdated(topic) {
+  return new Promise(resolve => {
+    // Instrument gSyncUI so we know when the update is complete.
+    let oldPromiseUpdateUI = gSyncUI._promiseUpdateUI.bind(gSyncUI);
+    gSyncUI._promiseUpdateUI = function() {
+      return oldPromiseUpdateUI().then(() => {
+        // Restore our override.
+        gSyncUI._promiseUpdateUI = oldPromiseUpdateUI;
+        // Resolve the promise so the caller knows the update is done.
+        resolve();
+      });
+    };
+    // Now send the notification.
+    Services.obs.notifyObservers(null, topic, null);
+  });
+}
+
 // Sync manages 3 broadcasters so the menus correctly reflect the Sync state.
 // Only one of these 3 should ever be visible - pass the ID of the broadcaster
 // you expect to be visible and it will check it's the only one that is.
 function checkBroadcasterVisible(broadcasterId) {
   let all = ["sync-reauth-state", "sync-setup-state", "sync-syncnow-state"];
   Assert.ok(all.indexOf(broadcasterId) >= 0, "valid id");
   for (let check of all) {
     let eltHidden = document.getElementById(check).hidden;
@@ -50,69 +69,69 @@ add_task(function* prepare() {
   });
 
   let xps = Components.classes["@mozilla.org/weave/service;1"]
                               .getService(Components.interfaces.nsISupports)
                               .wrappedJSObject;
   yield xps.whenLoaded();
   // Put Sync and the UI into a known state.
   Weave.Status.login = Weave.LOGIN_FAILED_NO_USERNAME;
-  Services.obs.notifyObservers(null, "weave:ui:clear-error", null);
+  yield notifyAndPromiseUIUpdated("weave:ui:clear-error");
 
   checkBroadcasterVisible("sync-setup-state");
   checkButtonTooltips("Sign In To Sync");
   // mock out the "_needsSetup()" function so we don't short-circuit.
   let oldNeedsSetup = window.gSyncUI._needsSetup;
-  window.gSyncUI._needsSetup = () => false;
+  window.gSyncUI._needsSetup = () => Promise.resolve(false);
   registerCleanupFunction(() => {
     window.gSyncUI._needsSetup = oldNeedsSetup;
     // and an observer to set the state back to what it should be now we've
     // restored the stub.
     Services.obs.notifyObservers(null, "weave:ui:clear-error", null);
   });
   // and a notification to have the state change away from "needs setup"
-  Services.obs.notifyObservers(null, "weave:ui:clear-error", null);
+  yield notifyAndPromiseUIUpdated("weave:ui:clear-error");
   checkBroadcasterVisible("sync-syncnow-state");
 });
 
 add_task(function* testSyncNeedsVerification() {
   Assert.equal(Notifications.notifications.length, 0, "start with no notifications");
   // mock out the "_needsVerification()" function
   let oldNeedsVerification = window.gSyncUI._needsVerification;
   window.gSyncUI._needsVerification = () => true;
   try {
     // a notification for the state change
-    Services.obs.notifyObservers(null, "weave:ui:clear-error", null);
+    yield notifyAndPromiseUIUpdated("weave:ui:clear-error");
     checkButtonTooltips("Verify");
   } finally {
     window.gSyncUI._needsVerification = oldNeedsVerification;
   }
 });
 
 
 add_task(function* testSyncLoginError() {
   Assert.equal(Notifications.notifications.length, 0, "start with no notifications");
   checkBroadcasterVisible("sync-syncnow-state");
 
   // Pretend we are in a "login failed" error state
   Weave.Status.sync = Weave.LOGIN_FAILED;
   Weave.Status.login = Weave.LOGIN_FAILED_LOGIN_REJECTED;
-  Services.obs.notifyObservers(null, "weave:ui:sync:error", null);
+  yield notifyAndPromiseUIUpdated("weave:ui:sync:error");
 
   Assert.equal(Notifications.notifications.length, 0, "no notifications shown on login error");
   // But the menu *should* reflect the login error.
   checkBroadcasterVisible("sync-reauth-state");
   // The tooltips for the buttons should also reflect it.
   checkButtonTooltips("Reconnect");
 
   // Now pretend we just had a successful login - the error notification should go away.
   Weave.Status.sync = Weave.STATUS_OK;
   Weave.Status.login = Weave.LOGIN_SUCCEEDED;
-  Services.obs.notifyObservers(null, "weave:service:login:start", null);
-  Services.obs.notifyObservers(null, "weave:service:login:finish", null);
+  yield notifyAndPromiseUIUpdated("weave:service:login:start");
+  yield notifyAndPromiseUIUpdated("weave:service:login:finish");
   Assert.equal(Notifications.notifications.length, 0, "no notifications left");
   // The menus should be back to "all good"
   checkBroadcasterVisible("sync-syncnow-state");
 });
 
 function checkButtonsStatus(shouldBeActive) {
   let button = document.getElementById("sync-button");
   let fxaContainer = document.getElementById("PanelUI-footer-fxa");
@@ -120,51 +139,51 @@ function checkButtonsStatus(shouldBeActi
     Assert.equal(button.getAttribute("status"), "active");
     Assert.equal(fxaContainer.getAttribute("syncstatus"), "active");
   } else {
     Assert.ok(!button.hasAttribute("status"));
     Assert.ok(!fxaContainer.hasAttribute("syncstatus"));
   }
 }
 
-function testButtonActions(startNotification, endNotification) {
+function* testButtonActions(startNotification, endNotification) {
   checkButtonsStatus(false);
   // pretend a sync is starting.
-  Services.obs.notifyObservers(null, startNotification, null);
+  yield notifyAndPromiseUIUpdated(startNotification);
   checkButtonsStatus(true);
   // and has stopped
-  Services.obs.notifyObservers(null, endNotification, null);
+  yield notifyAndPromiseUIUpdated(endNotification);
   checkButtonsStatus(false);
 }
 
-function doTestButtonActivities() {
-  testButtonActions("weave:service:login:start", "weave:service:login:finish");
-  testButtonActions("weave:service:login:start", "weave:service:login:error");
+function *doTestButtonActivities() {
+  yield testButtonActions("weave:service:login:start", "weave:service:login:finish");
+  yield testButtonActions("weave:service:login:start", "weave:service:login:error");
 
-  testButtonActions("weave:service:sync:start", "weave:service:sync:finish");
-  testButtonActions("weave:service:sync:start", "weave:service:sync:error");
+  yield testButtonActions("weave:service:sync:start", "weave:service:sync:finish");
+  yield testButtonActions("weave:service:sync:start", "weave:service:sync:error");
 
   // and ensure the counters correctly handle multiple in-flight syncs
-  Services.obs.notifyObservers(null, "weave:service:sync:start", null);
+  yield notifyAndPromiseUIUpdated("weave:service:sync:start");
   checkButtonsStatus(true);
   // sync stops.
-  Services.obs.notifyObservers(null, "weave:service:sync:finish", null);
+  yield notifyAndPromiseUIUpdated("weave:service:sync:finish");
   // Button should not be active.
   checkButtonsStatus(false);
 }
 
 add_task(function* testButtonActivitiesInNavBar() {
   // check the button's functionality while the button is in the NavBar - which
   // it already is.
-  doTestButtonActivities();
+  yield doTestButtonActivities();
 });
 
 add_task(function* testButtonActivitiesInPanel() {
   // check the button's functionality while the button is in the panel - it's
   // currently in the NavBar - move it to the panel and open it.
   CustomizableUI.addWidgetToArea("sync-button", CustomizableUI.AREA_PANEL);
   yield PanelUI.show();
   try {
-    doTestButtonActivities();
+    yield doTestButtonActivities();
   } finally {
     PanelUI.hide();
   }
 });
--- a/browser/components/customizableui/test/browser_987185_syncButton.js
+++ b/browser/components/customizableui/test/browser_987185_syncButton.js
@@ -43,17 +43,17 @@ add_task(function* asyncCleanup() {
     yield panelHidePromise;
   }
 
   restoreValues();
 });
 
 function mockFunctions() {
   // mock needsSetup
-  gSyncUI._needsSetup = function() false;
+  gSyncUI._needsSetup = function() Promise.resolve(false);
 
   // mock service.errorHandler.syncAndReportErrors()
   service.errorHandler.syncAndReportErrors = mocked_syncAndReportErrors;
 }
 
 function mocked_syncAndReportErrors() {
   syncWasCalled = true;
 }