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

Status: RESOLVED FIXED

(Core Graveyard :: Identity, defect)

Description

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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.

Comment 1

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Flagging this as leo? since it can break the persona flow on a page that triggers the error.

Comment 2

[Alex Keybl [:akeybl]]

Can you reproduce in a normal user flow?

Comment 3

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Fail gracefully if Persona observer methods called before watch()

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.

Comment 4

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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

Comment 5

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Checking that the new tests look good on all platforms

https://tbpl.mozilla.org/?tree=Try&rev=919c70b41b67

Comment 6

[Alex Keybl [:akeybl]]

Incorrect usage can be resolved server side.

Comment 7

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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

[Ben Adida [:benadida]]

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+

Comment 9

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(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

Comment 10

[Alex Keybl [: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.

Comment 11

[Jed Parsons (use needinfo, please) [:jedp, :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!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e09d80620b69

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e09d80620b69

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/f8148cef6556

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/f8148cef6556