1346542 - Back button navigation and web content copy/paste broken in restored GeckoApp instance

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Jan Henning [:JanH]]

STR:
1. Launch Firefox normally
2. Open a custom tab
3. Click on a link within the custom tab activity
4. Press the device's back button.

Expected:
1. We're going back in session history until eventually the custom tab is closed again.

Actual:
1. Nothing.

This can also be reproduced the other way round - open a custom tab first, and then the normal UI afterwards. In that case, the back button won't function in Firefox (long pressing still works, though).

Comment 1

[Julian Chu [:walkingice]]

Tried to bisect this bug, but cannot find the specific commit which might cause regression. At lease from rev *becff35a0bed14b536bb0a141b0e9640e9cb063d*[1] the back key still work, and it doesn't work in rev *da0ea1c722078f30c6f390627d3c680d3556a7a6*[2]

[1] https://hg.mozilla.org/mozilla-central/rev/becff35a0bed14b536bb0a141b0e9640e9cb063d
[2] https://hg.mozilla.org/mozilla-central/rev/da0ea1c722078f30c6f390627d3c680d3556a7a6

Comment 2

[Julian Chu [:walkingice]]

I keep bisect and have a little progress. In this commit[1] the back key still works like normal. After this commit[2], if I make it build-able[3] and try again, back key starts not working.(Actually back key completely doesn't work even if I only open normal page in Fennec without using Custom-Tab).

I might be wrong cause I mostly used artifact build to give a try, and I am not familiar with *.cpp files. Just curious about that whether bug 1343613 might effect back key in CustomTabs?

[1] https://hg.mozilla.org/mozilla-central/rev/0fd44a0d6351f6bebaa94477e64f89b5f58bb796
[2] https://hg.mozilla.org/mozilla-central/rev/f859107b689bdcbb2ba976ba0f48ac2e1b2a76e6
[3] have to cherry-pick this patch to pass build https://hg.mozilla.org/mozilla-central/rev/1673d1fc6164d14517a61401968fe9188c4a73a1

Comment 6

[Jan Henning [:JanH]]

Through bug 1349131, I've realised that this can happen without custom tabs as well - anything that triggers the mIsRestoringActivity code path is enough. So instead of using custom tabs, enabling "Don't keep activities" in Android's developer options equally reproduces this bug (and in the wild, this can happen when short of memory or if we've been in the background for some time).

Also, according to bug 1349131 this breaks native copy/paste from/to Gecko as well.

[Tracking Requested - why for this release]: See bug 1349131 - back button navigation and copy/paste can stop working.

Comment 7

[Julien Wajsberg [:julienw]]

> (and in the wild, this can happen when short of memory or if we've been in the background for some time).

This could definitely be what happens to me in bug 1349131, as I use a Z3C with only 2GB of memory. So I agree with the dupe here.

Comment 8

[Julien Cristau [:jcristau] (back April 22)]

Moving tracking flag from 1349131.

Comment 9

[Julian Chu [:walkingice]]

I traced the path of `GeckoApp.onBackPressed`. The back key doesn't work due to `isReadyForDispatchingToGecko`[1] return false. the State-Holder is set from native code and I have no idea how it works.

[1] https://dxr.mozilla.org/mozilla-central/rev/9577ddeaafd85554c2a855f385a87472a089d5c0/mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java#239

Comment 10

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1346542-1.0-Set-GeckoView-ready-state-when-resto.patch

We don't receive a "chrome-document-loaded" event when restoring an activity, so GeckoView is not set to ready state.

We need to set it to ready state somewhere in the restoring path.

Jan, does this work for all your use cases?

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8852183 [details] [diff] [review]
0001-Bug-1346542-1.0-Set-GeckoView-ready-state-when-resto.patch

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

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +1749,4 @@
>                  geckoConnected();
>                  if (mLayerView != null) {
>                      mLayerView.setPaintState(LayerView.PAINT_BEFORE_FIRST);
> +                    mLayerView.setState(GeckoView.State.READY);

I think we want to do this in GeckoView (we want it to work for geckoview_example). Also, we don't know if it's actually ready at this point, so we need to set the state from the Gecko thread, maybe inside nsWindow::GeckoViewSupport::Reattac, provided nsWindow::GeckoViewSupport::EnableEventDispatcher has been called already.

Comment 12

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

this will be blocker both for releasing custom tab and standalone mode

Comment 13

[Jan Henning [:JanH]]

Comment on attachment 8852183 [details] [diff] [review]
0001-Bug-1346542-1.0-Set-GeckoView-ready-state-when-resto.patch

Yup, seems to work fine now.

Comment 14

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1346542-1.1-Move-state-holder-to-GeckoView-Windo.patch

As discussed, I've moved out the state holder into GeckoView::Window and added holder swapping to the event dispatcher.

Comment 15

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1346542-2.0-Fix-code-style.-r-jchen.patch

Some more code style fixes.

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8852682 [details] [diff] [review]
0002-Bug-1346542-2.0-Fix-code-style.-r-jchen.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +117,5 @@
>  
>      // Object to hold onto our nsWindow connection when GeckoView gets destroyed.
>      private static class StateBinder extends Binder implements Parcelable {
> +        public final Parcelable mSuperState;
> +        public final Window mWindow;

Public members don't use prefix.

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8852681 [details] [diff] [review]
0001-Bug-1346542-1.1-Move-state-holder-to-GeckoView-Windo.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java
@@ +56,4 @@
>          new HashMap<String, List<BundleEventListener>>(DEFAULT_BACKGROUND_EVENTS_COUNT);
>  
>      private boolean mAttachedToGecko;
> +    private final AtomicReference<StateHolder> mStateHolder =

Just use `private volatile StateHolder`

@@ +73,5 @@
> +        mStateHolder.set(stateHolder);
> +    }
> +
> +    /* package */ void setStateHolder(final NativeQueue.StateHolder stateHolder) {
> +        mStateHolder.set(stateHolder);

I think you need `State currentState = stateHolder.getState(); stateHolder.checkAndSetState(currentState , currentState);` here, because if the new StateHolder has a different state than the old StateHolder, we want to flush any existing queued calls.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +89,4 @@
>      @WrapForJNI(dispatchTo = "proxy")
>      protected static final class Window extends JNIObject {
>          @WrapForJNI(skip = true)
> +        private final StateHolder mStateHolder =

Make it package-private

::: widget/android/nsWindow.cpp
@@ +265,4 @@
>      }
>  
>      GeckoViewSupport(nsWindow* aWindow,
> +                     GeckoView::Window::Param aInstance)

Keep using "const GeckoView::Window::LocalRef&" for aInstance

@@ +1253,4 @@
>  
>      // Attach a new GeckoView support object to the new window.
>      window->mGeckoViewSupport = mozilla::MakeUnique<GeckoViewSupport>(
> +                                window, aWindow);

Keep using "GeckoView::Window::LocalRef(aCls.Env(), aWindow)"

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7106a8bc897f
[1.2] Move state holder to GeckoView::Window and set ready state when reattaching to window. r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7ac485f20cf
[2.1] Fix code style. r=jchen

Comment 20

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1346542-2.1-Fix-code-style.-r-jchen.patch

Addressed comments.

Comment 21

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1346542-1.2-Move-state-holder-to-GeckoView-Windo.patch

Addressed comments.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc9240c4811
[3.0] Change setState to checkAndSetState to avoid updated state override. r=me

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7106a8bc897f
https://hg.mozilla.org/mozilla-central/rev/a7ac485f20cf
https://hg.mozilla.org/mozilla-central/rev/4cc9240c4811

Comment 24

[Sorina Florean [:sflorean]]

Verified as fixed in build 55.0a1 from 2017-04-02.
Device: Huawei Honor (Android 5.1.1).

If the device back button is tapped in custom tab session we're going back accordingly to the links we visited. Same after we open in Nightly.