Bug 1359618 - Prevent LayerView from accessing the compositor until UiCompositorControllerChild is open r=jchen draft
authorRandall Barker <rbarker@mozilla.com>
Tue, 25 Apr 2017 15:46:51 -0700
changeset 569886 b078ad0ac4a3b06ad9525170426a2d311ea0e9e4
parent 569885 cbf267741097838510073510a677b96110b7e831
child 626318 774006400bd07489cd21302937b830bdb50c6389
push id56298
push userbmo:rbarker@mozilla.com
push dateFri, 28 Apr 2017 00:16:23 +0000
reviewersjchen
bugs1359618
milestone55.0a1
Bug 1359618 - Prevent LayerView from accessing the compositor until UiCompositorControllerChild is open r=jchen MozReview-Commit-ID: 3SmdJx0QXW8
mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java
mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java
widget/android/GeneratedJNIWrappers.cpp
widget/android/GeneratedJNIWrappers.h
widget/android/nsWindow.cpp
--- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java
@@ -48,17 +48,16 @@ public class DynamicToolbarAnimator {
 
     private final Set<PinReason> mPinFlags = Collections.synchronizedSet(EnumSet.noneOf(PinReason.class));
 
     private final GeckoLayerClient mTarget;
     private LayerView.Compositor mCompositor;
     private final List<MetricsListener> mListeners;
     private ToolbarChromeProxy mToolbarChromeProxy;
     private int mMaxToolbarHeight;
-    private boolean mCompositorControllerOpen;
 
     public DynamicToolbarAnimator(GeckoLayerClient aTarget) {
         mTarget = aTarget;
         mListeners = new ArrayList<MetricsListener>();
     }
 
     public void addMetricsListener(MetricsListener aListener) {
         mListeners.add(aListener);
@@ -82,17 +81,17 @@ public class DynamicToolbarAnimator {
         for (MetricsListener listener : mListeners) {
             listener.onMetricsChanged(aMetrics);
         }
     }
 
     public void setMaxToolbarHeight(int maxToolbarHeight) {
         ThreadUtils.assertOnUiThread();
         mMaxToolbarHeight = maxToolbarHeight;
-        if (mCompositor != null) {
+        if (isCompositorReady()) {
             mCompositor.setMaxToolbarHeight(mMaxToolbarHeight);
         }
     }
 
     public int getCurrentToolbarHeight() {
         if ((mToolbarChromeProxy != null) && mToolbarChromeProxy.isToolbarChromeVisible()) {
             return mMaxToolbarHeight;
         }
@@ -105,37 +104,53 @@ public class DynamicToolbarAnimator {
     public boolean isPinned() {
         return !mPinFlags.isEmpty();
     }
 
     public boolean isPinnedBy(PinReason reason) {
         return mPinFlags.contains(reason);
     }
 
-    public void setPinned(boolean pinned, PinReason reason) {
-        if ((mCompositor != null) && (pinned != mPinFlags.contains(reason))) {
+    /* package */ void sendPinValueToCompositor(final boolean pinned, final PinReason reason) {
+        if (isCompositorReady()) {
              mCompositor.setPinned(pinned, reason.value);
         }
+    }
+
+    public void setPinned(final boolean pinned, final PinReason reason) {
+        // setPinned may be called from the main thread but compositor can only be accessed on UI thread
+        if (pinned != mPinFlags.contains(reason)) {
+            if (ThreadUtils.isOnUiThread() == true) {
+                sendPinValueToCompositor(pinned, reason);
+            } else {
+                ThreadUtils.postToUiThread(new Runnable() {
+                        @Override
+                    public void run() {
+                        sendPinValueToCompositor(pinned, reason);
+                    }
+                });
+            }
+        }
 
         if (pinned) {
             mPinFlags.add(reason);
         } else {
             mPinFlags.remove(reason);
         }
     }
 
     public void showToolbar(boolean immediately) {
-        if (mCompositor != null) {
+        if (isCompositorReady()) {
             mCompositor.sendToolbarAnimatorMessage(immediately ?
                 LayerView.REQUEST_SHOW_TOOLBAR_IMMEDIATELY : LayerView.REQUEST_SHOW_TOOLBAR_ANIMATED);
         }
     }
 
     public void hideToolbar(boolean immediately) {
-        if (mCompositor != null) {
+        if (isCompositorReady()) {
             mCompositor.sendToolbarAnimatorMessage(immediately ?
                 LayerView.REQUEST_HIDE_TOOLBAR_IMMEDIATELY : LayerView.REQUEST_HIDE_TOOLBAR_ANIMATED);
         }
     }
 
     /* package-private */ IntSize getViewportSize() {
         ThreadUtils.assertOnUiThread();
 
@@ -147,48 +162,37 @@ public class DynamicToolbarAnimator {
         return new IntSize(viewWidth, viewHeight);
     }
 
     public PointF getVisibleEndOfLayerView() {
         return new PointF(mTarget.getView().getWidth(),
             mTarget.getView().getHeight());
     }
 
-    private void dumpStateToCompositor() {
-        if ((mCompositor != null) && mCompositorControllerOpen) {
+    /* package-private */ void updateCompositor() {
+        if (isCompositorReady()) {
             mCompositor.setMaxToolbarHeight(mMaxToolbarHeight);
             if ((mToolbarChromeProxy != null) && mToolbarChromeProxy.isToolbarChromeVisible()) {
                 mCompositor.sendToolbarAnimatorMessage(LayerView.REQUEST_SHOW_TOOLBAR_IMMEDIATELY);
             } else {
                 mCompositor.sendToolbarAnimatorMessage(LayerView.REQUEST_HIDE_TOOLBAR_IMMEDIATELY);
             }
             for (PinReason reason : PinReason.values()) {
-              mCompositor.setPinned(mPinFlags.contains(reason), reason.value);
+                mCompositor.setPinned(mPinFlags.contains(reason), reason.value);
             }
-        } else if ((mCompositor != null) && !mCompositorControllerOpen) {
-            // Ask the UiCompositorControllerChild if it is open since the open message can
-            // sometimes be sent to a different instance of the LayerView such as when
-            // Fennec is being used in custom tabs.
-            mCompositor.sendToolbarAnimatorMessage(LayerView.IS_COMPOSITOR_CONTROLLER_OPEN);
         }
     }
 
     /* package-private */ void notifyCompositorCreated(LayerView.Compositor aCompositor) {
         ThreadUtils.assertOnUiThread();
         mCompositor = aCompositor;
-        dumpStateToCompositor();
     }
 
-    /* package-private */ void notifyCompositorControllerOpen() {
-        mCompositorControllerOpen = true;
-        dumpStateToCompositor();
-    }
-
-    /* package-private */ void notifyCompositorDestroyed() {
-        mCompositor = null;
+    private boolean isCompositorReady() {
+        return ((mCompositor != null) && (mCompositor.isReady()));
     }
 
 
     /* package-private */ Bitmap getBitmapOfToolbarChrome() {
         if (mToolbarChromeProxy != null) {
             return mToolbarChromeProxy.getBitmapOfToolbarChrome();
         }
         return null;
--- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java
@@ -66,18 +66,18 @@ public class LayerView extends FrameLayo
     private int mWidth, mHeight;
 
     private boolean onAttachedToWindowCalled;
     private int mDefaultClearColor = Color.WHITE;
     /* package */ GetPixelsResult mGetPixelsResult;
     private final List<DrawListener> mDrawListeners;
 
     /* This is written by the Gecko thread and the UI thread, and read by the UI thread. */
-    @WrapForJNI(stubName = "CompositorCreated", calledFrom = "ui")
     /* package */ volatile boolean mCompositorCreated;
+    /* package */ volatile boolean mCompositorControllerOpen;
 
     //
     // NOTE: These values are also defined in gfx/layers/ipc/UiCompositorControllerMessageTypes.h
     //       and must be kept in sync. Any new AnimatorMessageType added here must also be added there.
     //
     /* package */ final static int STATIC_TOOLBAR_NEEDS_UPDATE      = 0;  // Sent from compositor when the static toolbar wants to hide.
     /* package */ final static int STATIC_TOOLBAR_READY             = 1;  // Sent from compositor when the static toolbar image has been updated and is ready to animate.
     /* package */ final static int TOOLBAR_HIDDEN                   = 2;  // Sent to compositor when the real toolbar has been hidden.
@@ -97,20 +97,29 @@ public class LayerView extends FrameLayo
         ThreadUtils.postToUiThread(new Runnable() {
             @Override
             public void run() {
                 mCompositor.sendToolbarAnimatorMessage(message);
             }
         });
     }
 
+    @WrapForJNI(calledFrom = "ui")
+    /* package */ boolean isCompositorReady() {
+        ThreadUtils.assertOnUiThread();
+        return mCompositorCreated && mCompositorControllerOpen;
+    }
+
     /* package */ class Compositor extends JNIObject {
         public Compositor() {
         }
 
+        /* package */ boolean isReady() {
+            return isCompositorReady();
+        }
         @WrapForJNI(calledFrom = "ui", dispatchTo = "gecko")
         @Override protected native void disposeNative();
 
         // Gecko thread sets its Java instances; does not block UI thread.
         @WrapForJNI(calledFrom = "any", dispatchTo = "gecko")
         /* package */ native void attachToJava(GeckoLayerClient layerClient,
                                                NativePanZoomController npzc);
 
@@ -164,42 +173,36 @@ public class LayerView extends FrameLayo
         @WrapForJNI(calledFrom = "ui", dispatchTo = "current")
         /* package */ native void enableLayerUpdateNotifications(boolean enable);
 
         @WrapForJNI(calledFrom = "ui", dispatchTo = "current")
         /* package */ native void sendToolbarPixelsToCompositor(final int width, final int height, final int[] pixels);
 
         @WrapForJNI(calledFrom = "gecko")
         private void reattach() {
-            mCompositorCreated = true;
             ThreadUtils.postToUiThread(new Runnable() {
                 @Override
                 public void run() {
-                    // Make sure it is still valid
-                    if (mCompositorCreated) {
-                        mCompositor.setDefaultClearColor(mDefaultClearColor);
-                        mCompositor.enableLayerUpdateNotifications(!mDrawListeners.isEmpty());
-                        mToolbarAnimator.notifyCompositorCreated(mCompositor);
-                    }
+                    updateCompositor();
                 }
             });
         }
 
         @WrapForJNI(calledFrom = "gecko")
         private void destroy() {
             // The nsWindow has been closed. First mark our compositor as destroyed.
             LayerView.this.mCompositorCreated = false;
+            LayerView.this.mCompositorControllerOpen = false;
 
             LayerView.this.mLayerClient.setGeckoReady(false);
 
             // Then clear out any pending calls on the UI thread by disposing on the UI thread.
             ThreadUtils.postToUiThread(new Runnable() {
                 @Override
                 public void run() {
-                    LayerView.this.mToolbarAnimator.notifyCompositorDestroyed();
                     LayerView.this.mDrawListeners.clear();
                     disposeNative();
                 }
             });
         }
     }
 
     /* package */ void handleToolbarAnimatorMessage(int message) {
@@ -237,17 +240,31 @@ public class LayerView extends FrameLayo
                 setSurfaceBackgroundColor(Color.TRANSPARENT);
                 break;
             case LAYERS_UPDATED:
                 for (DrawListener listener : mDrawListeners) {
                     listener.drawFinished();
                 }
                 break;
             case COMPOSITOR_CONTROLLER_OPEN:
-                mToolbarAnimator.notifyCompositorControllerOpen();
+                // It is possible to get this message multiple times. Only act on it if we didn't know the compositor controller was open
+                if (mCompositorControllerOpen) {
+                    break;
+                }
+                mCompositorControllerOpen = true;
+                // updateCompositor makes a synchronous call to the compositor which will dead lock if called directly from here
+                ThreadUtils.postToUiThread(new Runnable() {
+                    @Override
+                    public void run() {
+                        mCompositor.setDefaultClearColor(mDefaultClearColor);
+                        mCompositor.enableLayerUpdateNotifications(!mDrawListeners.isEmpty());
+                        mToolbarAnimator.updateCompositor();
+                        updateCompositor();
+                    }
+                });
                 break;
             default:
                 Log.e(LOGTAG, "Unhandled Toolbar Animator Message: " + message);
                 break;
         }
     }
 
     private final Compositor mCompositor = new Compositor();
@@ -291,16 +308,17 @@ public class LayerView extends FrameLayo
     public void initializeView() {
         mLayerClient = new GeckoLayerClient(getContext(), this);
         if (mOverscroll != null) {
             mLayerClient.setOverscrollHandler(mOverscroll);
         }
 
         mPanZoomController = mLayerClient.getPanZoomController();
         mToolbarAnimator = mLayerClient.getDynamicToolbarAnimator();
+        mToolbarAnimator.notifyCompositorCreated(mCompositor);
 
         setFocusable(true);
         setFocusableInTouchMode(true);
 
         GeckoAccessibility.setDelegate(this);
     }
 
     /**
@@ -477,17 +495,17 @@ public class LayerView extends FrameLayo
 
     public void setSurfaceBackgroundColor(int newColor) {
         if (mSurfaceView != null) {
             mSurfaceView.setBackgroundColor(newColor);
         }
     }
 
     public void requestRender() {
-        if (mCompositorCreated) {
+        if (isCompositorReady()) {
             mCompositor.syncInvalidateAndScheduleComposite();
         }
     }
 
     public interface GetPixelsResult {
         public void onPixelsResult(int width, int height, IntBuffer pixels);
     }
 
@@ -498,17 +516,17 @@ public class LayerView extends FrameLayo
                 @Override
                 public void run() {
                     getPixels(getPixelsResult);
                 }
             });
             return;
         }
 
-        if (mCompositorCreated) {
+        if (isCompositorReady()) {
             mGetPixelsResult = getPixelsResult;
             mCompositor.requestScreenPixels();
         } else {
             getPixelsResult.onPixelsResult(0, 0, null);
         }
     }
 
     public void setListener(Listener listener) {
@@ -545,17 +563,17 @@ public class LayerView extends FrameLayo
         mServerSurfaceValid = true;
 
         updateCompositor();
     }
 
     void updateCompositor() {
         ThreadUtils.assertOnUiThread();
 
-        if (mCompositorCreated) {
+        if (isCompositorReady()) {
             // If the compositor has already been created, just resume it instead. We don't need
             // to block here because if the surface is destroyed before the compositor grabs it,
             // we can handle that gracefully (i.e. the compositor will remain paused).
             if (!mServerSurfaceValid) {
                 return;
             }
             // Asking Gecko to resume the compositor takes too long (see
             // https://bugzilla.mozilla.org/show_bug.cgi?id=735230#c23), so we
@@ -565,22 +583,23 @@ public class LayerView extends FrameLayo
             // been resumed, otherwise Gecko may send updates that get dropped.
             mCompositor.syncResumeResizeCompositor(mWidth, mHeight, getSurface());
             return;
         }
 
         // Only try to create the compositor if we have a valid surface and gecko is up. When these
         // two conditions are satisfied, we can be relatively sure that the compositor creation will
         // happen without needing to block anywhere.
-        if (mServerSurfaceValid && getLayerClient().isGeckoReady()) {
+        if (!mCompositorCreated && mServerSurfaceValid && getLayerClient().isGeckoReady()) {
             mCompositorCreated = true;
             mCompositor.createCompositor(mWidth, mHeight, getSurface());
-            mCompositor.setDefaultClearColor(mDefaultClearColor);
-            mCompositor.enableLayerUpdateNotifications(!mDrawListeners.isEmpty());
-            mToolbarAnimator.notifyCompositorCreated(mCompositor);
+        }
+
+        if (mCompositorCreated && !mCompositorControllerOpen) {
+            mCompositor.sendToolbarAnimatorMessage(IS_COMPOSITOR_CONTROLLER_OPEN);
         }
     }
 
     /* When using a SurfaceView (mSurfaceView != null), resizing happens in two
      * phases. First, the LayerView changes size, then, often some frames later,
      * the SurfaceView changes size. Because of this, we need to split the
      * resize into two phases to avoid jittering.
      *
@@ -598,17 +617,17 @@ public class LayerView extends FrameLayo
      * TextureView instead of a SurfaceView, the first phase is skipped.
      */
     private void onSizeChanged(int width, int height) {
         if (!mServerSurfaceValid || mSurfaceView == null) {
             surfaceChanged(width, height);
             return;
         }
 
-        if (mCompositorCreated) {
+        if (isCompositorReady()) {
             mCompositor.syncResumeResizeCompositor(width, height, getSurface());
         }
 
         if (mOverscroll != null) {
             mOverscroll.setSize(width, height);
         }
     }
 
@@ -634,17 +653,17 @@ public class LayerView extends FrameLayo
         // We need to coordinate with Gecko when pausing composition, to ensure
         // that Gecko never executes a draw event while the compositor is paused.
         // This is sent synchronously to make sure that we don't attempt to use
         // any outstanding Surfaces after we call this (such as from a
         // serverSurfaceDestroyed notification), and to make sure that any in-flight
         // Gecko draw events have been processed.  When this returns, composition is
         // definitely paused -- it'll synchronize with the Gecko event loop, which
         // in turn will synchronize with the compositor thread.
-        if (mCompositorCreated) {
+        if (isCompositorReady()) {
             mCompositor.syncPauseCompositor();
         }
 
         mServerSurfaceValid = false;
     }
 
     private void onDestroyed() {
         serverSurfaceDestroyed();
@@ -739,17 +758,17 @@ public class LayerView extends FrameLayo
                     addDrawListener(listener);
                 }
             });
             return;
         }
 
         boolean wasEmpty = mDrawListeners.isEmpty();
         mDrawListeners.add(listener);
-        if (mCompositorCreated && wasEmpty) {
+        if (isCompositorReady() && wasEmpty) {
             mCompositor.enableLayerUpdateNotifications(true);
         }
     }
 
     @RobocopTarget
     public void removeDrawListener(final DrawListener listener) {
         if (!ThreadUtils.isOnUiThread()) {
             ThreadUtils.postToUiThread(new Runnable() {
@@ -758,17 +777,17 @@ public class LayerView extends FrameLayo
                     removeDrawListener(listener);
                 }
             });
             return;
         }
 
         boolean notEmpty = mDrawListeners.isEmpty();
         mDrawListeners.remove(listener);
-        if (mCompositorCreated && notEmpty && mDrawListeners.isEmpty()) {
+        if (isCompositorReady() && notEmpty && mDrawListeners.isEmpty()) {
             mCompositor.enableLayerUpdateNotifications(false);
         }
     }
 
     @RobocopTarget
     public static interface DrawListener {
         public void drawFinished();
     }
@@ -816,13 +835,13 @@ public class LayerView extends FrameLayo
                 public void run() {
                     setClearColor(color);
                 }
             });
             return;
         }
 
         mDefaultClearColor = color;
-        if (mCompositorCreated) {
+        if (isCompositorReady()) {
             mCompositor.setDefaultClearColor(mDefaultClearColor);
         }
    }
 }
--- a/widget/android/GeneratedJNIWrappers.cpp
+++ b/widget/android/GeneratedJNIWrappers.cpp
@@ -1142,27 +1142,22 @@ const char LayerView::name[] =
 constexpr char LayerView::GetCompositor_t::name[];
 constexpr char LayerView::GetCompositor_t::signature[];
 
 auto LayerView::GetCompositor() const -> mozilla::jni::Object::LocalRef
 {
     return mozilla::jni::Method<GetCompositor_t>::Call(LayerView::mCtx, nullptr);
 }
 
-constexpr char LayerView::CompositorCreated_t::name[];
-constexpr char LayerView::CompositorCreated_t::signature[];
-
-auto LayerView::CompositorCreated() const -> bool
+constexpr char LayerView::IsCompositorReady_t::name[];
+constexpr char LayerView::IsCompositorReady_t::signature[];
+
+auto LayerView::IsCompositorReady() const -> bool
 {
-    return mozilla::jni::Field<CompositorCreated_t>::Get(LayerView::mCtx, nullptr);
-}
-
-auto LayerView::CompositorCreated(bool a0) const -> void
-{
-    return mozilla::jni::Field<CompositorCreated_t>::Set(LayerView::mCtx, nullptr, a0);
+    return mozilla::jni::Method<IsCompositorReady_t>::Call(LayerView::mCtx, nullptr);
 }
 
 const char LayerView::Compositor::name[] =
         "org/mozilla/gecko/gfx/LayerView$Compositor";
 
 constexpr char LayerView::Compositor::AttachToJava_t::name[];
 constexpr char LayerView::Compositor::AttachToJava_t::signature[];
 
--- a/widget/android/GeneratedJNIWrappers.h
+++ b/widget/android/GeneratedJNIWrappers.h
@@ -3557,36 +3557,34 @@ public:
         static const mozilla::jni::CallingThread callingThread =
                 mozilla::jni::CallingThread::UI;
         static const mozilla::jni::DispatchTarget dispatchTarget =
                 mozilla::jni::DispatchTarget::CURRENT;
     };
 
     auto GetCompositor() const -> mozilla::jni::Object::LocalRef;
 
-    struct CompositorCreated_t {
+    struct IsCompositorReady_t {
         typedef LayerView Owner;
         typedef bool ReturnType;
         typedef bool SetterType;
         typedef mozilla::jni::Args<> Args;
-        static constexpr char name[] = "mCompositorCreated";
-        static constexpr char signature[] =
-                "Z";
+        static constexpr char name[] = "isCompositorReady";
+        static constexpr char signature[] =
+                "()Z";
         static const bool isStatic = false;
         static const mozilla::jni::ExceptionMode exceptionMode =
                 mozilla::jni::ExceptionMode::ABORT;
         static const mozilla::jni::CallingThread callingThread =
                 mozilla::jni::CallingThread::UI;
         static const mozilla::jni::DispatchTarget dispatchTarget =
                 mozilla::jni::DispatchTarget::CURRENT;
     };
 
-    auto CompositorCreated() const -> bool;
-
-    auto CompositorCreated(bool) const -> void;
+    auto IsCompositorReady() const -> bool;
 
     static const mozilla::jni::CallingThread callingThread =
             mozilla::jni::CallingThread::UI;
 
 };
 
 class LayerView::Compositor : public mozilla::jni::ObjectBase<Compositor>
 {
--- a/widget/android/nsWindow.cpp
+++ b/widget/android/nsWindow.cpp
@@ -1158,17 +1158,17 @@ class nsWindow::PMPMSupport final
 {
     PMPMSupport() = delete;
 
     static LayerViewSupport* GetLayerViewSupport(jni::Object::Param aView)
     {
         const auto& layerView = LayerView::Ref::From(aView);
 
         LayerView::Compositor::LocalRef compositor = layerView->GetCompositor();
-        if (!layerView->CompositorCreated() || !compositor) {
+        if (!layerView->IsCompositorReady() || !compositor) {
             return nullptr;
         }
 
         LayerViewSupport* const lvs = LayerViewSupport::FromNative(compositor);
         if (!lvs) {
             // There is a pending exception whenever FromNative returns nullptr.
             compositor.Env()->ExceptionClear();
         }