Last Comment Bug 781883 - Cannot go from about:home to about:firefox with "Don't Keep Activities" enabled
: Cannot go from about:home to about:firefox with "Don't Keep Activities" enabled
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 17 Branch
: ARM Android
: -- normal (vote)
: Firefox 18
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
:
Mentors:
Depends on:
Blocks: 769269
  Show dependency treegraph
 
Reported: 2012-08-10 11:42 PDT by Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
Modified: 2016-07-29 14:29 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
fixed
verified


Attachments
Logcat output (562.15 KB, text/plain)
2012-08-10 22:27 PDT, Michael Comella (:mcomella) [not actively working on fennec: expect slow responses]
no flags Details
(1/2) Unregister GeckoApp as a tab listener in destroy (1007 bytes, patch)
2012-08-27 13:36 PDT, Kartikaya Gupta (email:kats@mozilla.com)
sriram.mozilla: review+
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
Details | Diff | Splinter Review
(2/2) - Move the LocationChange listener to Tabs.java (8.99 KB, patch)
2012-08-27 13:37 PDT, Kartikaya Gupta (email:kats@mozilla.com)
sriram.mozilla: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Michael Comella (:mcomella) [not actively working on fennec: expect slow responses] 2012-08-10 11:42:08 PDT
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:kats@mozilla.com) 2012-08-10 13:24:32 PDT
Works for me on the Galaxy Tab 10.1, Android 3.1
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-10 13:25:40 PDT
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) [not actively working on fennec: expect slow responses] 2012-08-10 22:25:59 PDT
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) [not actively working on fennec: expect slow responses] 2012-08-10 22:27:26 PDT
Created attachment 651095 [details]
Logcat output
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-12 18:05:11 PDT
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 2012-08-12 22:15:40 PDT
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) [not actively working on fennec: expect slow responses] 2012-08-12 22:21:22 PDT
Not exclusive to tablets.
Comment 8 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-27 12:21:54 PDT
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:kats@mozilla.com) 2012-08-27 13:32:31 PDT
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:kats@mozilla.com) 2012-08-27 13:36:26 PDT
Created attachment 655740 [details] [diff] [review]
(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] 2012-08-27 13:37:42 PDT
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:kats@mozilla.com) 2012-08-27 13:37:57 PDT
Created attachment 655741 [details] [diff] [review]
(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] 2012-08-27 13:41:11 PDT
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:kats@mozilla.com) 2012-08-27 17:33:39 PDT
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 16 Aaron Train [:aaronmt] 2012-08-31 09:23:30 PDT
Channel uplift?
Comment 17 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-31 09:30:45 PDT
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:kats@mozilla.com) 2012-08-31 09:48:52 PDT
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:kats@mozilla.com) 2012-09-08 00:09:44 PDT
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:kats@mozilla.com) 2012-09-08 00:12:08 PDT
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] 2012-09-10 15:27:20 PDT
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 23 Kartikaya Gupta (email:kats@mozilla.com) 2012-09-11 08:21:22 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.