Closed Bug 882496 Opened 7 years ago Closed 7 years ago

Error: rp is undefined" when calling `navigator.id.logout()

Categories

(Core Graveyard :: Identity, defect)

22 Branch
All
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
mozilla24
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: jedp, Assigned: jedp)

Details

Attachments

(1 file)

It's possible to get an error complaining that "rp is undefined" if you hit the unwatch() call in toolkit/identity/MinimalIdentity.jsm when there's no current rp flow.

We should test for the existence of rp before trying to access on it, and return early if there's no rp.
Flagging this as leo? since it can break the persona flow on a page that triggers the error.
blocking-b2g: --- → leo?
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
Can you reproduce in a normal user flow?
Flags: needinfo?(jparsons)
Fails gracefully if you call logout() or request() before watch().  Also if an unwatch() message is received unexpectedly, which I've observed once or twice with rapid page reloads, though I can't reliably duplicate it.
Attachment #762422 - Flags: review?(benadida)
(In reply to Alex Keybl [:akeybl] from comment #2)
> Can you reproduce in a normal user flow?

Not in a normal user flow, no.  

This can happen if the website or other relying party incorrectly calls logout() or request() before watch().

I'm more concerned about the case where the page is unloaded before watch() completes executing.  In this case, the internal method unwatch() will be called out of sequence.  It's possible to reproduce this on device if you're fast with your fingers.  I was able to trigger it a few times, but I can't reproduce it reliably.

The result in the latter case is annoying.  It breaks the page until you reload, and it's not obvious what has happened.

The patch for this is low risk.
Flags: needinfo?(jparsons)
Checking that the new tests look good on all platforms

https://tbpl.mozilla.org/?tree=Try&rev=919c70b41b67
Incorrect usage can be resolved server side.
blocking-b2g: leo? → -
(In reply to Alex Keybl [:akeybl] from comment #6)
> Incorrect usage can be resolved server side.

I don't see how.  The code that needs guarding is hit in the client before any server-side code is invoked.
Comment on attachment 762422 [details] [diff] [review]
Fail gracefully if Persona observer methods called before watch()

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

this looks good and needed. r+
Attachment #762422 - Flags: review?(benadida) → review+
(In reply to Ben Adida [:benadida] from comment #8)
> Comment on attachment 762422 [details] [diff] [review]
> Fail gracefully if Persona observer methods called before watch()
> 
> Review of attachment 762422 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this looks good and needed. r+

Thanks, Ben.

Alex, what are the criteria this patch fails to satisfy for uplift?  The bug it addresses could be triggered both by RPs misusing the API and also, I think more seriously, by the user simply switching tabs too fast.  The result will be that the application or tab is broken and will need restarting, which can be hard to figure out how to do.

You mention in Comment 6 that there's a server-side fix for this; what do you suggest?  I can't see that this could be remedied without the fix on the client.

Thanks for your feedback,
j
Flags: needinfo?(akeybl)
Read comment 9, and spoke with Ben. This blocking- was based upon the incorrect perception of triage that page should ensure an rp is defined in this situation.
blocking-b2g: - → leo+
Flags: needinfo?(akeybl)
Assignee: nobody → jparsons
(In reply to Alex Keybl [:akeybl] from comment #10)
> Read comment 9, and spoke with Ben. This blocking- was based upon the
> incorrect perception of triage that page should ensure an rp is defined in
> this situation.

Thanks so much Alex for the quick response!
https://hg.mozilla.org/mozilla-central/rev/e09d80620b69
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.