917538 - Entering/leaving Guest Mode and hitting back reenters Guest Mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Enter Guest Session
2) Leave Guest Session
3) Hit back

Fennec then hangs at a blank screen for several seconds, the home page is eventually shown again, and the user is back inside of guest mode.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

Brad is this on your radar already and would this bug be serious enough to push shipping guest mode back from FF25 if not fixed?

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

I don't think this is serious enough to block shipping guest mode, but we'll discuss it in our next triage

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Actually, I'll go a step further and say that this is expected behavior. Ian? Karen?

Comment 4

[Ian Barlow (:ibarlow)]

I would describe it as expected if we popped the "Firefox will now restart" dialog open again, but we don't. So it's kind of a suprise this way...

I don't know if I would block shipping on it, but we should decide whether we want to throw the dialog up and keep users in Firefox, or close out of the browser and land back on the Android home screen.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

"Leave Guest mode" -> "hit back button" -> "go back to guest mode"

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #5)
> "Leave Guest mode" -> "hit back button" -> "go back to guest mode"

Sorry, meant to say that that flow makes sense to me. What would your expectation be after hitting back?

Comment 7

[Brian Nicholson (:bnicholson)]

So what would happen if I leave guest mode, go to another page, then click back twice? If I don't go to guest mode in that case, that means the back button has inconsistent behavior. If I do still go to guest mode, that means guest mode has become a permanent part of my session history, which seems really awkward.

I don't think this should be the expected behavior. If the point of guest mode is to have a seamless way to pass a phone to friends/family without touching my existing session, leaving guest mode should restore the browser to its exact state before entering guest mode. Clicking back/forward from there should go back/forward in my history, not jump around between multiple sessions.

Also, if the expected behavior after leaving guest mode and clicking back were to re-enter guest mode, clicking back after entering guest mode should do the opposite (leave guest mode), which we currently don't do. So either way, we'll need to fix something here.

Comment 8

[Karen Rudnitski [:kar]]

I am personally of the mindset that I should not be able to 'back up' into guest browsing after I have specifically left it. If someone backs up to when I may have had a guest browsing page, then that skips it and goes back to before the guest session had even begun (ie back to 'my, non-guest' browsing session). I think that's the most intuitive given how we are positioning guest browsing.

Comment 9

[Ian Barlow (:ibarlow)]

I tend to agree with Karen and Brian. So much so that I even drew a picture that I hope we can agree on :)

http://cl.ly/image/33310H2p1i24

Comment 10

[Karen Rudnitski [:kar]]

If I could draw, that's what I would have drawn. That is what I was thinking, Ian.

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: 0001-Bug-917538-Fix-Fennec-task-behavior.patch

After studying http://developer.android.com/guide/components/tasks-and-back-stack.html and looking through this code, I've found several broken things responsible for this behavior.

What's happening is that the Restarter is launched in a new task. It then launches BrowserApp in a new task. When we push back in BrowserApp, we're moving the task to the back of the stack, which brings the previous task (the Restarter) back to the front. BrowserApp then gets relaunched.

First, this patch gets rid of the System.exit(0) call from Restarter. The System.exit(0) at the end of onCreate() is resulting in undefined behavior since the activity launch is interrupted; note that we weren't even calling super.onCreate(), which is normally enforced at runtime. Also, since Restarter runs in its own process, so there's no need to for us to immediately kill it for the restart to be successful anyway.

Secondly, this adds noHistory="true" to the <activity> for Restarter. According to the docs, an activity with noHistory="true" "will not remain in the activity stack for the task, so the user will not be able to return to it", which is exactly what we want.

Thirdly, this drops the 'FLAG_ACTIVITY_NEW_TASK | FLAG_ACTIVITY_MULTIPLE_TASK' flags passed to Restarter. I'm not sure what the reasoning behind adding the FLAG_ACTIVITY_MULTIPLE_TASK flag was, but the Android documentation explicitly says to "not use this flag unless you are implementing your own top-level application launcher", which we are not.

Finally, this drops the intent.setFlags() calls in Restarter when launching BrowserApp. BrowserApp is already defined with the singleTask launch mode, so the flags given here are redundant. Side note: we were using setFlags() instead of addFlags(), which means that the Intent.FLAG_ACTIVITY_NEW_TASK wasn't even getting applied as it was replaced by the following setFlags() call.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Brian Nicholson (:bnicholson) from comment #11)
> Thirdly, this drops the 'FLAG_ACTIVITY_NEW_TASK |
> FLAG_ACTIVITY_MULTIPLE_TASK' flags passed to Restarter. I'm not sure what
> the reasoning behind adding the FLAG_ACTIVITY_MULTIPLE_TASK flag was, but
> the Android documentation explicitly says to "not use this flag unless you
> are implementing your own top-level application launcher", which we are not.
If I recall, those flags were used to ensure we got a new process when we laucnhed. I also recall that at the time it was to work around a legacy issue. Since when this was written, Froyo was the latest and hottest, Eclair would have been the legacy, and we don't support that anymore.

> Finally, this drops the intent.setFlags() calls in Restarter when launching
> BrowserApp. BrowserApp is already defined with the singleTask launch mode,
> so the flags given here are redundant. Side note: we were using setFlags()
> instead of addFlags(), which means that the Intent.FLAG_ACTIVITY_NEW_TASK
> wasn't even getting applied as it was replaced by the following setFlags()
> call.
This makes me even more confident that the flags weren't needed.

Either way, I want to flag this for QA to test restarts on older devices (2.2 and 2.3) just to be extra careful. Need-info'ing Aaron so he's aware

Comment 13

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/7571fd150b38

Things that trigger a restart and are worth verifying with this patch:
- Guest mode entry/exit
- Installing non-restartless addons
- Changing the system language
- Closing the crash reporter

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7571fd150b38

Comment 15

[Aaron Train [:aaronmt]]

Not seeing any apparent fallout with this. Ran through the scenarios above on new and old hardware.

Comment 16

[Brian Nicholson (:bnicholson)]

Comment on attachment 809611 [details] [diff] [review]
0001-Bug-917538-Fix-Fennec-task-behavior.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Restarting breaks when leaving guest mode
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Fairly low risk; only touches restarter code
String or IDL/UUID changes made by this patch: none

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f7271bceffa1
https://hg.mozilla.org/releases/mozilla-beta/rev/5e2af62b43f9

Comment 18

[u421692]

Verified as fixed on:
Device: LG Nexus 4 (Android 4.2.2)
Build: Firefox 25 Beta 8 / Aurora 26.0a2 (2013-10-16)