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

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.

> (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?

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.

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

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.

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

Some more code style fixes.

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

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.

Addressed comments.

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

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

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.