Last Comment Bug 863803 - LayerView not shown after resuming from an OOM kill
: LayerView not shown after resuming from an OOM kill
Status: VERIFIED FIXED
: regression
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- major (vote)
: Firefox 23
Assigned To: Brian Nicholson (:bnicholson)
:
: Sebastian Kaspari (:sebastian)
Mentors:
: 864337 (view as bug list)
Depends on:
Blocks: 838793
  Show dependency treegraph
 
Reported: 2013-04-19 11:30 PDT by Brian Nicholson (:bnicholson)
Modified: 2016-07-29 14:33 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
fixed
+
fixed


Attachments
Part 1: Prevent multiple AboutHome fragments from being created after restore (2.62 KB, patch)
2013-04-23 14:50 PDT, Brian Nicholson (:bnicholson)
lucasr.at.mozilla: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Part 2: Remove unneccesary call to hideAboutHome() (826 bytes, patch)
2013-04-23 14:55 PDT, Brian Nicholson (:bnicholson)
lucasr.at.mozilla: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Brian Nicholson (:bnicholson) 2013-04-19 11:30:44 PDT
STR:
1) Open Fennec
2) Kill Fennec via OOM (using https://hg.mozilla.org/users/blassey_mozilla.com/oom-fennec/, or opening several apps while Fennec is in the background)
3) Reopen Fennec

After this, the LayerView doesn't appear, meaning the user is permanently stuck on about:home.

Seems to be a regression from bug 838793.
Comment 1 Brian Nicholson (:bnicholson) 2013-04-22 10:56:23 PDT
*** Bug 864337 has been marked as a duplicate of this bug. ***
Comment 2 Brian Nicholson (:bnicholson) 2013-04-23 14:50:59 PDT
Created attachment 741038 [details] [diff] [review]
Part 1: Prevent multiple AboutHome fragments from being created after restore

The problem is that we're actually creating and attaching two AboutHome fragments after an OOM restore. The first one is created at the beginning of the Activity's onCreate() automatically (since fragments automatically save and restore their state as part of the Activity lifecycle). The second fragment is created when we call AboutHome.newInstance(). We set mAboutHome to this new instance, and we detach it when we leave about:home, but the old fragment remains.

We can fix this by simply checking for the existence of an AboutHome fragment before creating a new instance.
Comment 3 Brian Nicholson (:bnicholson) 2013-04-23 14:55:26 PDT
Created attachment 741041 [details] [diff] [review]
Part 2: Remove unneccesary call to hideAboutHome()

Another thing I noticed when testing OOM restores and about:home is that about:home briefly appears, disappears, then appears again. This happens because we're calling hideAboutHome() if the restore mode is RESTORE_OOM, and it's later shown again when the tab selected callback runs and calls showAboutHome().

Since we changed AboutHome to be a fragment, there's no need for us to call hideAboutHome() here any more. The fragment is added dynamically, so it will only be attached/shown at startup if we're actually on about:home.
Comment 4 Brian Nicholson (:bnicholson) 2013-04-23 14:58:19 PDT
Comment on attachment 741038 [details] [diff] [review]
Part 1: Prevent multiple AboutHome fragments from being created after restore

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

::: mobile/android/base/BrowserApp.java
@@ +1232,4 @@
>          // it can't be used between the Activity's onSaveInstanceState() and
>          // onResume().
>          getSupportFragmentManager().beginTransaction()
> +                .add(R.id.gecko_layout, mAboutHome, "abouthome").commitAllowingStateLoss();

Oops, I meant to factor out this string. I changed this locally to:

+                .add(R.id.gecko_layout, mAboutHome, ABOUTHOME_TAG).commitAllowingStateLoss();
Comment 5 Lucas Rocha (:lucasr) 2013-04-24 06:18:41 PDT
Comment on attachment 741038 [details] [diff] [review]
Part 1: Prevent multiple AboutHome fragments from being created after restore

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

Nice.

::: mobile/android/base/BrowserApp.java
@@ +511,5 @@
>              }
>          });
>  
> +        // Find the Fragment if it was already added from a restored instance state.
> +        mAboutHome = (AboutHome) getSupportFragmentManager().findFragmentByTag(ABOUTHOME_TAG);

My bad, missed that case on my original review.
Comment 8 Brian Nicholson (:bnicholson) 2013-05-14 14:49:18 PDT
Comment on attachment 741038 [details] [diff] [review]
Part 1: Prevent multiple AboutHome fragments from being created after restore

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 838793
User impact if declined: user is stuck on about:home
Testing completed (on m-c, etc.): m-c several weeks
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none

Only needed if bug 838793 is uplifted.
Comment 9 Brian Nicholson (:bnicholson) 2013-05-14 14:49:36 PDT
Comment on attachment 741041 [details] [diff] [review]
Part 2: Remove unneccesary call to hideAboutHome()

[Approval Request Comment]
See above.

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