835012 - Don't show home page/frequently used tabs when backing out of Fx

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Dirkjan Ochtman (:djc)]

I frequently open up Twitter links in Firefox. When I'm done reading, I hit the Back button to go back to Twitter. When I do so, it looks like Firefox first closes the tab I was reading, loads the home page with recently used tabs, then disappears from view. It should try not to visualize the closing of the tab for (perceptive and otherwise) performance reasons.

Comment 1

[Aaron Train [:aaronmt]]

Which device and how much RAM is on it?

Comment 2

[Dirkjan Ochtman (:djc)]

It's a Nexua 4, I think they have 2GB.

Comment 3

[Dirkjan Ochtman (:djc)]

Ping. Yesterday, I noticed this again, though it now happens with the "previous" tab (compared to the one I was viewing). In addition to any performance issues there might be, it's also just confusing for the browser view to change when I'm backing out.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Don't close external tabs until onStop()

The activity isn't actually hidden until onStop(), so we need to delay the tab close until then if we want to avoid seeing UI changes. While this patch fixes the issue, it introduces problems of its own.

Since onStop() isn't guaranteed to be called (see diagram at http://developer.android.com/reference/android/app/Activity.html), we can be killed before we've sent the Tab:Close message to Gecko. That means that when we open Fennec again and it does an OOM restore, the external tab will be opened again.

There might be some tricks we can do, such as marking to-be-closed tabs in the outState bundle in onPause(); when resuming, we can simply ignore these tabs during restore. But we could also just WONTFIX this bug if this minor annoyance isn't worth all the effort.

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 741616 [details] [diff] [review]
Don't close external tabs until onStop()

Review of attachment 741616 [details] [diff] [review]:
-----------------------------------------------------------------

I'd love to see this bug fixed btw. IMO, this change is worth it.

::: mobile/android/base/GeckoApp.java
@@ +188,4 @@
>      private static final String RESTARTER_ACTION = "org.mozilla.gecko.restart";
>      private static final String RESTARTER_CLASS = "org.mozilla.gecko.Restarter";
>  
> +    private Tab mPendingCloseTab;

Drive-by: I'd probably rename this to something more explicit: mTabToCloseOnStop or something like this. "Pending" does tell much about the specific use case this member covers.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

I don't like adding complexity for something that should just work.

Comment 8

[Brian Nicholson (:bnicholson)]

Unassigning myself since I won't be working on this in the near future and we aren't sure whether the changes will be worth it.

Comment 10

[Aaron Train [:aaronmt]]

This sounds like wontfix

Comment 11

[Richard Newman [:rnewman]]

I don't agree that this is a WONTFIX.

It's annoying enough that it's acquired two dupes.

There might be a simple solution -- why not finish() the activity sooner? -- and if there isn't, it's still a legitimate issue that we simply haven't addressed yet.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#2296

We are making the right calls in the right order, but maybe we could defer the tab.closeTab(...) call to onPause() ?

Comment 13

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.
> java#2296
> 
> We are making the right calls in the right order, but maybe we could defer
> the tab.closeTab(...) call to onPause() ?

onPause() happens before onStop(), and onStop() is when the activity is hidden (see comment 4), so I think onPause() would be too early.

To fix this, I'd explore the following:
1) Create another event called "disk-cache-flush" that we listen to here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#163.
2) Post a Runnable or store the pending tab like we do in the WIP patch here, then close the tab after onStop().
3) After that, send the event created in #1 to immediately write the session to disk (to make sure the closed tab persists in the session).

It's not perfect since there's no guarantee that we won't be killed before step 3, but we'll see how much this really matters in practice.

Comment 14

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.