Jason Smith [:jsmith] Reporter
	Description • 12 years ago

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.

	Jason Smith [:jsmith] Reporter
	Comment 1 • 12 years ago

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.

	Jason Smith [:jsmith] Reporter
	Comment 3 • 12 years ago

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.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 4 • 12 years ago

Good catch.  Yes, the Trusty UI currently has no way of telling a caller that it has been canceled.

	Jason Smith [:jsmith] Reporter
	Comment 5 • 12 years ago

Fernando fixed a similar bug with mozPay, so he might have some insight to provide if need be.

	Ben Adida [:benadida]
	Comment 6 • 12 years ago

Given how likely it is that a user will click the close dialog, I'm moving this to blocking+

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 7 • 12 years ago

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

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 8 • 12 years ago

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!

	Ben Adida [:benadida]
	Comment 9 • 12 years ago

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?

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 10 • 12 years ago

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

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 11 • 12 years ago

So once I get an r+ on Bug 828076, I'll mark this for checkin.

	Jason Smith [:jsmith] Reporter
	Comment 12 • 12 years ago

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

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 13 • 12 years ago

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

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 14 • 12 years ago

r=vingtetun, bless him.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 15 • 12 years ago

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+?

	Ben Adida [:benadida]
	Comment 16 • 12 years ago

Comment on attachment 700028 [details]
Canceling trusty ui causes RP's oncancel() callback to fire

looks good, thanks for the explanations.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 17 • 12 years ago

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

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 18 • 12 years ago

Updated patch header

	Andrea Marchesini [:baku]
	Comment 19 • 12 years ago

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

https://hg.mozilla.org/releases/mozilla-b2g18/rev/02abef861cc7

	Jason Smith [:jsmith] Reporter
	Comment 20 • 12 years ago

Closing per inbound + b2g18 rules for work week = blocker closed!

	Ed Morley [:emorley]
	Comment 21 • 12 years ago

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

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 22 • 12 years ago

Jason, fyi, Bug 829110 adds tests for this to the uitests app

	Jason Smith [:jsmith] Reporter
	Comment 23 • 12 years ago

Verified by doing a couple sanity tests to make sure oncancel was firing when closing the trusted UI.