Sam Penrose Assignee
	Description • 10 years ago

0) As of bug 1030227 , when an RP calls request({refreshAuthentication:0}) and the user enters the correct password, its onlogin fires:

  https://github.com/mozilla/gecko-dev/blob/master/toolkit/identity/FirefoxAccounts.jsm#L175

while a separate call to the onlogin for every RP, including the request caller, is also triggered thanks to the change in the bug. The underlying mechanism is tricky:

1) When the refreshAuthentication UI exits after a successful challenge, the blob returned to Gecko here:

  https://github.com/mozilla/gecko-dev/blob/master/b2g/components/FxAccountsMgmtService.jsm#L72

looks like this:

  {"id":"<id>","data":{"method":"signIn","email":"<email>","password":"<pw>"}}

according to the current pattern, data.method should be refreshAuthentication (but see below). I believe I have traced the problem as far as:

  https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/fxa_ui.js#L87

(in other words, data on that line is the above blob coming from one of the fxam_* modules).

2) However, this bug is masking a crasher in FxAccountsMgmtService.jsm:

  https://github.com/mozilla/gecko-dev/blob/master/b2g/components/FxAccountsMgmtService.jsm#L140

which is that FxAccountsManager does not have a refreshAuthentication method.

3) FxAccountsManager started the UI in its _refreshAuthentication method (note the underscore), which is a hint that the [event returned by the UI / method called in FxAccountsManager] should be spelled "refreshSucceeded" or similar; the current event naming pattern does not distinguish between "pleaseCallUI" and "uiReturned" which is bad IMHO.

4) That's a lot of refactoring. For now I think the simple, 2.0-uplift-friendly  fix is to stop ONLOGIN firing when we have a refreshAuthentication in progress.

5) Sadly, ONLOGIN fires in FxAccounts.jsm, which has no concept of refreshAuthentication because shoot me now. Also, our control flow is promise-based, which means we need promise-friendly state management. Therefore Jed I propose to move the ugly this._refreshing flag I added to FxAccountsManager to FxAccounts, using it to suppress the ONLOGIN event. The resulting patch will have about 6 single-line edits and solve this problem.

6) Alternately, we could set a flag in FirefoxAccounts.jsm when it gets the RP's request(), but:

  i. b2g/components/FxAccountsMgmtService.jsm also listens to ONLOGIN
  ii. ONLOGIN might get a new listener
  iii. ONLOGIN after refreshAuthentication is a lie anyway
  iv. flags are ugly, having 2 is worse than having 1.

Thoughts?

	Sam Penrose Assignee
	Comment 1 • 10 years ago

Oh, in point 3) I left out the punchline:

... Because FxAccountsMgmtService.jsm erroneously and thankfully calls FxAccountsManager.signIn() instead of the non-existent .refreshAuthentication(), FxAccountsManager thinks we are logging in, and calls FxAccounts.setSignedInUser().

	Sam Penrose Assignee
	Comment 2 • 10 years ago

Another thought as I remember to ni? Jed for Comment 0. It would be cleaner to patch the Gaia system app to return {data: {method: 'refreshAuthentication'}} in step 1) above, but that's a challenging review/uplift for 2.0, and we'd need to first land and uplift a Gecko patch to FxAccountsMgmtService.jsm that discarded the event instead of crashing. Maybe; I'm about 80% sure that would work. On balance I still favor the proposal in point 5) above.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons]
	Comment 3 • 10 years ago

This approach makes sense to me (point 5 in the Description).  And I agree that confining the fixes to gecko will make 2.0 uplift easier.  Furthermore, there are, as you've said elsewhere, other larger refactors potentially coming up for subsequent versions, so investing in deeper structural changes here only to discard them later would seem pointless.

	Sam Penrose Assignee
	Comment 4 • 10 years ago

This patch is a simpler version of the proposal I made in Comment 0, point 5). It turns out that FxAccountsManager directly calls the saving of state method in FxAccounts which emits the event*. Since no state has changed, we can simply not re-save it, which bypasses the firing of ONLOGIN. xpcshell tests pass and behavior verified manually.

* which seems worth reconsidering

	Jed Parsons (use needinfo, please) [:jedp, :jparsons]
	Comment 5 • 10 years ago

Comment on attachment 8448232 [details] [diff] [review]
1031580-move-refresh-flag.patch

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

Wow, short and sweet!

::: services/fxaccounts/FxAccountsManager.jsm
@@ +140,5 @@
>          user.email = user.email || aEmail;
> +
> +        if (this._refreshing) { // No need to update local state
> +          return Promise.resolve({user: this._user});
> +        }

Wow, fantastic that it was this simple.  Could you add some more comments, though, to explain for future generations?  A reference to the bug would be sufficient, I think.

	Sam Penrose Assignee
	Comment 6 • 10 years ago

Thanks, Jed; comment added.

https://tbpl.mozilla.org/?tree=Try&rev=86efe5057416

	Carsten Book [:Tomcat]
	Comment 8 • 10 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/213b52d04d79

	Carsten Book [:Tomcat]
	Comment 9 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/213b52d04d79

	No-Jun Park [:njpark]
	Comment 10 • 10 years ago

Sam, just want to check, is it blocking Bug 1000323 or other 2.0 blocking bugs?  
If not, and *if* it is not deemed as a blocker, we can uplift to 2.0 by the uplift request route.

	Sam Penrose Assignee
	Comment 11 • 10 years ago

(In reply to No-Jun Park [:njpark] from comment #10)
> Sam, just want to check, is it blocking Bug 1000323 or other 2.0 blocking
> bugs?  
> If not, and *if* it is not deemed as a blocker, we can uplift to 2.0 by the
> uplift request route.

Guilherme, this is your call.

	Guilherme Gonçalves [:ggp]
	Comment 12 • 10 years ago

It does block bug 1000323.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 13 • 10 years ago

2.0+ as this blocks a blocker. There's also a small fix ready to go. Let's get this landed.

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/0e7a7d248e40