828182 - identifier close mozChromeEvent should let gaia know which frame it's fired on.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[(inactive after 6/18) Alive Kuo [:alive]]

I see https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/identity.js#L67
and learns the fact that if we have two identifier frame at the same time, we may not know which frame is closing. It's a pain to maintain an queue in gaia, so I think the simpler and best way is attach frame info in close event.

Jason, please help to move to the correct component if I am wrong. 
Thanks!

Comment 1

[Jason Smith [:jsmith]]

Talked with Alive and Ferjm in IRC about this:

The impact of this bug is a potential risk that closing one trusted UI with persona up when two persona dialogs up will close the wrong dialog.

It's edge case enough though not to block, as I don't expect multiple dialogs up at the same time to happen that often.

Comment 2

[Jason Smith [:jsmith]]

Yikes. This is causing a nasty hang right now with the trusted UI. Jed - Can you investigate?

Comment 3

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

I can investigate, yes.  Are the STR what you describe in Bug 830358?

Comment 4

[Jason Smith [:jsmith]]

(In reply to Jed Parsons [:jparsons] from comment #3)
> I can investigate, yes.  Are the STR what you describe in Bug 830358?

Yup, that's correct.

Comment 5

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

Spoke with Fernando in IRC and got caught up on what you all had already discussed.  I had worried about this in the past (it's actually my comment at the top of the open function :)  Sounds like the fix is simply to require the caller of close() to pass its chromeEventId as it does on open()

fwiw, I think this is incorrectly filed as an identity bug.  Since Trusty UI is so crucial, I think it should have its own component in gaia.

Assigning to me

Comment 6

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

The trusty UI needs tests, badly.  I've spun off bug 830528 to track the need for tests.  Any tests I put together for this feature I will attach to that bug, since just getting the test suite started for TUI seems like an entirely separate issue.

Comment 7

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

Attachment: Use chromeEventId to select which frame to close

I've tested this with Identity, but I don't have a good test environment for Marketplace.  Please ensure that I've done the right thing with payment.js.  Thanks!

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Thanks Jed. I'll take a look at this tonight.

Comment 9

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

Thanks, Fernando.  

I'll keep an eye on Bug 813811; as soon as the PR for that lands in gaia, I'll update this PR.  We're bound to have a couple of minor conflicts.

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 702061 [details]
Use chromeEventId to select which frame to close

r=me

I've left a comment in the PR.

Don't worry about Bug 813811. I'll merge this first as it was submitter in first place, so you don't need to rebase your patch :).

Comment 11

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

Ok.  Thanks very much, Fernando; I'll wait until this is merged.

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

https://github.com/mozilla-b2g/gaia/commit/cb12e3c55583c3c1ccb4cd41404679c09e042ee4

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.

Comment 14

[amiller]

No need to make a Test Case in Moztrap for this issue.

Comment 15

[Darren]

Cannot verify, need steps to blackbox test this issue.