From 763a1114a3e9aabc741840169c8b55ca775bff89 Mon Sep 17 00:00:00 2001
From: Doug Sherk <dsherk2@mozilla.com>
Date: Tue, 7 Aug 2012 18:51:13 -0700
Subject: [PATCH] Bug 779572: Properly protect state in AsyncPanZoomController
 with a monitor r=cjones

---
 gfx/layers/ipc/AsyncPanZoomController.cpp | 30 ++++++++++++++---------
 gfx/layers/ipc/AsyncPanZoomController.h   | 17 +++++++++++++
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp
index c40bdd75476..626f50a8eb4 100644
--- a/gfx/layers/ipc/AsyncPanZoomController.cpp
+++ b/gfx/layers/ipc/AsyncPanZoomController.cpp
@@ -243,12 +243,15 @@ nsEventStatus AsyncPanZoomController::OnTouchStart(const MultiTouchInput& aEvent
       ScheduleComposite();
       // Fall through.
     case FLING:
-      CancelAnimation();
+      {
+        MonitorAutoLock monitor(mMonitor);
+        CancelAnimation();
+      }
       // Fall through.
     case NOTHING:
       mX.StartTouch(xPos);
       mY.StartTouch(yPos);
-      mState = TOUCHING;
+      SetState(TOUCHING);
       break;
     case TOUCHING:
     case PANNING:
@@ -320,7 +323,7 @@ nsEventStatus AsyncPanZoomController::OnTouchEnd(const MultiTouchInput& aEvent)
     return nsEventStatus_eIgnore;
 
   case TOUCHING:
-    mState = NOTHING;
+    SetState(NOTHING);
     return nsEventStatus_eIgnore;
 
   case PANNING:
@@ -329,10 +332,10 @@ nsEventStatus AsyncPanZoomController::OnTouchEnd(const MultiTouchInput& aEvent)
       ScheduleComposite();
       RequestContentRepaint();
     }
-    mState = FLING;
+    SetState(FLING);
     return nsEventStatus_eConsumeNoDefault;
   case PINCHING:
-    mState = NOTHING;
+    SetState(NOTHING);
     // Scale gesture listener should have handled this.
     NS_WARNING("Gesture listener should have handled pinching in OnTouchEnd.");
     return nsEventStatus_eIgnore;
@@ -342,12 +345,12 @@ nsEventStatus AsyncPanZoomController::OnTouchEnd(const MultiTouchInput& aEvent)
 }
 
 nsEventStatus AsyncPanZoomController::OnTouchCancel(const MultiTouchInput& aEvent) {
-  mState = NOTHING;
+  SetState(NOTHING);
   return nsEventStatus_eConsumeNoDefault;
 }
 
 nsEventStatus AsyncPanZoomController::OnScaleBegin(const PinchGestureInput& aEvent) {
-  mState = PINCHING;
+  SetState(PINCHING);
   mLastZoomFocus = aEvent.mFocusPoint;
 
   return nsEventStatus_eConsumeNoDefault;
@@ -449,7 +452,7 @@ nsEventStatus AsyncPanZoomController::OnScale(const PinchGestureInput& aEvent) {
 }
 
 nsEventStatus AsyncPanZoomController::OnScaleEnd(const PinchGestureInput& aEvent) {
-  mState = PANNING;
+  SetState(PANNING);
   mX.StartTouch(aEvent.mFocusPoint.x);
   mY.StartTouch(aEvent.mFocusPoint.y);
   {
@@ -517,7 +520,7 @@ void AsyncPanZoomController::StartPanning(const MultiTouchInput& aEvent) {
 
   mX.StartTouch(touch.mScreenPoint.x);
   mY.StartTouch(touch.mScreenPoint.y);
-  mState = PANNING;
+  SetState(PANNING);
 
   if (angle < AXIS_LOCK_ANGLE || angle > (M_PI - AXIS_LOCK_ANGLE)) {
     mY.LockPanning();
@@ -925,11 +928,11 @@ void AsyncPanZoomController::ZoomToRect(const gfxRect& aRect) {
   gfx::Rect zoomToRect(gfx::Rect(aRect.x, aRect.y, aRect.width, aRect.height));
   gfx::Rect cssPageRect = mFrameMetrics.mCSSContentRect;
 
+  SetState(ANIMATING_ZOOM);
+
   {
     MonitorAutoLock mon(mMonitor);
 
-    mState = ANIMATING_ZOOM;
-
     nsIntRect viewport = mFrameMetrics.mViewport;
 
     // If the rect is empty, treat it as a request to zoom out to the full page
@@ -995,5 +998,10 @@ void AsyncPanZoomController::ZoomToRect(const gfxRect& aRect) {
   }
 }
 
+void AsyncPanZoomController::SetState(PanZoomState aState) {
+  MonitorAutoLock monitor(mMonitor);
+  mState = aState;
+}
+
 }
 }
diff --git a/gfx/layers/ipc/AsyncPanZoomController.h b/gfx/layers/ipc/AsyncPanZoomController.h
index f8b4ea4953c..549b24629e9 100644
--- a/gfx/layers/ipc/AsyncPanZoomController.h
+++ b/gfx/layers/ipc/AsyncPanZoomController.h
@@ -296,6 +296,8 @@ protected:
    * Cancels any currently running animation. Note that all this does is set the
    * state of the AsyncPanZoomController back to NOTHING, but it is the
    * animation's responsibility to check this before advancing.
+   *
+   * *** The monitor must be held while calling this.
    */
   void CancelAnimation();
 
@@ -398,6 +400,13 @@ private:
    CONTENT_PAINTING_AND_PAINT_PENDING
   };
 
+  /**
+   * Helper to set the current state. Holds the monitor before actually setting
+   * it. If the monitor is already held by the current thread, it is safe to
+   * instead use: |mState = NEWSTATE;|
+   */
+  void SetState(PanZoomState aState);
+
   nsRefPtr<CompositorParent> mCompositorParent;
   nsRefPtr<GeckoContentController> mGeckoContentController;
   nsRefPtr<GestureEventListener> mGestureEventListener;
@@ -428,6 +437,10 @@ private:
   AxisX mX;
   AxisY mY;
 
+  // Protects |mFrameMetrics|, |mLastContentPaintMetrics| and |mState|. Before
+  // manipulating |mFrameMetrics| or |mLastContentPaintMetrics|, the monitor
+  // should be held. When setting |mState|, either the SetState() function can
+  // be used, or the monitor can be held and then |mState| updated.
   Monitor mMonitor;
 
   // The last time the compositor has sampled the content transform for this
@@ -443,7 +456,11 @@ private:
   // Stores the previous focus point if there is a pinch gesture happening. Used
   // to allow panning by moving multiple fingers (thus moving the focus point).
   nsIntPoint mLastZoomFocus;
+
+  // Stores the state of panning and zooming this frame. This is protected by
+  // |mMonitor|; that is, it should be held whenever this is updated.
   PanZoomState mState;
+
   int mDPI;
 
   // Stores the current paint status of the frame that we're managing. Repaints