Closed Bug 1346542 Opened 3 years ago Closed 3 years ago

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

Categories

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

All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
fennec 55+ ---
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 + verified

People

(Reporter: JanH, Assigned: esawin)

References

(Blocks 3 open bugs)

Details

Attachments

(2 files, 3 obsolete files)

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).
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
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
Flags: needinfo?(esawin)
Duplicate of this bug: 1349918
Duplicate of this bug: 1348794
Priority: -- → P1
Duplicate of this bug: 1349131
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.
tracking-fennec: --- → ?
Summary: Custom tabs: Back button navigation broken in second GeckoApp instance → Back button navigation and web content copy/paste broken in second GeckoApp instance
Summary: Back button navigation and web content copy/paste broken in second GeckoApp instance → Back button navigation and web content copy/paste broken in restored GeckoApp instance
> (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.
Moving tracking flag from 1349131.
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
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?
Assignee: nobody → esawin
Flags: needinfo?(esawin)
Attachment #8852183 - Flags: review?(nchen)
Attachment #8852183 - Flags: feedback?(jh+bugzilla)
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.
Attachment #8852183 - Flags: review?(nchen) → feedback+
this will be blocker both for releasing custom tab and standalone mode
tracking-fennec: ? → 55+
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.
Attachment #8852183 - Flags: feedback?(jh+bugzilla) → feedback+
As discussed, I've moved out the state holder into GeckoView::Window and added holder swapping to the event dispatcher.
Attachment #8852183 - Attachment is obsolete: true
Attachment #8852681 - Flags: review?(nchen)
Some more code style fixes.
Attachment #8852682 - Flags: review?(nchen)
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.
Attachment #8852682 - Flags: review?(nchen) → review+
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)"
Attachment #8852681 - Flags: review?(nchen) → review+
Duplicate of this bug: 1350198
Pushed by esawin@mozilla.com:
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
Addressed comments.
Attachment #8852682 - Attachment is obsolete: true
Attachment #8853368 - Flags: review+
Addressed comments.
Attachment #8852681 - Attachment is obsolete: true
Attachment #8853369 - Flags: review+
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc9240c4811
[3.0] Change setState to checkAndSetState to avoid updated state override. r=me
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.
Status: RESOLVED → VERIFIED
Depends on: 1351169
You need to log in before you can comment on or make changes to this bug.