863803 - LayerView not shown after resuming from an OOM kill

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

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 2

[Brian Nicholson (:bnicholson)]

Attachment: 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)]

Attachment: 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)]

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)]

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 6

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff6868ca5426
https://hg.mozilla.org/integration/mozilla-inbound/rev/0172e653051d

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff6868ca5426
https://hg.mozilla.org/mozilla-central/rev/0172e653051d

Comment 8

[Brian Nicholson (:bnicholson)]

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)]

Comment on attachment 741041 [details] [diff] [review]
Part 2: Remove unneccesary call to hideAboutHome()

[Approval Request Comment]
See above.

Comment 10

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-beta/rev/c89587a72be8
https://hg.mozilla.org/releases/mozilla-beta/rev/ee77f9c5b8bd