Closed Bug 834871 Opened 10 years ago Closed 10 years ago

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

Categories

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

Other
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:leo+, b2g18 fixed)

VERIFIED FIXED
blocking-b2g leo+
Tracking Status
b2g18 --- fixed

People

(Reporter: krupa.mozbugs, Assigned: ferjm)

References

Details

Attachments

(2 files)

Attached image 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.
Blocks: TrustedUI
No longer blocks: marketplace-payments
blocking-b2g: --- → tef?
Component: General → Gaia::System
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.
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.
No longer blocks: marketplace-payments
Assignee: nobody → alberto.pastor
blocking-b2g: tef? → leo+
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?
Flags: sec-review?
(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.
Flags: sec-review?
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?
(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.
Summary: Sometimes, the trustedUI shows up with the app in the background instead of the homsecreen → If the app process containing a trusted UI gets killed in the background, we should remove the trusted UI as well
See bug 836313 for the app process getting killed.
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.
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?
Assignee: alberto.pastor → ferjmoreno
Attachment #715645 - Flags: review?(jparsons)
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.
Attachment #715645 - Flags: feedback?(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!
Attachment #715645 - Flags: feedback?(alberto.pastor) → feedback+
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
Yes
Attachment #715645 - Flags: review?(jparsons) → review?(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!
Attachment #715645 - Flags: review?(alberto.pastor) → review+
No longer blocks: TrustedUI
Fernando - Is this bug already fixed? comment 16 shows the patch already merged.
Flags: needinfo?(ferjmoreno)
Yes! Sorry, I forgot to mark this as fixed.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(ferjmoreno)
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
Uplifted commit 22cd02ea644a5a655b06d7fb92b1d9fd4f7b0524 as:
v1-train: d6f9ccb04afd2bd3f69d2597ee605fd88c941640
Verified on 3/6 build.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap?(jsmith)
Flags: in-moztrap?(jsmith)
Flags: in-moztrap?
Flags: needinfo?(jhammink)
Flags: in-moztrap?
Removing NI on this.  We'll cover the case in future test passes, as a moztrap test case has been added.
Flags: needinfo?(jhammink)
You need to log in before you can comment on or make changes to this bug.