Bug 1190279 - fix UI for unverified FxA users in both hamburger menu and Sync prefs pane. r=oeger
☠☠ backed out by 0f5e2bbf8c1c ☠ ☠
authorMark Hammond <mhammond@skippinet.com.au>
Wed, 05 Aug 2015 14:22:20 +1000
changeset 287810 ad37329e81ced1f0480fb59572d6a2855cc0e86b
parent 287809 80b69899262f9f0c7976dc2d22500e8f547fc96c
child 287811 7f50f6fe0f3db7644291b89f43f823d7d3fb9ad5
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersoeger
bugs1190279
milestone42.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 1190279 - fix UI for unverified FxA users in both hamburger menu and Sync prefs pane. r=oeger
browser/base/content/browser-fxaccounts.js
browser/base/content/test/general/browser.ini
browser/base/content/test/general/browser_fxaccounts.js
browser/base/content/test/general/fxa_profile_handler.sjs
browser/components/preferences/in-content/sync.js
--- a/browser/base/content/browser-fxaccounts.js
+++ b/browser/base/content/browser-fxaccounts.js
@@ -28,16 +28,17 @@ let gFxAccounts = {
     delete this.topics;
     return this.topics = [
       "weave:service:ready",
       "weave:service:sync:start",
       "weave:service:login:error",
       "weave:service:setup-complete",
       "weave:ui:login:error",
       "fxa-migration:state-changed",
+      this.FxAccountsCommon.ONLOGIN_NOTIFICATION,
       this.FxAccountsCommon.ONVERIFIED_NOTIFICATION,
       this.FxAccountsCommon.ONLOGOUT_NOTIFICATION,
       "weave:notification:removed",
       this.FxAccountsCommon.ON_PROFILE_CHANGE_NOTIFICATION,
     ];
   },
 
   get panelUIFooter() {
@@ -217,36 +218,37 @@ let gFxAccounts = {
     this.showDoorhanger("sync-error-panel");
   },
 
   updateUI: function () {
     this.updateAppMenuItem();
     this.updateMigrationNotification();
   },
 
+  // Note that updateAppMenuItem() returns a Promise that's only used by tests.
   updateAppMenuItem: function () {
     if (this._migrationInfo) {
       this.updateAppMenuItemForMigration();
-      return;
+      return Promise.resolve();
     }
 
     let profileInfoEnabled = false;
     try {
       profileInfoEnabled = Services.prefs.getBoolPref("identity.fxaccounts.profile_image.enabled");
     } catch (e) { }
 
     // Bail out if FxA is disabled.
     if (!this.weave.fxAccountsEnabled) {
       // When migration transitions from needs-verification to the null state,
       // fxAccountsEnabled is false because migration has not yet finished.  In
       // that case, hide the button.  We'll get another notification with a null
       // state once migration is complete.
       this.panelUIFooter.hidden = true;
       this.panelUIFooter.removeAttribute("fxastatus");
-      return;
+      return Promise.resolve();
     }
 
     this.panelUIFooter.hidden = false;
 
     // Make sure the button is disabled in customization mode.
     if (this._inCustomizationMode) {
       this.panelUIStatus.setAttribute("disabled", "true");
       this.panelUILabel.setAttribute("disabled", "true");
@@ -306,33 +308,39 @@ let gFxAccounts = {
             this.panelUIFooter.setAttribute("fxaprofileimage", "set");
             this.panelUIAvatar.style.listStyleImage = "url('" + profile.avatar + "')";
           };
           img.src = profile.avatar;
         }
       }
     }
 
-    // Calling getSignedInUserProfile() without a user logged in causes log
-    // noise that looks like an actual error...
-    fxAccounts.getSignedInUser().then(userData => {
+    return fxAccounts.getSignedInUser().then(userData => {
       // userData may be null here when the user is not signed-in, but that's expected
       updateWithUserData(userData);
-      return userData ? fxAccounts.getSignedInUserProfile() : null;
+      // unverified users cause us to spew log errors fetching an OAuth token
+      // to fetch the profile, so don't even try in that case.
+      if (!userData || !userData.verified || !profileInfoEnabled) {
+        return null; // don't even try to grab the profile.
+      }
+      return fxAccounts.getSignedInUserProfile().catch(err => {
+        // Not fetching the profile is sad but the FxA logs will already have noise.
+        return null;
+      });
     }).then(profile => {
       if (!profile) {
         return;
       }
       updateWithProfile(profile);
     }).catch(error => {
       // This is most likely in tests, were we quickly log users in and out.
       // The most likely scenario is a user logged out, so reflect that.
       // Bug 995134 calls for better errors so we could retry if we were
       // sure this was the failure reason.
-      this.FxAccountsCommon.log.error("Error updating FxA profile", error);
+      this.FxAccountsCommon.log.error("Error updating FxA account info", error);
       updateWithUserData(null);
     });
   },
 
   updateAppMenuItemForMigration: Task.async(function* () {
     let status = null;
     let label = null;
     switch (this._migrationInfo.state) {
--- a/browser/base/content/test/general/browser.ini
+++ b/browser/base/content/test/general/browser.ini
@@ -298,16 +298,18 @@ skip-if = e10s
 [browser_drag.js]
 skip-if = true # browser_drag.js is disabled, as it needs to be updated for the new behavior from bug 320638.
 [browser_favicon_change.js]
 [browser_favicon_change_not_in_document.js]
 [browser_findbarClose.js]
 [browser_focusonkeydown.js]
 [browser_fullscreen-window-open.js]
 skip-if = buildapp == 'mulet' || e10s || os == "linux" # Bug 933103 - mochitest's EventUtils.synthesizeMouse functions not e10s friendly. Linux: Intermittent failures - bug 941575.
+[browser_fxaccounts.js]
+support-files = fxa_profile_handler.sjs
 [browser_fxa_migrate.js]
 [browser_fxa_oauth.js]
 [browser_fxa_web_channel.js]
 [browser_gestureSupport.js]
 skip-if = e10s # Bug 863514 - no gesture support.
 [browser_getshortcutoruri.js]
 [browser_hide_removing.js]
 [browser_homeDrop.js]
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/general/browser_fxaccounts.js
@@ -0,0 +1,258 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+let {Log} = Cu.import("resource://gre/modules/Log.jsm", {});
+let {Task} = Cu.import("resource://gre/modules/Task.jsm", {});
+let {fxAccounts} = Cu.import("resource://gre/modules/FxAccounts.jsm", {});
+let FxAccountsCommon = {};
+Cu.import("resource://gre/modules/FxAccountsCommon.js", FxAccountsCommon);
+
+const TEST_ROOT = "http://example.com/browser/browser/base/content/test/general/";
+
+// instrument gFxAccounts to send observer notifications when it's done
+// what it does.
+(function() {
+  let unstubs = {}; // The original functions we stub out.
+
+  // The stub functions.
+  let stubs = {
+    updateAppMenuItem: function() {
+      return unstubs['updateAppMenuItem'].call(gFxAccounts).then(() => {
+        Services.obs.notifyObservers(null, "test:browser_fxaccounts:updateAppMenuItem", null);
+      });
+    },
+    // Opening preferences is trickier than it should be as leaks are reported
+    // due to the promises it fires off at load time  and there's no clear way to
+    // know when they are done.
+    // So just ensure openPreferences is called rather than whether it opens.
+    openPreferences: function() {
+      Services.obs.notifyObservers(null, "test:browser_fxaccounts:openPreferences", null);
+    }
+  };
+
+  for (let name in stubs) {
+    unstubs[name] = gFxAccounts[name];
+    gFxAccounts[name] = stubs[name];
+  }
+  // and undo our damage at the end.
+  registerCleanupFunction(() => {
+    for (let name in unstubs) {
+      gFxAccounts[name] = unstubs[name];
+    }
+    stubs = unstubs = null;
+  });
+})();
+
+// Other setup/cleanup
+let newTab;
+
+Services.prefs.setCharPref("identity.fxaccounts.remote.signup.uri",
+                           TEST_ROOT + "accounts_testRemoteCommands.html");
+
+registerCleanupFunction(() => {
+  Services.prefs.clearUserPref("identity.fxaccounts.remote.signup.uri");
+  Services.prefs.clearUserPref("identity.fxaccounts.remote.profile.uri");
+  gBrowser.removeTab(newTab);
+});
+
+add_task(function* initialize() {
+  // Set a new tab with something other than about:blank, so it doesn't get reused.
+  // We must wait for it to load or the promiseTabOpen() call in the next test
+  // gets confused.
+  newTab = gBrowser.selectedTab = gBrowser.addTab("about:mozilla", {animate: false});
+  yield promiseTabLoaded(newTab);
+});
+
+// The elements we care about.
+let panelUILabel = document.getElementById("PanelUI-fxa-label");
+let panelUIStatus = document.getElementById("PanelUI-fxa-status");
+let panelUIFooter = document.getElementById("PanelUI-footer-fxa");
+
+// The tests
+add_task(function* test_nouser() {
+  let user = yield fxAccounts.getSignedInUser();
+  Assert.strictEqual(user, null, "start with no user signed in");
+  let promiseUpdateDone = promiseObserver("test:browser_fxaccounts:updateAppMenuItem");
+  Services.obs.notifyObservers(null, this.FxAccountsCommon.ONLOGOUT_NOTIFICATION, null);
+  yield promiseUpdateDone;
+
+  // Check the world - the FxA footer area is visible as it is offering a signin.
+  Assert.ok(isFooterVisible())
+
+  Assert.equal(panelUILabel.getAttribute("label"), panelUIStatus.getAttribute("defaultlabel"));
+  Assert.ok(!panelUIStatus.hasAttribute("tooltiptext"), "no tooltip when signed out");
+  Assert.ok(!panelUIFooter.hasAttribute("fxastatus"), "no fxsstatus when signed out");
+  Assert.ok(!panelUIFooter.hasAttribute("fxaprofileimage"), "no fxaprofileimage when signed out");
+
+  let promiseOpen = promiseTabOpen("about:accounts?entryPoint=menupanel");
+  panelUIStatus.click();
+  yield promiseOpen;
+});
+
+/*
+XXX - Bug 1191162 - need a better hawk mock story or this will leak in debug builds.
+
+add_task(function* test_unverifiedUser() {
+  let promiseUpdateDone = promiseObserver("test:browser_fxaccounts:updateAppMenuItem");
+  yield setSignedInUser(false); // this will fire the observer that does the update.
+  yield promiseUpdateDone;
+
+  // Check the world.
+  Assert.ok(isFooterVisible())
+
+  Assert.equal(panelUILabel.getAttribute("label"), "foo@example.com");
+  Assert.equal(panelUIStatus.getAttribute("tooltiptext"),
+               panelUIStatus.getAttribute("signedinTooltiptext"));
+  Assert.equal(panelUIFooter.getAttribute("fxastatus"), "signedin");
+  let promisePreferencesOpened = promiseObserver("test:browser_fxaccounts:openPreferences");
+  panelUIStatus.click();
+  yield promisePreferencesOpened
+  yield signOut();
+});
+*/
+
+add_task(function* test_verifiedUserEmptyProfile() {
+  // We see 2 updateAppMenuItem() calls - one for the signedInUser and one after
+  // we first fetch the profile. We want them both to fire or we aren't testing
+  // the state we think we are testing.
+  let promiseUpdateDone = promiseObserver("test:browser_fxaccounts:updateAppMenuItem", 2);
+  configureProfileURL({}); // successful but empty profile.
+  yield setSignedInUser(true); // this will fire the observer that does the update.
+  yield promiseUpdateDone;
+
+  // Check the world.
+  Assert.ok(isFooterVisible())
+  Assert.equal(panelUILabel.getAttribute("label"), "foo@example.com");
+  Assert.equal(panelUIStatus.getAttribute("tooltiptext"),
+               panelUIStatus.getAttribute("signedinTooltiptext"));
+  Assert.equal(panelUIFooter.getAttribute("fxastatus"), "signedin");
+
+  let promisePreferencesOpened = promiseObserver("test:browser_fxaccounts:openPreferences");
+  panelUIStatus.click();
+  yield promisePreferencesOpened;
+  yield signOut();
+});
+
+add_task(function* test_verifiedUserDisplayName() {
+  let promiseUpdateDone = promiseObserver("test:browser_fxaccounts:updateAppMenuItem", 2);
+  configureProfileURL({ displayName: "Test User Display Name" });
+  yield setSignedInUser(true); // this will fire the observer that does the update.
+  yield promiseUpdateDone;
+
+  Assert.ok(isFooterVisible())
+  Assert.equal(panelUILabel.getAttribute("label"), "Test User Display Name");
+  Assert.equal(panelUIStatus.getAttribute("tooltiptext"),
+               panelUIStatus.getAttribute("signedinTooltiptext"));
+  Assert.equal(panelUIFooter.getAttribute("fxastatus"), "signedin");
+  yield signOut();
+});
+
+add_task(function* test_verifiedUserProfileFailure() {
+  // profile failure means only one observer fires.
+  let promiseUpdateDone = promiseObserver("test:browser_fxaccounts:updateAppMenuItem", 1);
+  configureProfileURL(null, 500);
+  yield setSignedInUser(true); // this will fire the observer that does the update.
+  yield promiseUpdateDone;
+
+  Assert.ok(isFooterVisible())
+  Assert.equal(panelUILabel.getAttribute("label"), "foo@example.com");
+  Assert.equal(panelUIStatus.getAttribute("tooltiptext"),
+               panelUIStatus.getAttribute("signedinTooltiptext"));
+  Assert.equal(panelUIFooter.getAttribute("fxastatus"), "signedin");
+  yield signOut();
+});
+
+// Helpers.
+function isFooterVisible() {
+  let style = window.getComputedStyle(panelUIFooter);
+  return style.getPropertyValue("display") == "flex";
+}
+
+function configureProfileURL(profile, responseStatus = 200) {
+  let responseBody = profile ? JSON.stringify(profile) : "";
+  let url = TEST_ROOT + "fxa_profile_handler.sjs?" +
+            "responseStatus=" + responseStatus +
+            "responseBody=" + responseBody +
+            // This is a bit cheeky - the FxA code will just append "/profile"
+            // to the preference value. We arrange for this to be seen by our
+            //.sjs as part of the query string.
+            "&path=";
+
+  Services.prefs.setCharPref("identity.fxaccounts.remote.profile.uri", url);
+}
+
+function promiseObserver(topic, count = 1) {
+  return new Promise(resolve => {
+    let obs = (subject, topic, data) => {
+      if (--count == 0) {
+        Services.obs.removeObserver(obs, topic);
+        resolve(subject);
+      }
+    }
+    Services.obs.addObserver(obs, topic, false);
+  });
+}
+
+// Stolen from browser_aboutHome.js
+function promiseWaitForEvent(node, type, capturing) {
+  return new Promise((resolve) => {
+    node.addEventListener(type, function listener(event) {
+      node.removeEventListener(type, listener, capturing);
+      resolve(event);
+    }, capturing);
+  });
+}
+
+let promiseTabOpen = Task.async(function*(urlBase) {
+  info("Waiting for tab to open...");
+  let event = yield promiseWaitForEvent(gBrowser.tabContainer, "TabOpen", true);
+  let tab = event.target;
+  yield promiseTabLoadEvent(tab);
+  ok(tab.linkedBrowser.currentURI.spec.startsWith(urlBase),
+     "Got " + tab.linkedBrowser.currentURI.spec + ", expecting " + urlBase);
+  let whenUnloaded = promiseTabUnloaded(tab);
+  gBrowser.removeTab(tab);
+  yield whenUnloaded;
+});
+
+function promiseTabUnloaded(tab)
+{
+  return new Promise(resolve => {
+    info("Wait for tab to unload");
+    function handle(event) {
+      tab.linkedBrowser.removeEventListener("unload", handle, true);
+      info("Got unload event");
+      resolve(event);
+    }
+    tab.linkedBrowser.addEventListener("unload", handle, true, true);
+  });
+}
+
+// FxAccounts helpers.
+function setSignedInUser(verified) {
+  let data = {
+    email: "foo@example.com",
+    uid: "1234@lcip.org",
+    assertion: "foobar",
+    sessionToken: "dead",
+    kA: "beef",
+    kB: "cafe",
+    verified: verified,
+
+    oauthTokens: {
+      // a token for the profile server.
+      profile: "key value",
+    }
+  }
+  return fxAccounts.setSignedInUser(data);
+}
+
+let signOut = Task.async(function* () {
+  // This test needs to make sure that any updates for the logout have
+  // completed before starting the next test, or we see the observer
+  // notifications get out of sync.
+  let promiseUpdateDone = promiseObserver("test:browser_fxaccounts:updateAppMenuItem");
+  // we always want a "localOnly" signout here...
+  yield fxAccounts.signOut(true);
+  yield promiseUpdateDone;
+});
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/general/fxa_profile_handler.sjs
@@ -0,0 +1,34 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// This is basically an echo server!
+// We just grab responseStatus and responseBody query params!
+
+function reallyHandleRequest(request, response) {
+  var query = "?" + request.queryString;
+
+  var responseStatus = 200;
+  var match = /responseStatus=([^&]*)/.exec(query);
+  if (match) {
+    responseStatus = parseInt(match[1]);
+  }
+
+  var responseBody = "";
+  match = /responseBody=([^&]*)/.exec(query);
+  if (match) {
+    responseBody = decodeURIComponent(match[1]);
+  }
+
+  response.setStatusLine("1.0", responseStatus, "OK");
+  response.write(responseBody);
+}
+
+function handleRequest(request, response)
+{
+  try {
+    reallyHandleRequest(request, response);
+  } catch (e) {
+    response.setStatusLine("1.0", 500, "NotOK");
+    response.write("Error handling request: " + e);
+  }
+}
--- a/browser/components/preferences/in-content/sync.js
+++ b/browser/components/preferences/in-content/sync.js
@@ -114,16 +114,17 @@ let gSyncPane = {
 
   _init: function () {
     let topics = ["weave:service:login:error",
                   "weave:service:login:finish",
                   "weave:service:start-over:finish",
                   "weave:service:setup-complete",
                   "weave:service:logout:finish",
                   FxAccountsCommon.ONVERIFIED_NOTIFICATION,
+                  FxAccountsCommon.ONLOGIN_NOTIFICATION,
                   FxAccountsCommon.ON_PROFILE_CHANGE_NOTIFICATION,
                   ];
     let migrateTopic = "fxa-migration:state-changed";
 
     // Add the observers now and remove them on unload
     //XXXzpao This should use Services.obs.* but Weave's Obs does nice handling
     //        of `this`. Fix in a followup. (bug 583347)
     topics.forEach(function (topic) {