Closed Bug 823751 Opened 7 years ago Closed 7 years ago

When closing a trusted UI dialog with persona open called from navigator.id.request(), an oncancel callback fails to fire

Categories

(Core Graveyard :: Identity, defect)

defect
Not set

Tracking

(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

VERIFIED FIXED
mozilla21
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: jsmith, Assigned: jedp)

References

Details

(Keywords: testcase)

Attachments

(2 files, 1 obsolete file)

Build: B2G 18 12/19/2012
Device: Unagi

Steps:

1. Go to http://www.se.rit.edu/~jds2501/persona_test.html with identity enabled
2. Select login
3. Close the trusted UI dialog

Expected:

An oncancel callback should fire.

Actual:

No oncancel callback fires.
100% reproducible. We're probably hitting a similar issue we hit with nav mozPay with having to handle the special case of closing a trusted context.
Keywords: testcase
blocking-basecamp: --- → ?
Summary: When closing a trusted UI dialog with persona open called form request(), an oncancel callback fails to fire → When closing a trusted UI dialog with persona open called from navigator.id.request(), an oncancel callback fails to fire
Attached file Testcase
So I confirmed I coded that test case correctly, as if I run the same test with the shim, I end up getting the oncancel callback. So this is definitely a bug on the client side code.
Good catch.  Yes, the Trusty UI currently has no way of telling a caller that it has been canceled.
Assignee: nobody → jparsons
Fernando fixed a similar bug with mozPay, so he might have some insight to provide if need be.
Given how likely it is that a user will click the close dialog, I'm moving this to blocking+
blocking-basecamp: ? → +
I've opened and claimed Bug 828076 for implementing an onCancel callback in the Trusty UI.  (Obviously this doesn't only affect identity; there's no way for a caller to know if the ui has been canceled, so it merits a separate bug to track it.)
Hi, Ben,

This required changing the logic of the pipe between gaia and the id service.  Wanted to know if you have any feedback.  Thanks!
Attachment #700028 - Flags: feedback?(benadida)
Comment on attachment 700028 [details]
Canceling trusty ui causes RP's oncancel() callback to fire

overall looks good. One thing. You're using the .match() function on the UUID. Is that going to trigger a regexp match and potentially give you some false positives?
Attachment #700028 - Flags: feedback?(benadida) → feedback+
(In reply to Ben Adida [:benadida] from comment #9)
> Comment on attachment 700028 [details]
> Canceling trusty ui causes RP's oncancel() callback to fire
> 
> overall looks good. 

Thanks for the speedy review, Ben!

> One thing. You're using the .match() function on the
> UUID. Is that going to trigger a regexp match and potentially give you some
> false positives?

I don't think so - if the uuid isn't in the id, then the match will return null.  Basically, i just want to see if the id ends with the uuid for this pipe.

It seems nearly impossible that there'd be an accidental match with a uuid, but even so, that's just to bail from the function early; there's still a switch statement that confirms only exact matches for messages opening the dialog, canceling the dialog, and getting an assertion.  

The default in the switch case will warn and do nothing if some other message containing our uuid got in.  I put that in there for debugging to ensure there were no typos in message prefixes, either now or with future development.
So once I get an r+ on Bug 828076, I'll mark this for checkin.
(In reply to Jed Parsons [:jparsons] from comment #11)
> So once I get an r+ on Bug 828076, I'll mark this for checkin.

Feel free to ask Ed for help landing this. They are wanting these patches to land asap after r+. The tree managers are fully aware and willing to help here at a fast pace.
(In reply to Jason Smith [:jsmith] from comment #12)
> (In reply to Jed Parsons [:jparsons] from comment #11)
> > So once I get an r+ on Bug 828076, I'll mark this for checkin.
> 
> Feel free to ask Ed for help landing this. They are wanting these patches to
> land asap after r+. The tree managers are fully aware and willing to help
> here at a fast pace.

Thanks, Jason.  I just want someone from marketplace to test that the patch doesn't break their stuff.  Fernando and I talked it over, but I want someone to actually try it to be sure.  I'll ping some of the good marketplace folks.
r=vingtetun, bless him.
Comment on attachment 700028 [details]
Canceling trusty ui causes RP's oncancel() callback to fire

Thanks for the feedback, Ben.  Can I get an r+?
Attachment #700028 - Flags: review?(benadida)
Comment on attachment 700028 [details]
Canceling trusty ui causes RP's oncancel() callback to fire

looks good, thanks for the explanations.
Attachment #700028 - Flags: review?(benadida) → review+
Jason, I can't access your test html page, but when you do test this, be sure to specify the oncancel handler in the request() options, not in the watch() options.

You can test with this if you like: http://people.mozilla.org/~jparsons/test_b2g.html

Also, notoriousb2g.123done.org has an oncancel handler.  It re-enables the login button.  If the oncancel handler does not fire, the login button will remain faded out and unclickable.

oncancel is described in the spec here: https://developer.mozilla.org/en-US/docs/DOM/navigator.id.request#Parameters
Attachment #700160 - Attachment is patch: true
Closing per inbound + b2g18 rules for work week = blocker closed!
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
Jason, fyi, Bug 829110 adds tests for this to the uitests app
Target Milestone: --- → mozilla21
Verified by doing a couple sanity tests to make sure oncancel was firing when closing the trusted UI.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.