LayerView not shown after resuming from an OOM kill

VERIFIED FIXED in Firefox 22

Status

()

defect
--
major
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

({regression})

Trunk
Firefox 23
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox22+ verified, firefox23+ fixed, firefox24+ fixed)

Details

Attachments

(2 attachments)

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.
tracking-fennec: --- → ?
Severity: normal → major
Duplicate of this bug: 864337
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.
Attachment #741038 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #741041 - Flags: review?(lucasr.at.mozilla)
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 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.
Attachment #741038 - Flags: review?(lucasr.at.mozilla) → review+
Attachment #741041 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/ff6868ca5426
https://hg.mozilla.org/mozilla-central/rev/0172e653051d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
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.
Attachment #741038 - Flags: approval-mozilla-beta?
Comment on attachment 741041 [details] [diff] [review]
Part 2: Remove unneccesary call to hideAboutHome()

[Approval Request Comment]
See above.
Attachment #741041 - Flags: approval-mozilla-beta?
Attachment #741038 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #741041 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: RESOLVED → VERIFIED
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.