Bug 1491442 - Make sure to never trigger async CA transactions when the main thread might be resizing a window. r=mattwoodrow

This avoids most window resizing glitches.

Differential Revision: https://phabricator.services.mozilla.com/D40518

--HG--
extra : moz-landing-system : lando

widget/cocoa/nsChildView.h

@@ -20,6 +20,7 @@
 #include "nsCocoaUtils.h"
 #include "gfxQuartzSurface.h"
 #include "GLContextTypes.h"
+#include "mozilla/DataMutex.h"
 #include "mozilla/Mutex.h"
 #include "nsRegion.h"
 #include "mozilla/MouseEvents.h"
@@ -563,6 +564,24 @@ class nsChildView final : public nsBaseWidget {
   nsresult SetPrefersReducedMotionOverrideForTest(bool aValue) override;
   nsresult ResetPrefersReducedMotionOverrideForTest() override;
 
+  // Called when the main thread enters a phase during which visual changes
+  // are imminent and any layer updates on the compositor thread would interfere
+  // with visual atomicity.
+  // Has no effect if StaticPrefs::gfx_core_animation_enabled_AtStartup() is false.
+  // "Async" CATransactions are CATransactions which happen on a thread that's
+  // not the main thread.
+  void SuspendAsyncCATransactions();
+
+  // Called when we know that the current main thread paint will be completed once
+  // the main thread goes back to the event loop.
+  void MaybeScheduleUnsuspendAsyncCATransactions();
+
+  // Called from the runnable dispatched by MaybeScheduleUnsuspendAsyncCATransactions().
+  // At this point we know that the main thread is done handling the visual change
+  // (such as a window resize) and we can start modifying CALayers from the
+  // compositor thread again.
+  void UnsuspendAsyncCATransactions();
+
  protected:
   virtual ~nsChildView();
 
@@ -691,6 +710,24 @@ class nsChildView final : public nsBaseWidget {
   // Always null if StaticPrefs::gfx_core_animation_enabled_AtStartup() is true.
   RefPtr<mozilla::gfx::DrawTarget> mBackingSurface;
 
+  // Coordinates the triggering of CoreAnimation transactions between the main
+  // thread and the compositor thread in order to avoid glitches during window
+  // resizing and window focus changes.
+  struct WidgetCompositingState {
+    // While mAsyncCATransactionsSuspended is true, no CoreAnimation transaction
+    // should be triggered on a non-main thread, because they might race with
+    // main-thread driven updates such as window shape changes, and cause glitches.
+    bool mAsyncCATransactionsSuspended = false;
+
+    // Set to true if mNativeLayerRoot->ApplyChanges() needs to be called at the
+    // next available opportunity. Set to false whenever ApplyChanges does get
+    // called.
+    bool mNativeLayerChangesPending = false;
+  };
+  mozilla::DataMutex<WidgetCompositingState> mCompositingState;
+
+  RefPtr<mozilla::CancelableRunnable> mUnsuspendAsyncCATransactionsRunnable;
+
   // This flag is only used when APZ is off. It indicates that the current pan
   // gesture was processed as a swipe. Sometimes the swipe animation can finish
   // before momentum events of the pan gesture have stopped firing, so this

widget/cocoa/nsChildView.mm

@@ -337,6 +337,7 @@ nsChildView::nsChildView()
       mDrawing(false),
       mIsDispatchPaint(false),
       mPluginFocused{false},
+      mCompositingState("nsChildView::mCompositingState"),
       mCurrentPanGestureBelongsToSwipe{false} {}
 
 nsChildView::~nsChildView() {
@@ -820,6 +821,7 @@ void nsChildView::BackingScaleFactorChanged() {
     return;
   }
 
+  SuspendAsyncCATransactions();
   mBackingScaleFactor = newScale;
   NSRect frame = [mView frame];
   mBounds = nsCocoaUtils::CocoaRectToGeckoRectDevPix(frame, newScale);
@@ -872,6 +874,7 @@ void nsChildView::Resize(double aWidth, double aHeight, bool aRepaint) {
 
   if (!mView || (mBounds.width == width && mBounds.height == height)) return;
 
+  SuspendAsyncCATransactions();
   mBounds.width = width;
   mBounds.height = height;
 
@@ -906,6 +909,7 @@ void nsChildView::Resize(double aX, double aY, double aWidth, double aHeight, bo
     mBounds.y = y;
   }
   if (isResizing) {
+    SuspendAsyncCATransactions();
     mBounds.width = width;
     mBounds.height = height;
   }
@@ -928,6 +932,69 @@ void nsChildView::Resize(double aX, double aY, double aWidth, double aHeight, bo
   NS_OBJC_END_TRY_ABORT_BLOCK;
 }
 
+// The following three methods are primarily an attempt to avoid glitches during
+// window resizing.
+// Here's some background on how these glitches come to be:
+// CoreAnimation transactions are per-thread. They don't nest across threads.
+// If you submit a transaction on the main thread and a transaction on a
+// different thread, the two will race to the window server and show up on the
+// screen in the order that they happen to arrive in at the window server.
+// When the window size changes, there's another event that needs to be
+// synchronized with: the window "shape" change. Cocoa has built-in synchronization
+// mechanics that make sure that *main thread* window paints during window resizes
+// are synchronized properly with the window shape change. But no such built-in
+// synchronization exists for CATransactions that are triggered on a non-main
+// thread.
+// To cope with this, we define a "danger zone" during which we simply avoid
+// triggering any CATransactions on a non-main thread (called "async" CATransactions
+// here). This danger zone starts at the earliest opportunity at which we know
+// about the size change, which is nsChildView::Resize, and ends at a point at
+// which we know for sure that the paint has been handled completely, which is
+// when we return to the event loop after layer display.
+void nsChildView::SuspendAsyncCATransactions() {
+  if (!StaticPrefs::gfx_core_animation_enabled_AtStartup()) {
+    return;
+  }
+
+  if (mUnsuspendAsyncCATransactionsRunnable) {
+    mUnsuspendAsyncCATransactionsRunnable->Cancel();
+    mUnsuspendAsyncCATransactionsRunnable = nullptr;
+  }
+
+  // Make sure that there actually will be a CATransaction on the main thread
+  // during which we get a chance to schedule unsuspension. Otherwise we might
+  // accidentally stay suspended indefinitely.
+  [mView markLayerForDisplay];
+
+  auto compositingState = mCompositingState.Lock();
+  compositingState->mAsyncCATransactionsSuspended = true;
+}
+
+void nsChildView::MaybeScheduleUnsuspendAsyncCATransactions() {
+  auto compositingState = mCompositingState.Lock();
+  if (compositingState->mAsyncCATransactionsSuspended && !mUnsuspendAsyncCATransactionsRunnable) {
+    mUnsuspendAsyncCATransactionsRunnable =
+        NewCancelableRunnableMethod("nsChildView::MaybeScheduleUnsuspendAsyncCATransactions", this,
+                                    &nsChildView::UnsuspendAsyncCATransactions);
+    NS_DispatchToMainThread(mUnsuspendAsyncCATransactionsRunnable);
+  }
+}
+
+void nsChildView::UnsuspendAsyncCATransactions() {
+  mUnsuspendAsyncCATransactionsRunnable = nullptr;
+
+  auto compositingState = mCompositingState.Lock();
+  compositingState->mAsyncCATransactionsSuspended = false;
+  if (compositingState->mNativeLayerChangesPending) {
+    // We need to call mNativeLayerRoot->ApplyChanges() at the next available
+    // opportunity, and it needs to happen during a CoreAnimation transaction.
+    // The easiest way to handle this request is to mark the layer as needing
+    // display, because this will schedule a main thread CATransaction, during
+    // which HandleMainThreadCATransaction will call ApplyChanges().
+    [mView markLayerForDisplay];
+  }
+}
+
 nsresult nsChildView::SynthesizeNativeKeyEvent(int32_t aNativeKeyboardLayout,
                                                int32_t aNativeKeyCode, uint32_t aModifierFlags,
                                                const nsAString& aCharacters,
@@ -1446,6 +1513,7 @@ void nsChildView::PaintWindowInContentLayer() {
 }
 
 void nsChildView::HandleMainThreadCATransaction() {
+  MaybeScheduleUnsuspendAsyncCATransactions();
   WillPaintWindow();
 
   if (GetLayerManager()->GetBackendType() == LayersBackend::LAYERS_BASIC) {
@@ -1463,7 +1531,9 @@ void nsChildView::HandleMainThreadCATransaction() {
   // Apply the changes from mContentLayer to its underlying CALayer. Now is a
   // good time to call this because we know we're currently inside a main thread
   // CATransaction.
+  auto compositingState = mCompositingState.Lock();
   mNativeLayerRoot->ApplyChanges();
+  compositingState->mNativeLayerChangesPending = false;
 }
 
 #pragma mark -
@@ -1964,10 +2034,18 @@ void nsChildView::PostRender(WidgetRenderingContext* aContext) {
   if (StaticPrefs::gfx_core_animation_enabled_AtStartup()) {
     mContentLayer->NotifySurfaceReady();
 
-    // Force a CoreAnimation layer tree update from this thread.
-    [CATransaction begin];
-    mNativeLayerRoot->ApplyChanges();
-    [CATransaction commit];
+    auto compositingState = mCompositingState.Lock();
+    if (compositingState->mAsyncCATransactionsSuspended) {
+      // We should not trigger a CATransactions on this thread. Instead, let the
+      // main thread take care of calling ApplyChanges at an appropriate time.
+      compositingState->mNativeLayerChangesPending = true;
+    } else {
+      // Force a CoreAnimation layer tree update from this thread.
+      [CATransaction begin];
+      mNativeLayerRoot->ApplyChanges();
+      compositingState->mNativeLayerChangesPending = false;
+      [CATransaction commit];
+    }
   } else {
     UniquePtr<GLManager> manager(GLManager::CreateGLManager(aContext->mLayerManager));
     GLContext* gl = manager ? manager->gl() : aContext->mGL;
@@ -2463,7 +2541,13 @@ void nsChildView::TrackScrollEventAsSwipe(const mozilla::PanGestureInput& aSwipe
 
 void nsChildView::SwipeFinished() { mSwipeTracker = nullptr; }
 
-void nsChildView::UpdateBoundsFromView() { mBounds = CocoaPointsToDevPixels([mView frame]); }
+void nsChildView::UpdateBoundsFromView() {
+  auto oldSize = mBounds.Size();
+  mBounds = CocoaPointsToDevPixels([mView frame]);
+  if (mBounds.Size() != oldSize) {
+    SuspendAsyncCATransactions();
+  }
+}
 
 already_AddRefed<gfx::DrawTarget> nsChildView::StartRemoteDrawingInRegion(
     LayoutDeviceIntRegion& aInvalidRegion, BufferMode* aBufferMode) {