956605 - about:accounts page indicating email needs verification doesn't change on verification

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P2)

Description

[Mark Hammond [:markh] [:mhammond]]

+++ This bug was initially created as a clone of Bug #951982 +++

STR:

* With FxAccounts enabled, create a new account.  about:accounts changes to a page indicating the email needs verification.

* Verify the email address (for my test a different browser was used for the verification - not clear if this is a requirement).

Expected:

* After verification the existing about:accounts page should update to reflect the email is now verified.

Actual:

* about:accounts remains in a state where it says verification is still pending.

Note that the rest of the system does detect the state change and sync starts working - it's just about:accounts that doesn't update.  Note also that if you happen to restart Fx while waiting for verification, you end up with bug 951982.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-956605-part-1-refactor-switchToTabHavingURI-into.patch

While about:accounts has mechanisms in place to handle this state change, it's not currently supported by the server.  In IRC, rfkelly clarified:

rfkelly: markh: we had plans for a server-initiated "oh your state has changed" message using SSE or similar, but it's not implemented and not in the preliminary server API
rfkelly: currently the client has to poll to get this information

The client does already poll for this state change but makes no attempt at changing the about:accounts state.  These patches address that.

This patch refactors switchToTabHavingURI() into a new utility function (generator) findAllTabBrowsersHavingURI() which yields all tab browser elements with the specified URL, and switchToTabHavingURI() calls this function to operate as it did previously.  Part 2 uses this new utility function to reload all about:accounts tabs upon verification.

(Note it would obviously be possible to not do this refactor and have browserid_identity duplicate the logic for finding a tab, but this seems a little cleaner.)

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-956605-part-2-reload-all-about-accounts-tabs-on-.patch

This patch changes browserid_identity to reload all about:accounts tabs when the email is verified.  It does mean that if your email address is already validated, any about:accounts pages will be reloaded at startup even though they are probably already in the correct state - but that seems OK given this is a work-around for something the server and about:accounts will later manage internally.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8355962 [details] [diff] [review]
0001-Bug-956605-part-1-refactor-switchToTabHavingURI-into.patch

Review of attachment 8355962 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +6896,2 @@
>   */
> +function* findAllTabBrowsersHavingURI(aURI) {

findAllBrowsersHavingURI() would be a better name. A tabBrowser is the container holding all tabs and browsers and everything else.

@@ +6906,5 @@
>      }
>  
>      let browsers = aWindow.gBrowser.browsers;
>      for (let i = 0; i < browsers.length; i++) {
>        let browser = browsers[i];

As we don't need the index anymore... maybe use |for ... of|?

@@ +6908,5 @@
>      let browsers = aWindow.gBrowser.browsers;
>      for (let i = 0; i < browsers.length; i++) {
>        let browser = browsers[i];
> +      if (browser.currentURI.equals(aURI))
> +        yield browser;

This doesn't yield a window? Why do we need to yield a window? Can't we just use |browser.ownerDocument.defaultView|?

@@ +6963,5 @@
>    if (aOpenNew) {
> +    let isBrowserWindow = !!window.gBrowser;
> +    // This can be passed either nsIURI or a string.
> +    if (!(aURI instanceof Ci.nsIURI))
> +      aURI = Services.io.newURI(aURI, null, null);

Creating a nsIURI instance just to use aURI.spec below seems pointless. We should rather set aURI = aURI.spec if (aURI instanceof Ci.nsIURI).

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-956605-part-1-refactor-switchToTabHavingURI-into.patch

Thanks Tim.

> > +function* findAllTabBrowsersHavingURI(aURI) {
> 
> findAllBrowsersHavingURI() would be a better name. A tabBrowser is the
> container holding all tabs and browsers and everything else.

I had "tab browser" rather than "tabBrowser" in mind - the distinction being that not *all* <browser> elements will be returned, just those associated with tabs.  However, I've changed the name to your suggestion.

> As we don't need the index anymore... maybe use |for ... of|?
Done.

> This doesn't yield a window?
It is the "inner" generator, so didn't yield a window, but...

> Why do we need to yield a window? Can't we just
> use |browser.ownerDocument.defaultView|?

Good idea - done.

> @@ +6963,5 @@
> >    if (aOpenNew) {
> > +    let isBrowserWindow = !!window.gBrowser;
> > +    // This can be passed either nsIURI or a string.
> > +    if (!(aURI instanceof Ci.nsIURI))
> > +      aURI = Services.io.newURI(aURI, null, null);
> 
> Creating a nsIURI instance just to use aURI.spec below seems pointless. We
> should rather set aURI = aURI.spec if (aURI instanceof Ci.nsIURI).

Note the existing code created the nsIURI, and still does inside the findAllBrowsersHavingURI method - but I changed this version to use what you suggested instead.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-956605-part-2-reload-all-about-accounts-tabs-on-.patch

Updated for the changes in part 1.

Comment 6

[Chris Karlof [:ckarlof]]

Comment on attachment 8355964 [details] [diff] [review]
0002-Bug-956605-part-2-reload-all-about-accounts-tabs-on-.patch

Review of attachment 8355964 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this should be done in browserid_identity.js. Instead, it should be done in services/fxaccounts/FxAccounts.jsm, which manages the browser's "logged in" state. In particular, the change should be made in pollEmailStatus, after the module detects the verified status changes from unverified -> verified.

Comment 7

[Chris Karlof [:ckarlof]]

Comment on attachment 8356296 [details] [diff] [review]
0002-Bug-956605-part-2-reload-all-about-accounts-tabs-on-.patch

I don't think this should be done in browserid_identity.js. Instead, it should be done in services/fxaccounts/FxAccounts.jsm, which manages the browser's "logged in" state. In particular, the change should be made in pollEmailStatus, after the module detects the verified status changes from unverified -> verified.

Comment 8

[Chris Karlof [:ckarlof]]

FWIW, I like the heavy handed approach here to fix it "for now" until we arrive at final UX for this effort.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-956605-part-2-reload-all-about-accounts-tabs-on-.patch

As requested, the reload code has been moved to FxAccounts.jsm

Comment 10

[Chris Karlof [:ckarlof]]

Comment on attachment 8356310 [details] [diff] [review]
0002-Bug-956605-part-2-reload-all-about-accounts-tabs-on-.patch

Thanks.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8356294 [details] [diff] [review]
0001-Bug-956605-part-1-refactor-switchToTabHavingURI-into.patch

Review of attachment 8356294 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: browser/base/content/browser.js
@@ +6891,5 @@
>   *
>   * @param aURI
>   *        URI to search for
> + * @return A generator.  If you only care about the first such tab, just
> + *         abort the generator after the first hit.

Hmmm. The function is a generator but doesn't really return anything. I think we can just remove these lines. Maybe "Implements a generator" instead of "Returns ..." above.

@@ +6919,5 @@
>  
>    // Prioritise this window.
> +  if (isBrowserWindow) {
> +    for (let browser of findAllInWindow(window))
> +      yield browser;

We can just do |yield* findAllWindow(window);|

@@ +6930,5 @@
>      // and the current window (which was checked earlier).
>      if (browserWin.closed || browserWin == window)
>        continue;
> +    for (let browser of findAllInWindow(browserWin))
> +      yield browser;

We can delegate here as well. And maybe clean up a little, like:

if (!browserWin.closed && browserWin != window) {
  yield* findAllInWindow(browserWin);
}

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0012-have-about-accounts-reload-when-the-user-s-logged-in.patch

Here's an alternative that also solves the same basic problem for logged in and logged out state changing.  Eg, currently, if about:accounts is showing you as being logged in, but some other widget in the browser is used to log out (eg, the sync options panel), about:accounts doesn't change to reflect the now logged out status.

Chris, what do you think?

Comment 13

[Chris Karlof [:ckarlof]]

Comment on attachment 8361044 [details] [diff] [review]
0012-have-about-accounts-reload-when-the-user-s-logged-in.patch

Review of attachment 8361044 [details] [diff] [review]:
-----------------------------------------------------------------

Cool. So with the work that Zach is working on with the state management of about:accounts, this should show the logged in state that shows this: https://www.dropbox.com/s/c4c694p96vpeyi5/Desktop%20About%20Accounts%20Signed%20In%20or%20Unverified.png

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +41,5 @@
> +    let observeStateChange = function() {
> +      // if this window has focus we assume it was this window which triggered
> +      // the state change.  Without this, we see the window the user just
> +      // logged in with refreshing immediately after login, which is ugly...
> +      if (!document.hasFocus()) {

I think we should remove this if clause, i.e., always refresh. When the user clicks the verification email after creating an account, i.e., ONVERIFIED_NOTFICATION, this page should always change to the logged in state, regardless of whether it has focus. 

For ONLOGIN_NOTIFICATION, it could also transition to the logged in state, which is the same page. It would also be reasonable to just close the tab, regardless if it has focus.

For ONLOGOUT_NOTIFICATION, it's crazy to still have this tab open, but closing it is also reasonable.

::: services/fxaccounts/FxAccounts.jsm
@@ +395,5 @@
>                // Now that the user is verified, we can proceed to fetch keys
>                if (this.whenVerifiedPromise) {
>                  this.whenVerifiedPromise.resolve(data);
>                  delete this.whenVerifiedPromise;
> +                this.notifyObservers(ONVERIFIED_NOTIFICATION);

We also fire this notification at the end of fetchAndUnwrapKeys(), which is when everything is actually ready. What was the reason for adding in here? I think this might cause problems. I see why this is is a rational place to fire it, but this notification shouldn't be fired twice and should probably only be fired after the keys are fetched.

::: services/fxaccounts/FxAccountsCommon.js
@@ +43,5 @@
>  
>  // Observer notifications.
>  this.ONLOGIN_NOTIFICATION = "fxaccounts:onlogin";
>  this.ONLOGOUT_NOTIFICATION = "fxaccounts:onlogout";
> +this.ONVERIFIED_NOTIFICATION = "fxaccounts:onverified";

This already seems to be in Nightly?

Comment 14

[Chris Karlof [:ckarlof]]

Comment on attachment 8361044 [details] [diff] [review]
0012-have-about-accounts-reload-when-the-user-s-logged-in.patch

Review of attachment 8361044 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/fxaccounts/FxAccounts.jsm
@@ +395,5 @@
>                // Now that the user is verified, we can proceed to fetch keys
>                if (this.whenVerifiedPromise) {
>                  this.whenVerifiedPromise.resolve(data);
>                  delete this.whenVerifiedPromise;
> +                this.notifyObservers(ONVERIFIED_NOTIFICATION);

Something confusing here is that ferjm landed this http://hg.mozilla.org/mozilla-central/diff/dea1e5ca965d/services/fxaccounts/FxAccounts.jsm which changes ONLOGIN -> ONVERIFIED slightly. ONLOGIN used to mean "I'm completely ready, with keys fetched", and he changed ONLOGIN to mean "I logged in, but I might not be verified". We can keep ferjm's changes, but if we do, then it's not appropriate to fire ONVERIFIED here. It should be fired as it currently is, after the keys are fetched. Summary: remove this line.

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-956605-ensure-about-accounts-changes-in-response.patch

The approach taken in the older patches didn't sit well with me, particularly the fact it did a full reload of about:accounts as things changed.

This new patch is more reliable.  It's quite large, but most of that is because I added lots of new tests for about:accounts, including tests for things not touched here - eg, all the "?action=..." modes.

In a nutshell:

* about:accounts adds observers for signed in user state changes, and updates the visibility of various elements accordingly.  eg, a normal "about:accounts" page will flip between "manage" and "get started" as the state changes.  Note that it handles the fact that some of these state change notifications may have originated from the page itself (ie, when the page itself is going the setSignedInUser()) and takes no action in that case, as the page already handles that.  So in effect it is only responding to changes made by some *other* about:accounts page or the Fx UI itself.

* If about:accounts is in an "?action=" mode, it reinitializes the iframe, which does the right thing - eg, the remote frame correctly handles when you are now verified.

* The tests have been made e10s-friendly, predicting a future when about:accounts might want to live in a content process.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8381890 [details] [diff] [review]
0001-Bug-956605-ensure-about-accounts-changes-in-response.patch

Review of attachment 8381890 [details] [diff] [review]:
-----------------------------------------------------------------

I'm wondering, can we make the control flow so that also for a page actively used by the user we wait for the notifications and then call .injectData()? That way we wouldn't need to track whether we're currently handling a remote command or not.

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +172,5 @@
>      }, (err) => this.injectData("message", { status: "error", error: err })
> +    ).then(
> +      () => this.isHandlingRemoteCommand = false,
> +      () => this.isHandlingRemoteCommand = false
> +    )

We could make onLogin(), onSessionStatus(), and onSignOut() return a promise and do the .isHandlingRemoteCommand toggling in handleRemoteCommand() then. I think that would be a little cleaner.

@@ +320,5 @@
> +    if (window.location.href.contains('action=')) {
> +      log("about:accounts in special mode: reloading it");
> +      wrapper.init(wrapper.iframe.getAttribute("src"));
> +      return;
> +    }

Shouldn't we just do a location.reload()? Going through init() again would make us add a second FirefoxAccountsCommand listener.

@@ +329,5 @@
> +      case fxAccountsCommon.ONLOGOUT_NOTIFICATION:
> +        show("intro");
> +        hide("manage");
> +        break;
> +      default:

With a binary choice here, an |if| statement would look better.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

(In reply to Tim Taubert [:ttaubert] from comment #16)
> I'm wondering, can we make the control flow so that also for a page actively
> used by the user we wait for the notifications and then call .injectData()?
> That way we wouldn't need to track whether we're currently handling a remote
> command or not.

I don't get what you mean here - ie, I don't understand what the pages which *are not* actively doing something would do in this case.

> Shouldn't we just do a location.reload()? Going through init() again would
> make us add a second FirefoxAccountsCommand listener.

TBH, I'm a little confused by this - I was previously convinced that iframe.contentWindow.location.reload dropped the existing FirefoxAccountsCommand listener.  Now I'm not convinced of that, but also not convinced it doesn't :)  I need to investigate that more.

One other thing I only just noticed is that the isHandlingRemoteCommand logic doesn't work as expected when an already verified user logs in - this ends up causing 2 notifications - ONLOGIN, but then a few event-loop spins later (so after isHandlingRemoteCommand has gone false) an ONVERIFIED.  This means that an already verified user logging in causes that page to reload which is somewhat ugly.

Does anyone have thoughts on that?

Anyway, this is P2, so I'll invest more time later.

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Zach,
  I'm not sure who else maintains this - please feel free to forward the request if someone else can answer it better.

I think to fix this properly, we would arrange for aboutaccounts.js to send a message to the iframe to tell it that the current browser state has changed, and for it to update itself accordingly without a full refresh of the iframe (which looks bad).  Eg, we'd just send it a "stateChanged" message - it could then respond by using its existing mechanisms to request the current state is and update itself accordingly.  Does this sound feasible?

Comment 19

[Zachary Carter [:zaach]]

Yep, that sounds fine. Github issue: https://github.com/mozilla/fxa-content-server/issues/660

Comment 20

[Chris Karlof [:ckarlof]]

Mark, I'm of the opinion that (currently) once the remote page submits the "login" message to the browser, it's done -- at that point it's dead code. I think to properly think about this we need consider all the states and what the appropriate transitions should be.

For the simplest case (creating an account and the user verifies her email), jgruen has indicated that about:accounts should transition to the "Manage" screen, which doesn't involve any messaging to the iframe (see pg 6 of https://www.dropbox.com/s/1dm7d3o3jk8rowc/desktop%20touch%20up.pdf).

I'm going to try to document the state machine more.

Comment 21

[Chris Karlof [:ckarlof]]

FWIW, I think the current patch up for review is a step in the right direction, but it probably doesn't handle all the weird cases the user can get in. Maybe that's fine for Fx29 -- the most important thing is that the common case is handled well.

Comment 22

[Chris Karlof [:ckarlof]]

Bug 976667 is related, with some comments from John.

Comment 23

[Chris Karlof [:ckarlof]]

Here's my cut at a state machine: 


States:
loggedOut
loggedInVerified
loggedInUnverified
loggedInError (reauth needed) <- I'm not sure if we currently have a way of representing this state in FxAccounts.jsm

Behavior:

1) after about:accounts?action=<action> loads

switch (<action>)

case "(no action)":
  loggedOut => show(intro)
  else => show(manage)
 
case "signin":
  loggedOut => show(remote with signin url)
  else => show(manage)
  

case "signup":
  loggedOut => show(remote with signup url)
  else => show(manage)

case "reauth"
  (if we can know we're in the loggedInError state)
  loggedInError => show(remote with reauth url)
  else => show(manage)

  (otherwise)
  all => show(remote with reauth url)
  

2) when about:accounts observes ONLOGIN_NOTIFICATION, ONVERIFIED_NOTIFICATION, ONLOGOUT_NOTIFICATION:

ONLOGOUT_NOTIFICATION => close the window
ONVERIFIED_NOTIFICATION || ONLOGIN_NOTIFICATION => window.location = about:accounts (This effectively does a reload, stripping the params. This should show the intro or manage page, depending on whether we're logged in or logged out.)

3) when we process a "login" command from the remote page

This is a tricky one, because we don't necessarily want to respond to our own ONLOGIN_NOTIFICATION. If the user is unverified, we need to preserve the "check your email" message. Some options:

Simplest: Don't do anything special during "login" commands. Never listen to ONLOGIN_NOTIFICATION, and rely on ONVERIFIED_NOTIFICATON to update the page state after we verify the user's email is verified and we get her keys.
Alternative: Only stop listening for ONLOGIN_NOTIFICATON when we receive a "login" command.

Either way, the rules in 2) will redirect the tab to about:accounts in response to ONVERIFIED_NOTIFICATION, which will show the "Manage" page, satisfying Bug 976667.

Comment 24

[Chris Karlof [:ckarlof]]

> ONLOGOUT_NOTIFICATION => close the window

Result of IRC discussion between gavin, me, markh is to change this to "redirect to about:accounts?action=signin"

Comment 25

[Chris Karlof [:ckarlof]]

John can you review the above state machine from a UX perspective?

Comment 26

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-956605-have-about-accounts-do-the-right-thing-ba.patch

Implements as described in this bug, including the late-breaking change of action=signin on logout.

Depends on bug 980182 for the tests - although the about:accounts changes themselves will apply without that bug if you want to try it out.

Comment 27

[Chris Karlof [:ckarlof]]

Comment on attachment 8387262 [details] [diff] [review]
0002-Bug-956605-have-about-accounts-do-the-right-thing-ba.patch

Review of attachment 8387262 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I'll try to test this tomorrow. Some things I'd like to look at:

1) During account creation when we show the "go confirm your email", what happens during refresh and navigation events.
2) I'm curious what the UX looks like during login w/previously verified email.

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +271,5 @@
> +        // asking to sign-up when already signed in just shows manage.
> +        showManage();
> +      } else {
> +        show("remote");
> +        wrapper.init();

I think we should make this more explicit. Can we add identity.fxaccounts.remote.signup.uri which explicitly adds /signup to the path? Alternatively, we could add /signup to identity.fxaccounts.remote.uri. I think this will make it more explicit to the remote page that we explicitly want to trigger a signup and it should clear any logged in state it has.

@@ +292,5 @@
>        } else {
>          show("stage");
>          show("intro");
>          // load the remote frame in the background
>          wrapper.init();

Likewise, I think we should make sure that in the default case, the remote url takes the user to /signup so the page can be signaled to clear any of its underlying state.

Comment 28

[Chris Karlof [:ckarlof]]

Adding a dependency on adding an explicit signup URI in the prefs.

Comment 29

[Chris Karlof [:ckarlof]]

Comment on attachment 8387262 [details] [diff] [review]
0002-Bug-956605-have-about-accounts-do-the-right-thing-ba.patch

Review of attachment 8387262 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +290,2 @@
>          let sb = Services.strings.createBundle("chrome://browser/locale/syncSetup.properties");
>          document.title = sb.GetStringFromName("manage.pageTitle");

Should this crap regarding the page title be included in showManage(). I notice it's being excluded from other calls to showManage() and I wonder why.

Comment 30

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-956605-have-about-accounts-do-the-right-thing-ba.patch

Comment 31

[Chris Karlof [:ckarlof]]

Comment on attachment 8388916 [details] [diff] [review]
0002-Bug-956605-have-about-accounts-do-the-right-thing-ba.patch

Review of attachment 8388916 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +312,5 @@
>  }
>  
> +function showManage() {
> +  show("stage");
> +  show("manage");

hide('remote') here?

Comment 32

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/fx-team/rev/5639010709ef

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5639010709ef

Comment 34

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8388916 [details] [diff] [review]
0002-Bug-956605-have-about-accounts-do-the-right-thing-ba.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FxA Sync
User impact if declined: User confusion about current FxA state
Testing completed (on m-c, etc.): Landed on m-c, extensive manual testing
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Needs a branch patch.

Comment 36

[Mark Hammond [:markh] [:mhammond]]

Oops - I didn't mark the dependency.  This patch should apply on top of bug 980182, which I just requested for Aurora.

Comment 37

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #36)
> Oops - I didn't mark the dependency.  This patch should apply on top of bug
> 980182, which I just requested for Aurora.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/51525b7ced1d