834871 - If the app process containing a trusted UI gets killed in the background, we should remove the trusted UI as well

Status: VERIFIED FIXED

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

Description

[krupa raj[:krupa]]

Attachment: screenshot

<project name="https://git.mozilla.org/releases/gecko.git" path="gecko" remote="mozillaorg" revision="d72fcfb5ebcd1a51ba4e318517244bdb34ddbfa5"/>
  <project name="https://git.mozilla.org/releases/gaia.git" path="gaia" remote="mozillaorg" revision="6369dbf33b622faf4b4d176fed30b77c5c319dfc"/>

steps to reproduce:
1. Install marketplace-dev from http://people.mozilla.org/~kmcmillan/mktdev.html
2. Click on Login/Register
3. switch to a different app for a bit
4. Click on the marketplace-dev app

observed behavior:
Sometimes the trustedUI shows up with the app in the background instead of the homsecreen.

reproducible? not always but often enough. 

See screenshot. I didn't see anything weird in the logcat.

Comment 1

[Jason Smith [:jsmith]]

I can reproduce...pretty easily in fact. It's almost as if the the process for marketplace was killed in the background and restarted when you revisited the app, but the trusted UI remains on top. Obviously that destroys pretty much the entire point of a trusted UI.

Comment 2

[Jason Smith [:jsmith]]

This is already part of the tree for marketplace payments that resides under the basecamp payments tracking bug under trusted UI. This does not belong in that tree.

Comment 3

[Caitlin Galimidi [:c8o]]

I understand we may literally not be able to fix this at this late date.
However, should someone on security be aware of this concern?
Jason? Krupa? What do you think?

Comment 4

[Jason Smith [:jsmith]]

(In reply to Caitlin Galimidi from comment #3)
> I understand we may literally not be able to fix this at this late date.
> However, should someone on security be aware of this concern?
> Jason? Krupa? What do you think?

Lucas was actually the one who made the call on this bug - it was cut from v1 for being too high risk for regressions. No need to flag for sec-review.

Comment 5

[alberto.pastor]

It seems that the problem is the Marketplace app restarts (without the user explicitly closing it), so the trustedUI keeps thinking is open when it has actually been closed. 

We would need to add some logic in the trustedUI for being aware when an app crashes for reseting the trustedUI process. I don't think is very risky. Thoughts?

Comment 6

[Jason Smith [:jsmith]]

(In reply to Alberto Pastor from comment #5)
> It seems that the problem is the Marketplace app restarts (without the user
> explicitly closing it), so the trustedUI keeps thinking is open when it has
> actually been closed. 
> 
> We would need to add some logic in the trustedUI for being aware when an app
> crashes for reseting the trustedUI process. I don't think is very risky.
> Thoughts?

That's a good idea that I think we should consider for uplift, but given that we're fixing here is handling the case where the app process crashes or gets killed, I wouldn't block on this.

However - I'm going to open a separate bug about the marketplace app getting killed in this case. That I think might block.

Comment 7

[Jason Smith [:jsmith]]

See bug 836313 for the app process getting killed.

Comment 8

[Jason Smith [:jsmith]]

Now I understand what the problem is here. We're dieing on a stress test with memory here with the trusted UI, except that we don't clean up the trusted UI when the app process gets killed. So there's a potential risk right now that a user could return to the marketplace app after being killed due to lack of memory and complete a purchase without the correct marketplace UI after the payment flow completes present.

Comment 9

[Jason Smith [:jsmith]]

Not sure if this is part of our v1 memory acceptance criteria or not.

Chris Jones - Would we classify this as part of our memory acceptance criteria for v1?

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8195

Pointer to Github pull-request

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

We were always closing the dialog stack associated with the last displayed app, which is not correct. We need to be more specific and close the stack associated with a given origin wherever it's possible.

Comment 12

[alberto.pastor]

Just made a comment in Github, but I think is good fix. We were closing always the currentStack instead of the app stack that caused the close method. Thanks :ferj!

Comment 13

[Jason Smith [:jsmith]]

ferjm and albertopq - Would this bug fix the following STR:

1. Open marketplace
2. Start a login into persona
3. Hit the home button
4. Go to the card view
5. Kill the marketplace process with the trusted UI open
6. Launch marketplace again

Expected - marketplace launches with no trusted UI present

Actual - marketplace launches with the old trusted UI present

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Yes

Comment 15

[alberto.pastor]

Comment on attachment 715645 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8195

As soon as the comments are addressed. Thanks!

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Thanks Alberto!

https://github.com/mozilla-b2g/gaia/commit/22cd02ea644a5a655b06d7fb92b1d9fd4f7b0524

Comment 17

[Jason Smith [:jsmith]]

Fernando - Is this bug already fixed? comment 16 shows the patch already merged.

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Yes! Sorry, I forgot to mark this as fixed.

Comment 19

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted commit 22cd02ea644a5a655b06d7fb92b1d9fd4f7b0524 as:
v1-train: d6f9ccb04afd2bd3f69d2597ee605fd88c941640

Comment 20

[Jason Smith [:jsmith]]

Verified on 3/6 build.

Comment 21

[John Hammink]

Removing NI on this.  We'll cover the case in future test passes, as a moztrap test case has been added.