LayerView not shown after resuming from an OOM kill

VERIFIED FIXED in Firefox 22

Status

()

Firefox for Android
General
--
major
VERIFIED FIXED
4 years ago
9 months ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

({regression})

Trunk
Firefox 23
ARM
Android
regression
Points:
---

Firefox Tracking Flags

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

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
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.

Updated

4 years ago
tracking-fennec: --- → ?

Updated

4 years ago
Severity: normal → major
Keywords: regression
(Assignee)

Updated

4 years ago
Duplicate of this bug: 864337
(Assignee)

Comment 2

4 years ago
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.
Attachment #741038 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 3

4 years ago
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.
Attachment #741041 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 4

4 years ago
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+

Updated

4 years ago
Attachment #741041 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 6

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff6868ca5426
https://hg.mozilla.org/integration/mozilla-inbound/rev/0172e653051d
https://hg.mozilla.org/mozilla-central/rev/ff6868ca5426
https://hg.mozilla.org/mozilla-central/rev/0172e653051d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
(Assignee)

Comment 8

4 years ago
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?
(Assignee)

Comment 9

4 years ago
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?

Updated

4 years ago
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox24: --- → fixed
tracking-firefox22: --- → +
tracking-firefox23: --- → +
tracking-firefox24: --- → +
status-firefox23: affected → fixed
(Assignee)

Comment 10

4 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/c89587a72be8
https://hg.mozilla.org/releases/mozilla-beta/rev/ee77f9c5b8bd
status-firefox22: affected → fixed

Updated

4 years ago
Attachment #741038 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

4 years ago
Attachment #741041 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

4 years ago
Status: RESOLVED → VERIFIED
status-firefox22: fixed → verified
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.