Bug 1491442 - Make sure to never trigger async CA transactions when the main thread might be resizing a window. r=mattwoodrow
authorMarkus Stange <mstange@themasta.com>
Fri, 16 Aug 2019 01:14:47 +0000
changeset 488413 a5ba7777641703116a83c6b4bbea66308fb6e816
parent 488412 24dbae685801b451fba51d0e3788a821038c9321
child 488414 e6e88a4fc6b95571d04db7841603bea7148df02f
push id36443
push userccoroiu@mozilla.com
push dateFri, 16 Aug 2019 09:48:15 +0000
treeherdermozilla-central@5d4cbfe103bb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmattwoodrow
bugs1491442
milestone70.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
widget/cocoa/nsChildView.h
widget/cocoa/nsChildView.mm
--- a/widget/cocoa/nsChildView.h
+++ b/widget/cocoa/nsChildView.h
@@ -15,16 +15,17 @@
 
 #include "nsISupports.h"
 #include "nsBaseWidget.h"
 #include "nsIWeakReferenceUtils.h"
 #include "TextInputHandler.h"
 #include "nsCocoaUtils.h"
 #include "gfxQuartzSurface.h"
 #include "GLContextTypes.h"
+#include "mozilla/DataMutex.h"
 #include "mozilla/Mutex.h"
 #include "nsRegion.h"
 #include "mozilla/MouseEvents.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/webrender/WebRenderTypes.h"
 #include "mozilla/gfx/MacIOSurface.h"
 
 #include "nsString.h"
@@ -558,16 +559,34 @@ class nsChildView final : public nsBaseW
   void DispatchAPZWheelInputEvent(mozilla::InputData& aEvent, bool aCanTriggerSwipe);
   nsEventStatus DispatchAPZInputEvent(mozilla::InputData& aEvent);
 
   void SwipeFinished();
 
   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();
 
   void ReportMoveEvent();
   void ReportSizeEvent();
 
   void TearDownView();
 
@@ -686,16 +705,34 @@ class nsChildView final : public nsBaseW
   mozilla::UniquePtr<mozilla::VibrancyManager> mVibrancyManager;
   RefPtr<mozilla::SwipeTracker> mSwipeTracker;
   mozilla::UniquePtr<mozilla::SwipeEventQueue> mSwipeEventQueue;
 
   // Only used for drawRect-based painting in popups.
   // 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
   // flag tells us that we shouldn't allow the remaining events to cause
   // scrolling. It is reset to false once a new gesture starts (as indicated by
   // a PANGESTURE_(MAY)START event).
   bool mCurrentPanGestureBelongsToSwipe;
 
--- a/widget/cocoa/nsChildView.mm
+++ b/widget/cocoa/nsChildView.mm
@@ -332,16 +332,17 @@ nsChildView::nsChildView()
       mIsFullscreen(false),
       mIsOpaque(false),
       mTitlebarCGContext(nullptr),
       mBackingScaleFactor(0.0),
       mVisible(false),
       mDrawing(false),
       mIsDispatchPaint(false),
       mPluginFocused{false},
+      mCompositingState("nsChildView::mCompositingState"),
       mCurrentPanGestureBelongsToSwipe{false} {}
 
 nsChildView::~nsChildView() {
   ReleaseTitlebarCGContext();
 
   if (mSwipeTracker) {
     mSwipeTracker->Destroy();
     mSwipeTracker = nullptr;
@@ -815,16 +816,17 @@ void nsChildView::BackingScaleFactorChan
   CGFloat newScale = nsCocoaUtils::GetBackingScaleFactor(mView);
 
   // ignore notification if it hasn't really changed (or maybe we have
   // disabled HiDPI mode via prefs)
   if (mBackingScaleFactor == newScale) {
     return;
   }
 
+  SuspendAsyncCATransactions();
   mBackingScaleFactor = newScale;
   NSRect frame = [mView frame];
   mBounds = nsCocoaUtils::CocoaRectToGeckoRectDevPix(frame, newScale);
 
   if (StaticPrefs::gfx_core_animation_enabled_AtStartup()) {
     mNativeLayerRoot->SetBackingScale(mBackingScaleFactor);
   }
 
@@ -867,16 +869,17 @@ void nsChildView::Move(double aX, double
 void nsChildView::Resize(double aWidth, double aHeight, bool aRepaint) {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
   int32_t width = NSToIntRound(aWidth);
   int32_t height = NSToIntRound(aHeight);
 
   if (!mView || (mBounds.width == width && mBounds.height == height)) return;
 
+  SuspendAsyncCATransactions();
   mBounds.width = width;
   mBounds.height = height;
 
   ManipulateViewWithoutNeedingDisplay(mView, ^{
     [mView setFrame:DevPixelsToCocoaPoints(mBounds)];
   });
 
   if (mVisible && aRepaint) {
@@ -901,16 +904,17 @@ void nsChildView::Resize(double aX, doub
   BOOL isResizing = (mBounds.width != width || mBounds.height != height);
   if (!mView || (!isMoving && !isResizing)) return;
 
   if (isMoving) {
     mBounds.x = x;
     mBounds.y = y;
   }
   if (isResizing) {
+    SuspendAsyncCATransactions();
     mBounds.width = width;
     mBounds.height = height;
   }
 
   ManipulateViewWithoutNeedingDisplay(mView, ^{
     [mView setFrame:DevPixelsToCocoaPoints(mBounds)];
   });
 
@@ -923,16 +927,79 @@ void nsChildView::Resize(double aX, doub
     ReportMoveEvent();
     if (mOnDestroyCalled) return;
   }
   if (isResizing) ReportSizeEvent();
 
   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,
                                                const nsAString& aUnmodifiedCharacters,
                                                nsIObserver* aObserver) {
   AutoObserverNotifier notifier(aObserver, "keyevent");
   return mTextInputHandler->SynthesizeNativeKeyEvent(
       aNativeKeyboardLayout, aNativeKeyCode, aModifierFlags, aCharacters, aUnmodifiedCharacters);
@@ -1441,16 +1508,17 @@ void nsChildView::PaintWindowInContentLa
   }
 
   PaintWindowInIOSurface(
       surf, LayoutDeviceIntRegion::FromUnknownRegion(mContentLayer->CurrentSurfaceInvalidRegion()));
   mContentLayer->NotifySurfaceReady();
 }
 
 void nsChildView::HandleMainThreadCATransaction() {
+  MaybeScheduleUnsuspendAsyncCATransactions();
   WillPaintWindow();
 
   if (GetLayerManager()->GetBackendType() == LayersBackend::LAYERS_BASIC) {
     // We're in BasicLayers mode, i.e. main thread software compositing.
     // Composite the window into our layer's surface.
     PaintWindowInContentLayer();
   } else {
     // Trigger a synchronous OMTC composite. This will call NextSurface and
@@ -1458,17 +1526,19 @@ void nsChildView::HandleMainThreadCATran
     // surface, and the main thread (this thread) will wait inside PaintWindow
     // during that time.
     PaintWindow(LayoutDeviceIntRegion(GetBounds()));
   }
 
   // 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 -
 
 void nsChildView::ReportMoveEvent() { NotifyWindowMoved(mBounds.x, mBounds.y); }
 
 void nsChildView::ReportSizeEvent() {
   if (mWidgetListener) mWidgetListener->WindowResized(this, mBounds.width, mBounds.height);
@@ -1959,20 +2029,28 @@ bool nsChildView::PreRenderImpl(WidgetRe
   }
   return true;
 }
 
 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;
     NSOpenGLContext* glContext =
         gl ? GLContextCGL::Cast(gl)->GetNSOpenGLContext() : mGLPresenter->GetNSOpenGLContext();
     [mView postRender:glContext];
   }
   mViewTearDownLock.Unlock();
@@ -2458,17 +2536,23 @@ void nsChildView::TrackScrollEventAsSwip
 
   if (!mAPZC) {
     mCurrentPanGestureBelongsToSwipe = true;
   }
 }
 
 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) {
   if (StaticPrefs::gfx_core_animation_enabled_AtStartup()) {
     MOZ_RELEASE_ASSERT(mBasicCompositorIOSurface,
                        "Should have been set up by nsChildView::PreRender");
 
     mContentLayer->InvalidateRegionThroughoutSwapchain(aInvalidRegion.ToUnknownRegion());