1305439 - Switching to Fennec when a custom tab is open results in a white screen

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dylan Roeh (:droeh) (he/him)]

If you open a custom tab and then switch to Fennec, it appears to be focused on the custom tab, but just displays a white screen. Switching tabs gets Fennec to render the page normally.

Comment 1

[Dylan Roeh (:droeh) (he/him)]

Attachment: Save/restore last selected tab in GeckoApp

Sebastian, do you mind reviewing this since it's fairly front-endy? It's a simple fix that just stores the selected tab when a GeckoApp is paused and selects that tab when a GeckoApp is resumed. It kills two birds with one stone, as we don't have this white screen issue when switching back to Fennec while a custom tab is open, and we also don't have CustomTabsActivity potentially displaying the wrong tab if we switch back to it from Fennec.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8800259 [details] [diff] [review]
Save/restore last selected tab in GeckoApp

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

LGTM.

Do we need to store/load the value in onSaveInstanceState() and onRestoreInstanceState() too? In case Android decides to kill the Activity while it is in the background. You can test this with "Don't keep activities" enabled in the developer settings. Maybe it doesn't matter here because we take the whole process with us when BrowserApp is destroyed.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46db039dbe10
Store the last selected tab in GeckoApp in onPause and switch back to it in onResume. r=sebastian

Comment 4

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20754fd4499a
Backed out changeset 46db039dbe10 for failing testAboutPage on Android. r=backout

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed this out for failing testAboutPage on Android:

https://hg.mozilla.org/integration/mozilla-inbound/rev/20754fd4499aedb03b7dfeeb5499c18bfbb1c497

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=46db039dbe102cf4529df8fe237b20455c3f154d
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=37588084&repo=mozilla-inbound

11:45:54     INFO -  TEST-PASS | testAboutPage | Url is correct - mochi.test:8888/tests/robocop/robocop_blank_01.html should equal mochi.test:8888/tests/robocop/robocop_blank_01.html
11:45:54     INFO -  TEST-PASS | testAboutPage | Menu view is not null - android.widget.FrameLayout{41a46488 V.E...C. ......ID 704,0-800,96 #7f09009c app:id/menu} should not equal null
11:45:54     INFO -  TEST-PASS | testAboutPage | Waiting for menu item ^Settings$ - ^Settings$ is present and enabled
11:45:54     INFO -  TEST-PASS | testAboutPage | Waiting for and scrolling once to find item ^Mozilla Nightly$ - ^Mozilla Nightly$ found
11:45:54     INFO -  TEST-PASS | testAboutPage | Waiting for enabled text ^Mozilla Nightly$ - ^Mozilla Nightly$ option is present and enabled
11:45:54     INFO -  TEST-PASS | testAboutPage | Waiting for and scrolling once to find item ^About (Fennec|Nightly|Firefox Aurora|Firefox Beta|Firefox)$ - ^About (Fennec|Nightly|Firefox Aurora|Firefox Beta|Firefox)$ found
11:45:54     INFO -  TEST-PASS | testAboutPage | Waiting for enabled text ^About (Fennec|Nightly|Firefox Aurora|Firefox Beta|Firefox)$ - ^About (Fennec|Nightly|Firefox Aurora|Firefox Beta|Firefox)$ option is present and enabled
11:45:54     INFO -  TEST-PASS | testAboutPage | Given message occurred for registered event: {"parentId":0,"delayLoad":false,"title":"about:","selected":true,"isPrivate":false,"stub":true,"external":false,"desktopMode":false,"tabIndex":-1,"tabID":1,"cancelEditMode":false,"type":"Tab:Added","uri":"about:"} - Tab:Added should equal Tab:Added
11:45:54     INFO -  TEST-PASS | testAboutPage | Given message occurred for registered event: {"errorType":"","bgColor":"rgb(206, 215, 222)","type":"DOMContentLoaded","tabID":1} - DOMContentLoaded should equal DOMContentLoaded
11:45:54     INFO -  EventExpecter: no longer listening for Tab:Added
11:45:54     INFO -  EventExpecter: no longer listening for DOMContentLoaded
11:45:55     INFO -  TEST-PASS | testAboutPage | The url argument is not null - about: should not equal null
11:45:55     INFO -  waitForCondition timeout after 15000 ms.
11:45:55  WARNING -  TEST-UNEXPECTED-FAIL | testAboutPage | Url is correct - got mochi.test:8888/tests/robocop/robocop_blank_01.html, expected about:
11:45:55     INFO -  0 ERROR Exception caught during test! - junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testAboutPage | Url is correct - got mochi.test:8888/tests/robocop/robocop_blank_01.html, expected about:
11:45:55     INFO -  	at junit.framework.Assert.fail(Assert.java:50)
11:45:55     INFO -  	at org.mozilla.gecko.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:124)
11:45:55     INFO -  	at org.mozilla.gecko.FennecMochitestAssert.ok(FennecMochitestAssert.java:145)
11:45:55     INFO -  	at org.mozilla.gecko.FennecMochitestAssert.is(FennecMochitestAssert.java:150)
11:45:55     INFO -  	at org.mozilla.gecko.tests.BaseTest.verifyUrlInContentDescription(BaseTest.java:493)
11:45:55     INFO -  	at org.mozilla.gecko.tests.testAboutPage.testAboutPage(testAboutPage.java:45)
11:45:55     INFO -  	at java.lang.reflect.Method.invokeNative(Native Method)
11:45:55     INFO -  	at java.lang.reflect.Method.invoke(Method.java:525)
11:45:55     INFO -  	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
11:45:55     INFO -  	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
11:45:55     INFO -  	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
11:45:55     INFO -  	at org.mozilla.gecko.tests.BaseRobocopTest.runTest(BaseRobocopTest.java:188)
11:45:55     INFO -  	at junit.framework.TestCase.runBare(TestCase.java:134)
11:45:55     INFO -  	at junit.framework.TestResult$1.protect(TestResult.java:115)
11:45:55     INFO -  	at junit.framework.TestResult.runProtected(TestResult.java:133)
11:45:55     INFO -  	at junit.framework.TestResult.run(TestResult.java:118)
11:45:55     INFO -  	at junit.framework.TestCase.run(TestCase.java:124)
11:45:55     INFO -  	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191)
11:45:55     INFO -  	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176)
11:45:55     INFO -  	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554)
11:45:55     INFO -  	at org.mozilla.gecko.FennecInstrumentationTestRunner.onStart(FennecInstrumentationTestRunner.java:64)
11:45:55     INFO -  	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701)

Comment 6

[Dylan Roeh (:droeh) (he/him)]

Attachment: Save/restore last selected tab in GeckoApp (updated)

Right, so this turned out to be a bit more complicated than it initially seemed. Since GeckoPreferences is a separate activity, we were restoring the previous tab after opening about: in testAboutPage. Likewise I realized we had a similar problem with GeckoApp.onNewIntent() -- some intents should override the behavior of restoring the previous tab. I've also updated it to save/restore the last selected tab in onSave/RestoreInstanceState -- thanks for catching that!

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8801312 [details] [diff] [review]
Save/restore last selected tab in GeckoApp (updated)

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

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +168,5 @@
>      private static final int CLEANUP_DEFERRAL_SECONDS = 15;
>  
>      private static boolean sAlreadyLoaded;
>  
> +    private static GeckoApp lastActiveGeckoApp;

This is a problem. This static reference will keep the activity in the process and the JVM can't garbage collect it.

E.g. when switching from a custom tab to the browser then we will keep a reference to the custom tab activity (and everything attached) until the browser is paused again and we replace the reference.

Replacing the reference in onResume() and/or using a WeakReference is slightly better but in general holding static references to a Context object is considered an anti-pattern. I wonder if we could avoid this? (Maybe just keeping the hashCode() for comparison is an option?).

@@ +205,5 @@
>      private boolean mSessionRestoreParsingFinished = false;
>  
>      private EventDispatcher eventDispatcher;
>  
> +    private Tab lastSelectedTab = null;

If we save/restore just the tab id then I wonder if we only need to keep the id here (Just to avoid holding on to a tab for too long).

Comment 8

[Dylan Roeh (:droeh) (he/him)]

Attachment: Save/restore last selected tab in GeckoApp (updated)

I went back to the drawing board on this one since it seemed like it was getting ugly; in this patch I store the last selected *browser* tab ID in GeckoApp, update it in BrowserApp.onPause, and restore it in CustomTabsActivity.onPause; additionally, each CustomTabsActivity stores its own tab ID in onPause and restores it (if it's valid) in onResume.

This way, we do not need to change the behavior of GeckoApp or BrowserApp at all -- they never call selectTab -- which makes this quite a bit simpler and safer, I think.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8802581 [details] [diff] [review]
Save/restore last selected tab in GeckoApp (updated)

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

I don't understand the patch, maybe I'm just confused.

* BrowserApp is setting lastSelectedBrowserTab and therefore it is saved and restored (implementation in GeckoApp). However it is not using the value for anything.

* CustomTabsActivity has its own field: customTabId. This field is used in onResume() / onPause(), but never saved/restored in a onSaveInstanceState/onRestoreInstanceState. Theoretically it uses lastSelectedBrowserTab, but it is not set/written by a CustomTabsActivity.

I like the approach of just using the tab ID. What I was expecting is:

* One implementation to save/restore the last tab in GeckoApp - with maybe a method that can provide a tab id and the implementation might be different/overriden in BrowserApp/CustomTabsActivity. Does CustomTabsActivity really need its own tab id handling?

* Code to select the restored tab (Don't we need this in BrowserApp too?)

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8802581 [details] [diff] [review]
Save/restore last selected tab in GeckoApp (updated)

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

I tried a build locally and I think this approach only works if the Browser was started before the custom tab (and the custom tab is started in the same process; e.g. no process kill until activity is switched).

STR:
* Open a tab ("A") in the browser
* Leave the browser and kill it
* Open a custom tab ("B") in an app
* Open the browser again. Now tab "B" is selected.

=> A new process was started with the custom tab and lastSelectedBrowserTab is null. It is not restored because BrowserApp is not launched again. Therefore it can't be selected in onPause() of CustomTabActivity.

I wonder if we can get this working without letting BrowserApp select the last tab itself. What do you think? I'll clear review, but reflag me again if needed!

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +592,5 @@
> +    @Override
> +    protected void onRestoreInstanceState(final Bundle inState) {
> +        super.onRestoreInstanceState(inState);
> +
> +        lastSelectedBrowserTab = inState.getInt(SAVED_STATE_LAST_TAB);

All this code in GeckoApp seems to be specific for BrowserApp. If we continue using this approach then we could just move the code to BrowserApp and access BrowserApp.lastSelectedBrowserTab from the CustomTabsActivity?

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
@@ +16,5 @@
>  import org.mozilla.gecko.util.NativeJSObject;
>  import org.mozilla.gecko.util.ThreadUtils;
>  
>  public class CustomTabsActivity extends GeckoApp {
> +    private int customTabId = -1;

We might need to save and restore this value in onSaveInstanceState/onRestoreInstanceState of CustomTabsActivity - in case the activity is destroyed while in background.

STR (with "Don't keep activities" enabled to always simulate the activity being destroyed):

* Open custom tab A in app X
* Press 'home' (activity is getting destroyed in the background)
* Open custom tab B in app Y
* Switch back to app X: Activity is re-created, customTabId is not set. Nothing gets selected and tab B is visible (with white screen issue as described in comment 0)

Comment 11

[Dylan Roeh (:droeh) (he/him)]

Attachment: Save/restore last selected tab in GeckoApp v2

Yeah, having spent a while thinking about it I don't think that approach is salvageable without basically going down the same road the first approach went, so I cleaned up the previous patch and updated it with the suggestions you made in comment 7. Let me know what you think.

Comment 12

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8804850 [details] [diff] [review]
Save/restore last selected tab in GeckoApp v2

Forgot to flag this for review.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8804850 [details] [diff] [review]
Save/restore last selected tab in GeckoApp v2

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

(Sorry for being a PITA reviewer here)

I think we are almost there. Apart from the lastActiveGeckoApp thing I mentioned below I see one last issue: When pausing and resuming the same custom tab (with "don't keep activities" on) I see a white page. It's fixed after switching to the browser or a different tab. This might be unrelated and I remember a similar bug - So maybe something we do not need to handle in this bug.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +2091,5 @@
>          }
>  
>          GeckoAppShell.setGeckoInterface(this);
>  
> +        if (lastSelectedTabId >= 0 && lastActiveGeckoApp.get() != this) {

I saw this crashing once with a NullPointerException (lastActiveGeckoApp = null).

I guess this can happen in situations where the activity gets destroyed (and the process later too). The next time the app is resumed the activity is re-created and the tab id is restored but lastActiveGeckoApp is not set.

But do we even need to check if this is not the same as the last active gecko app? Selecting the same tab again doesn't do anything and removing this check seems to work fine here. Is there an edge case I am missing?

Comment 14

[Dylan Roeh (:droeh) (he/him)]

Attachment: Save/restore last selected tab in GeckoApp v3

(In reply to Sebastian Kaspari (:sebastian) from comment #13)
> Comment on attachment 8804850 [details] [diff] [review]
> Save/restore last selected tab in GeckoApp v2
> 
> Review of attachment 8804850 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (Sorry for being a PITA reviewer here)
> 
> I think we are almost there. Apart from the lastActiveGeckoApp thing I
> mentioned below I see one last issue: When pausing and resuming the same
> custom tab (with "don't keep activities" on) I see a white page. It's fixed
> after switching to the browser or a different tab. This might be unrelated
> and I remember a similar bug - So maybe something we do not need to handle
> in this bug.

I can't reproduce this, but there's a pretty good chance that was bug 1310195, which I landed a patch for earlier this week.

> ::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
> @@ +2091,5 @@
> >          }
> >  
> >          GeckoAppShell.setGeckoInterface(this);
> >  
> > +        if (lastSelectedTabId >= 0 && lastActiveGeckoApp.get() != this) {
> 
> I saw this crashing once with a NullPointerException (lastActiveGeckoApp =
> null).
> 
> I guess this can happen in situations where the activity gets destroyed (and
> the process later too). The next time the app is resumed the activity is
> re-created and the tab id is restored but lastActiveGeckoApp is not set.

Good catch, that's easy enough to fix. If we don't have a lastActiveGeckoApp I think we want to restore the last selected tab (provided it has been set); this version of the patch does so.

> But do we even need to check if this is not the same as the last active
> gecko app? Selecting the same tab again doesn't do anything and removing
> this check seems to work fine here. Is there an edge case I am missing?

Yep, the reason the first patch bounced is because of this issue; to give an example, if we go to Settings > Mozilla Fennec > About Fennec, GeckoPreferences opens a new tab at about: and then finishes, and we should return to Fennec focused on that (new) tab -- selecting the previously selected tab is the wrong behavior in that case.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe3fe09d24c8
Store the last selected tab in GeckoApp in onPause and switch back to it in onResume. r=sebastian

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe3fe09d24c8