Closed Bug 1031580 Opened 7 years ago Closed 7 years ago

FxA should not fire ONLOGIN twice after an RP-triggered refreshAuthentication


(Firefox OS Graveyard :: FxA, defect)

Gonk (Firefox OS)
Not set


(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

2.0 S5 (4july)
blocking-b2g 2.0+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed


(Reporter: spenrose, Assigned: spenrose)




(1 file, 1 obsolete file)

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

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:

looks like this:


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

(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:

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.

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().
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.
Flags: needinfo?(jparsons)
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.
Flags: needinfo?(jparsons)
Attached patch 1031580-move-refresh-flag.patch (obsolete) — Splinter Review
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
Attachment #8448232 - Flags: review?(jparsons)
Comment on attachment 8448232 [details] [diff] [review]

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

Wow, short and sweet!

::: services/fxaccounts/FxAccountsManager.jsm
@@ +140,5 @@
> = || 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.
Attachment #8448232 - Flags: review?(jparsons) → review+
Thanks, Jed; comment added.
Attachment #8448232 - Attachment is obsolete: true
blocking-b2g: --- → 2.0?
Keywords: checkin-needed
Target Milestone: --- → 2.0 S5 (4july)
Closed: 7 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(spenrose)
(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.
Flags: needinfo?(spenrose) → needinfo?(ggoncalves)
It does block bug 1000323.
Flags: needinfo?(ggoncalves)
2.0+ as this blocks a blocker. There's also a small fix ready to go. Let's get this landed.
blocking-b2g: 2.0? → 2.0+
You need to log in before you can comment on or make changes to this bug.