Closed Bug 917538 Opened 6 years ago Closed 6 years ago

Entering/leaving Guest Mode and hitting back reenters Guest Mode

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 27
Tracking Status
firefox24 --- unaffected
firefox25 + verified
firefox26 + verified
firefox27 + verified
fennec 25+ ---

People

(Reporter: bnicholson, Assigned: bnicholson)

References

Details

Attachments

(1 file)

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.
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?
Flags: needinfo?(blassey.bugs)
I don't think this is serious enough to block shipping guest mode, but we'll discuss it in our next triage
Flags: needinfo?(blassey.bugs)
Assignee: nobody → blassey.bugs
Actually, I'll go a step further and say that this is expected behavior. Ian? Karen?
Flags: needinfo?(krudnitski)
Flags: needinfo?(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.
Flags: needinfo?(ibarlow)
"Leave Guest mode" -> "hit back button" -> "go back to guest mode"
(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?
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.
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.
Flags: needinfo?(krudnitski)
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
If I could draw, that's what I would have drawn. That is what I was thinking, Ian.
Status: NEW → ASSIGNED
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.
Assignee: blassey.bugs → bnicholson
Attachment #809611 - Flags: review?(blassey.bugs)
(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
Flags: needinfo?(aaron.train)
Attachment #809611 - Flags: review?(blassey.bugs) → review+
tracking-fennec: ? → 25+
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
https://hg.mozilla.org/mozilla-central/rev/7571fd150b38
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Not seeing any apparent fallout with this. Ran through the scenarios above on new and old hardware.
Status: RESOLVED → VERIFIED
Flags: needinfo?(aaron.train)
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
Attachment #809611 - Flags: approval-mozilla-beta?
Attachment #809611 - Flags: approval-mozilla-aurora?
Attachment #809611 - Flags: approval-mozilla-beta?
Attachment #809611 - Flags: approval-mozilla-beta+
Attachment #809611 - Flags: approval-mozilla-aurora?
Attachment #809611 - Flags: approval-mozilla-aurora+
Verified as fixed on:
Device: LG Nexus 4 (Android 4.2.2)
Build: Firefox 25 Beta 8 / Aurora 26.0a2 (2013-10-16)
You need to log in before you can comment on or make changes to this bug.