Closed Bug 1293299 Opened 3 years ago Closed 3 years ago

Opening a custom tab in Fennec displays a white screen

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: droeh, Assigned: droeh)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 8 obsolete files)

Opening a custom tab in Fennec currently displays a white screen rather than rendering the page, and causes similar behavior in Fennec afterwards.
Hi Dylan,

Can you give more information about the device, build and the website you are loading?
Priority: -- → P2
Attached patch FennecView (obsolete) — Splinter Review
This patch adds a class, FennecView (derived from GeckoView) which shares a single window among multiple instances. It fixes the "white screen" issue with custom tabs, but currently breaks APZ (to be addressed in another patch).
Attachment #8780148 - Flags: review?(snorp)
Attachment #8780148 - Flags: review?(nchen)
Attached patch NPZCSupport patch (obsolete) — Splinter Review
This fixes the APZ breakage of the first patch by not destroying the corresponding NativePanZoomController when we call DetachFromWindow on an NPZCSupport. I'm kind of half-expecting some awful side effect here, but it seems to work.
Attachment #8780234 - Flags: review?(nchen)
Comment on attachment 8780234 [details] [diff] [review]
NPZCSupport patch

Do we at least know why this works? Are we not reattaching NPZC correctly? Fixing shutdown problems by not shutting down is not really a solution.
Attachment #8780234 - Flags: review?(nchen) → review-
Yeah, sorry, I should've explained that when I put up the patch. Basically, consider the sequence of events:

(1) We open Fennec, maybe open a few tabs, browse as usual
(2) We open another app that opens a custom tab
(3) We reopen Fennec and (try to) continue to browse

When (2) happens we create a new FennecView and thus a new NativePanZoomController; when we call attachToJava we create a new NPZCSupport corresponding to the new PanZoomController -- this (the NPZCSupport constructor, specifically) updates the nsWindow to store the new NPZCSupport and calls DetachFromWindow on the original NPZCSupport, which destroys the original NativePanZoomController. Then, in (3), we switch back to the original FennecView, which tries to use the original NativePanZoomController but fails because it has been destroyed.

I think maybe a better approach than modifying DetachFromWindow as I've done here might be to leave it alone and update the NPZCSupport constructor to not call DetachFromWindow at all. That way when we call DetachFromWindow elsewhere (eg the LayerViewSupport destructor) we're still getting the same old behavior, but we can attach a new NPZC without destroying the old one, which is the goal.

Thoughts?
Flags: needinfo?(nchen)
Attached patch NPZCSupport patch (obsolete) — Splinter Review
This just removes the DetachFromWindow call from the NPZCSupport constructor; DetachFromWindow is left untouched.
Attachment #8780234 - Attachment is obsolete: true
Attachment #8780658 - Flags: review?(nchen)
Blocks: 1294821
Comment on attachment 8780148 [details] [diff] [review]
FennecView

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

I'm not too thrilled about this. FennecView moves us away from GeckoView. Even outside of custom tabs, we now do extra things in onWindowVisibilityChanged, which could introduce regressions. It exposes GeckoView.Window, which is supposed to be a private object within GeckoView.

I think it's better to extend the onSaveInstanceState/onRestoreInstanceState interface that GeckoView exposes, and do a save/restore whenever the custom view opens/closes. For example, in pseudo-code,

> class CustomTab {
>   void onOpen() {
>     final Object state = GeckoApp.geckoView.saveState();
>     this.geckoView.restoreState(state);
>   }
>   void onClose() {
>     final Object state = this.geckoView.saveState();
>     GeckoApp.geckoView.restoreState(state);
>   }
> }

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/FennecView.java
@@ +35,5 @@
> +            window = mWindow;
> +            openWindow();
> +        }
> +
> +        if (window == null) {

Make it an else block

@@ +44,5 @@
> +    }
> +
> +    @Override
> +    public void onWindowVisibilityChanged(int visibility) {
> +        if (visibility == VISIBLE && visibility != mVisibility) {

Why do we need to keep mVisibility? Can onWindowVisibilityChanged be called with the same value in a row?

::: widget/android/AndroidBridgeUtilities.h
@@ +1,5 @@
>  #ifndef ALOG
>  #if defined(DEBUG) || defined(FORCE_ALOG)
>  #define ALOG(args...)  __android_log_print(ANDROID_LOG_INFO, "Gecko" , ## args)
>  #else
> +#define ALOG(args...)  ((void)0)

What's this for?

::: widget/android/nsWindow.cpp
@@ +208,5 @@
> +            } else {
> +                nsAppShell::SyncRunEvent(WindowEvent<Functor>(
> +                        mozilla::Move(aCall)));
> +                return;
> +            }

Why is a sync call necessary again?

@@ +1006,5 @@
>          mSurface = mCompositor->GetSurface();
>          return mSurface.Get();
>      }
>  
> +    void setCompositor(const LayerView::Compositor::LocalRef& aInstance) {

nit: CamelCase

@@ +1008,5 @@
>      }
>  
> +    void setCompositor(const LayerView::Compositor::LocalRef& aInstance) {
> +        mCompositor = aInstance;
> +        Base::AttachNative(aInstance, this);

We attach a new Java instance here, but we never detach the previous Java instance.
Attachment #8780148 - Flags: review?(nchen) → feedback+
Comment on attachment 8780658 [details] [diff] [review]
NPZCSupport patch

This needs rebasing because of bug 1293709.
Flags: needinfo?(nchen)
Attachment #8780658 - Flags: review?(nchen)
Attached patch FennecView patch (updated) (obsolete) — Splinter Review
This combines the two patches, rebases, and hopefully addresses all of the above.
Attachment #8780148 - Attachment is obsolete: true
Attachment #8780658 - Attachment is obsolete: true
Attachment #8784487 - Flags: review?(snorp)
Attachment #8784487 - Flags: review?(nchen)
Comment on attachment 8784487 [details] [diff] [review]
FennecView patch (updated)

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

Did you see comment 7?

> I think it's better to extend the onSaveInstanceState/onRestoreInstanceState interface that GeckoView exposes, and do a save/restore whenever the custom view opens/closes.
Attachment #8784487 - Flags: review?(nchen)
Yeah, sorry, miscommunication. Snorp and I talked about that and he said that I should go ahead with this approach.
Attached patch FennecView patch (updated) (obsolete) — Splinter Review
Rebased because of bug 1296757. Let's talk on irc if you're still unhappy with the direction.
Attachment #8784487 - Attachment is obsolete: true
Attachment #8784487 - Flags: review?(snorp)
Attachment #8784865 - Flags: review?(snorp)
Attachment #8784865 - Flags: review?(nchen)
Comment on attachment 8784865 [details] [diff] [review]
FennecView patch (updated)

(Unsetting r? as we're going to be seeing a different patch soon)
Attachment #8784865 - Flags: review?(snorp)
Attached patch GeckoViewFragment patch (obsolete) — Splinter Review
This patch uses a Fragment to pass state around between GeckoViews as necessary, and eliminates FennecView.
Attachment #8784865 - Attachment is obsolete: true
Attachment #8784865 - Flags: review?(nchen)
Attachment #8786046 - Flags: review?(snorp)
Attachment #8786046 - Flags: review?(nchen)
Comment on attachment 8786046 [details] [diff] [review]
GeckoViewFragment patch

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

Looks good to me with nits

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewFragment.java
@@ +16,5 @@
> +public class GeckoViewFragment extends android.support.v4.app.Fragment {
> +    private static final String LOGTAG = "GeckoViewFragment";
> +
> +    private static Parcelable state = null;
> +    private GeckoView mGeckoView = null;

Apparently it's no longer a thing to use mFoo, so just calls this geckoView I guess.

@@ +17,5 @@
> +    private static final String LOGTAG = "GeckoViewFragment";
> +
> +    private static Parcelable state = null;
> +    private GeckoView mGeckoView = null;
> +    private ViewGroup mParentViewGroup = null;

Ditto

@@ +33,5 @@
> +    }
> +
> +    @Override
> +    public void onResume() {
> +        if (state != null) {

I wonder if we also only want to do this if the active GeckoViewFragment changed? You could track that in some static variable. That way, if you're just hitting the home button and then switch back to Fennec no extra work occurs.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java
@@ +472,5 @@
>      Listener getListener() {
>          return mListener;
>      }
>  
> +    public void attachCompositor() {

I would remove the 'public' which makes it 'package' visibility. Usually we annotate this by putting it as /* package */

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java
@@ +257,5 @@
> +    }
> +
> +    @Override
> +    public void revive() {
> +        mDestroyed = false;

This is definitely wonky...but not sure I have a better solution
Attachment #8786046 - Flags: review?(snorp) → review+
Comment on attachment 8786046 [details] [diff] [review]
GeckoViewFragment patch

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

::: mobile/android/base/resources/layout/customtabs_activity.xml
@@ +25,5 @@
>              android:layout_height="match_parent"
>              android:layout_below="@+id/tablet_tab_strip"
>              android:layout_above="@+id/find_in_page">
>  
> +            <fragment class="org.mozilla.gecko.GeckoViewFragment" 

Nit: trailng space

::: mobile/android/base/resources/layout/gecko_app.xml
@@ +25,5 @@
>                          android:layout_height="match_parent"
>                          android:layout_below="@+id/tablet_tab_strip"
>                          android:layout_above="@+id/find_in_page">
>  
> +            <fragment class="org.mozilla.gecko.GeckoViewFragment" 

Nit: trailng space

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +201,5 @@
>          return new StateBinder(superState, this.window);
>      }
>  
>      @Override
> +    protected Parcelable onSaveInstanceState()

Why separate out saveInstanceState from onSaveInstanceState?

@@ +219,5 @@
>          stateSaved = false;
>      }
>  
>      @Override
> +    protected void onRestoreInstanceState(final Parcelable state)

Likewise with restoreInstanceState and onRestoreInstanceState.

@@ +242,5 @@
> +
> +    /* package */ void reattachWindow() {
> +        // If we have destroyed the PZC we need to let it know it is being undestroyed.
> +        if (getPanZoomController().isDestroyed()) {
> +            getPanZoomController().revive();

Get rid of this block and get rid of isDestroyed(). Rename revive() to attach(). Call attach() inside the Runnable inside GeckoLayerClient.onGeckoReady(), and initialize mDestroyed to true in NativePanZoomController. That should ensure that the NPZC is re-enabled every time.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewFragment.java
@@ +36,5 @@
> +    public void onResume() {
> +        if (state != null) {
> +            // "Restore" the window from the previously used GeckoView to this GeckoView and attach it
> +            mGeckoView.restoreInstanceState(state);
> +            mGeckoView.reattachWindow();

reattachWindow should be called inside GeckoView.onRestoreInstanceState, but only if GeckoView.onAttachedToWindow has already been called.

@@ +37,5 @@
> +        if (state != null) {
> +            // "Restore" the window from the previously used GeckoView to this GeckoView and attach it
> +            mGeckoView.restoreInstanceState(state);
> +            mGeckoView.reattachWindow();
> +            mGeckoView.attachCompositor();

attachCompositor should be called inside LayerView.onRestoreInstanceState, but only if LayerView.onAttachedToWindow has already been called.

::: widget/android/nsWindow.cpp
@@ +300,5 @@
>              // can get a head start on opening our window.
>              return aCall();
> +        } else if (aCall.IsTarget(&GeckoViewSupport::Reattach)) {
> +            // Ensure this is synchronous so that we don't call (eg)
> +            // CreateCompositor before we have actually reattached.

The proper way to fix this is to unset the Gecko ready flag in GeckoLayerClient when the Compositor is destroyed (i.e. set GeckoLayerClient.mGeckoReady to false in LayerView.Compositor.destroy).

@@ +1064,5 @@
>      using Base::DisposeNative;
>  
>      void OnDetach()
>      {
> +        mCompositor->Destroy(); 

Nit: remove this change.

@@ +1083,5 @@
>          mSurface = mCompositor->GetSurface();
>          return mSurface;
>      }
>  
> +    LayerView::Compositor::GlobalRef GetCompositor() {

You're returning a copy of GlobalRef. Return LayerView::Compositor::Param instead.

@@ +1425,5 @@
>  {
>      // Associate our previous GeckoEditable with the new GeckoView.
>      mEditable->OnViewChange(aView);
>  
> +    // If we aren't changing compositors, we don't want to actually do anything here.

Reattaching again should be harmless, no?
Attachment #8786046 - Flags: review?(nchen) → feedback+
Updated to contain all of snorp's and Jim's recommendations, with one exception:

I left saveInstanceState/restoreInstanceState separated out from onSaveInstanceState/onRestoreInstanceState as we need to call them from GeckoViewFragment, and onSaveInstanceState/onRestoreInstanceState are protected.
Attachment #8786046 - Attachment is obsolete: true
Attachment #8786513 - Flags: review?(nchen)
Comment on attachment 8786513 [details] [diff] [review]
GeckoViewFragment patch (updated)

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

(In reply to Dylan Roeh (:droeh) from comment #17)
> Created attachment 8786513 [details] [diff] [review]
> GeckoViewFragment patch (updated)
> 
> Updated to contain all of snorp's and Jim's recommendations, with one
> exception:
> 
> I left saveInstanceState/restoreInstanceState separated out from
> onSaveInstanceState/onRestoreInstanceState as we need to call them from
> GeckoViewFragment, and onSaveInstanceState/onRestoreInstanceState are
> protected.

You can still call protected methods in GeckoView from GeckoViewFragment, but the "correct" method to use is probably FragmentManager.saveFragmentInstanceState, or otherwise View.saveHierarchyState.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +220,5 @@
> +        }
> +
> +        // We have to always call super.onRestoreInstanceState because View keeps
> +        // track of these calls and throws an exception when we don't call it.
> +        super.onRestoreInstanceState(stateBinder.superState);

Separating restoreInstanceState from onRestoreInstanceState is undesirable, because here you're calling the superclass's onRestoreInstanceState from restoreInstanceState, which may not come from an onRestoreInstanceState call.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewFragment.java
@@ +17,5 @@
> +    private static final String LOGTAG = "GeckoViewFragment";
> +
> +    private static Parcelable state = null;
> +    private GeckoView geckoView = null;
> +    private static GeckoViewFragment lastUsed = null;

Nit: keep static members together.

@@ +21,5 @@
> +    private static GeckoViewFragment lastUsed = null;
> +
> +    @Override
> +    public void onCreate(Bundle savedInstanceState) {
> +        Log.i(LOGTAG, "DYLAN -- GeckoViewFragment.onCreate called");

Nit: remove logging.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/GeckoLayerClient.java
@@ +88,5 @@
>       *    that because mViewportMetrics might get reassigned in between reading the different
>       *    fields. */
>      private volatile ImmutableViewportMetrics mViewportMetrics;
>  
> +    protected volatile boolean mGeckoIsReady;

Let this stay private and add a method to unset it.

@@ +93,2 @@
>  
>      private final PanZoomController mPanZoomController;

Change mPanZoomController to package-private.

@@ +93,4 @@
>  
>      private final PanZoomController mPanZoomController;
>      private final DynamicToolbarAnimator mToolbarAnimator;
>      private final LayerView mView;

Change mView to package-private.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java
@@ +71,5 @@
>  
>      private boolean mServerSurfaceValid;
>      private int mWidth, mHeight;
>  
> +    protected boolean onAttachedToWindowCalled;

I'd rather you make this private and keep a separate flag in GeckoView.

@@ +166,5 @@
>  
>          mPaintState = PAINT_START;
>          mFullScreenState = FullScreenState.NONE;
>  
> +        onAttachedToWindowCalled = false;

Don't need this.

::: widget/android/nsWindow.cpp
@@ +1031,5 @@
>      {
>          return mSurface;
>      }
>  
> +    LayerView::Compositor::Param GetCompositor() {

Remove this now that it's not used
Attachment #8786513 - Flags: review?(nchen) → feedback+
This addresses what you brought up above.

However: I did not use either of your suggestions for saving/restoring -- I don't think either of them is workable. You cannot save/restoreHierarchyState between views belonging to different activities, so that's right out -- as soon as you open a custom tab everything is hosed. For saveFragmentInstanceState, I don't see how that helps us -- once you have a saved state, you can't restore it to an existing fragment as far as I can tell, only create a new fragment with it. Moreover, even if we could (eg) restore a Fennec Fragment to a custom tabs Fragment, that would not be something we want to do -- the fragment stores the GeckoView, and we cannot share a GeckoView between different activities.

Instead I added save/restoreWindow to GeckoView, which just do the parts of onSave/RestoreInstanceState that we actually need here.
Attachment #8786513 - Attachment is obsolete: true
Attachment #8786858 - Flags: review?(nchen)
Comment on attachment 8786858 [details] [diff] [review]
GeckoViewFragment patch (updated)

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +195,5 @@
>          GeckoAppShell.setLayerView(this);
>  
>          initializeView(EventDispatcher.getInstance());
> +
> +        onAttachedToWindowCalled = false;

Nit: don't need this.

@@ +286,4 @@
>      }
>  
>      @Override
>      public void onDetachedFromWindow()

Unset onAttachedToWindowCalled in onDetachedFromWindow

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java
@@ +430,5 @@
>      Listener getListener() {
>          return mListener;
>      }
>  
> +    protected void attachCompositor() {

Keep attachCompositor private. Use another onAttachedToWindowCalled flag in LayerView and call attachCompositor from LayerView.onRestoreInstanceState.
OK, here's a version with (hopefully) everything fixed.
Attachment #8786858 - Attachment is obsolete: true
Attachment #8786858 - Flags: review?(nchen)
Attachment #8786911 - Flags: review?(nchen)
Comment on attachment 8786911 [details] [diff] [review]
GeckoViewFragment patch (updated)

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

LGTM
Attachment #8786911 - Flags: review?(nchen) → review+
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57d9b63474d1
Create a class, GeckoViewFragment, which passes GeckoView state between different activities. r=jchen
https://hg.mozilla.org/mozilla-central/rev/57d9b63474d1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Blocks: 1300574
See Also: → 1300574
You need to log in before you can comment on or make changes to this bug.