781883 - Cannot go from about:home to about:firefox with "Don't Keep Activities" enabled

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

1) Quit Firefox if already running.
2) Open Firefox.
3) Clicking the awesomebar and type in "about:firefox"

Expected: about:firefox opens
Actual: The page remains on about:home

Note that there is also odd behavior going in reverse - going from about:firefox to about:home also breaks. This behavior seems to be less broken after browsing for a short time, specifically after opening new tabs.

Reproduced on TF201, Android 4.0.3. Cannot repo on Galaxy Nexus, Android 4.0.4.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Works for me on the Galaxy Tab 10.1, Android 3.1

Comment 2

[Kartikaya Gupta (email:[e-mail])]

I assume there's nothing useful in the logcat? Do you get any indicator of progress? i.e. does the spinner appear and disappear, or does it never appear at all?

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The(In reply to Kartikaya Gupta (:kats) from comment #2)
> I assume there's nothing useful in the logcat?

Nothing directly related to entering "about:firefox" into the URL bar. However, bug 781729 could be related – IntentReceiverLeaked gets called when the awesomebar is first pressed.

> ... Do you get any indicator of
> progress? i.e. does the spinner appear and disappear, or does it never
> appear at all?

No indication of progress. The screen goes to gray and then loads about:home, the same way it does when clean launching the browser.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Logcat output

Comment 5

[Kartikaya Gupta (email:[e-mail])]

It looks like you have the "don't keep activities" checkbox thing set. Does the problem still happen if you uncheck that?

Comment 6

[mcomella]

Disabling that seems to fix the problem.

Wow, I wonder how long I've had that on for. Perhaps that's why my tablet has acted so crappy lately. :/

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Not exclusive to tablets.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

The problem here seems to be related to painting - the page actually does load but doesn't paint. If you reload the page it will render properly as about:firefox. I suspect that the about: pages load really quickly and finish loading before the gfx objects on the Java side are fully created again, and something gets dropped as a result.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Actually turns out the problem is that we start loading the page before GeckoApp is reinitialized, and the Content:LocationChange event that browser.js sends to java gets ignored. This means that the BrowserApp class never invokes its hideAboutHome code and so the about:home content remains visible.

In general it seems like a problem that we listen for so many events in GeckoApp, because when the activity is torn down we unregister all of those listeners and no longer handle those events. We only start handling them again when the activity is restored. In this case moving the Content:LocationChange handler to Tabs.java makes it persist and fixes the problem.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/2) Unregister GeckoApp as a tab listener in destroy

Without this patch, we register a new GeckoApp as a tab listener every time we create a new one, and never unregister the previous ones. In addition to causing an ever-increasing memory leak, this does mountains of unnecessary work.

Comment 11

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 655740 [details] [diff] [review]
(1/2) Unregister GeckoApp as a tab listener in destroy

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

Awesome!

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/2) - Move the LocationChange listener to Tabs.java

This removes the small window between one GeckoApp getting torn down and the next GeckoApp starting up where we have nothing listening for Content:LocationChange events. Therefore this fixes the problem by making sure that hideAboutHome gets run when it's supposed to.

Comment 13

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 655741 [details] [diff] [review]
(2/2) - Move the LocationChange listener to Tabs.java

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

Looks good to me.

I might have added a getHandler() to GeckoApp. Just in case I had added, please use it, instead of:
GeckoApp.mAppContext.mMainHandler <-- one less "public static" member :(

Comment 14

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f301dd62fb2
https://hg.mozilla.org/integration/mozilla-inbound/rev/12504471edef

(In reply to Sriram Ramasubramanian [:sriram] from comment #13)
> I might have added a getHandler() to GeckoApp. Just in case I had added,
> please use it

I didn't see one

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3f301dd62fb2
https://hg.mozilla.org/mozilla-central/rev/12504471edef

Comment 16

[Aaron Train [:aaronmt]]

Channel uplift?

Comment 17

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

I could see uplifting to Aurora, just to get the fix out a bit sooner. I don't think we need to rush it to Beta though.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Yeah I plan on requesting uplift to aurora in a week or so. I'm a little concerned that this might have unintentional side-effects so I'd like to let it bake a little longer.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 655740 [details] [diff] [review]
(1/2) Unregister GeckoApp as a tab listener in destroy

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: When "don't keep activities" is enabled, the browser will gradually leak memory and get slower and slower as the main activity is destroyed and recreated
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): mobile-only, fairly low risk
String or UUID changes made by this patch: none

Requesting this first patch for aurora and beta since both of those branches are vulnerable to this problem. However since I haven't heard of a lot of people complaining of this, maybe it's not worth uplifting.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 655741 [details] [diff] [review]
(2/2) - Move the LocationChange listener to Tabs.java

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: When "don't keep activities" is checked, going to/from about:home to some other fast-loading page may result in the wrong page getting displayed
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): mobile only. i would consider this medium risk but it's been baking on m-c long enough with no regressions, so i think it's safe for aurora
String or UUID changes made by this patch: none

Comment 21

[bhavana bajaj [:bajaj]]

Comment on attachment 655740 [details] [diff] [review]
(1/2) Unregister GeckoApp as a tab listener in destroy

Approving it for beta as we still have 3 weeks and this seems worth taking in based on "User Impact If declined" in comment 18

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Landed both patches on aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/5bbf21722e91
https://hg.mozilla.org/releases/mozilla-aurora/rev/a1959bc7c68f

Landed patch (1/2) on beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/1b0b9038652c

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Thanks, forgot to update the flags. However this won't actually be fixed on 16 - the second patch is needed to actually fix the problem.