Closed Bug 1030227 Opened 8 years ago Closed 8 years ago

FxA is not firing onlogin for RPs such as FMD which have called watch()


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

No description provided.
blocking-b2g: --- → 2.0?
Attached patch 1030227-fire-onlogin.patch (obsolete) — Splinter Review
As a follow-up, I think the getAssertion().then(rp.doLogin()) bit should be broken out into a common routine, but ONLOGIN -> request() seems safe to me for now. Thanks Jed!
Attachment #8446008 - Flags: review?(jparsons)
Target Milestone: --- → 2.0 S5 (4july)
blocking-b2g: 2.0? → 2.0+
Comment on attachment 8446008 [details] [diff] [review]

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

I don't think we have tests for any of fxaccounts:onlogin, :onlogout, or :onverified, do we?  We ought to ensure that they result in onlogin() etc. being fired for an RP.  And we should ensure that these sequences are on the checklist of manual tests to perform.

I agree that some refactoring could be done with getAssertion().then(rp.doLogin()).

One little fix, apart from which, this looks good to me!  Thanks, Sam.

::: toolkit/identity/FirefoxAccounts.jsm
@@ +90,5 @@
>          for (let [rpId,] of this._rpFlows) {
>            this.doLogout(rpId);
>          }
>          break;
>        case "quit-application-granted":

After the test for ONVERIFIED_NOTIFICATION, can you remove the observers for ONLOGIN and also ONLOGOUT, which seems to have been missed in some prior patch?
Attachment #8446008 - Flags: review?(jparsons) → review+
Thanks Jed, especially for catching that leak. Agreed on all points.
Attachment #8446008 - Attachment is obsolete: true
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.