Bug 413292. Make Begin/EndUpdateViewBatch be sure to remove the batch-count from the same viewmanager we added one to. r+sr=bzbarsky
authorroc+@cs.cmu.edu
Sat, 26 Jan 2008 15:59:50 -0800
changeset 10730 795bc4705be9f721d021fc4843027d605a4195e9
parent 10729 c223fb65ec142dbd26082aab29aaffae85b70e24
child 10731 22a57b1fac626db7ab7ff5721b5496eaa63e1a09
push id1
push userbsmedberg@mozilla.com
push dateThu, 20 Mar 2008 16:49:24 +0000
treeherdermozilla-central@61007906a1f8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs413292
milestone1.9b3pre
Bug 413292. Make Begin/EndUpdateViewBatch be sure to remove the batch-count from the same viewmanager we added one to. r+sr=bzbarsky
editor/libeditor/base/nsEditor.cpp
editor/libeditor/base/nsEditor.h
editor/libeditor/text/Makefile.in
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsDocumentViewer.cpp
layout/base/nsPresShell.cpp
view/public/nsIViewManager.h
view/src/nsViewManager.cpp
view/src/nsViewManager.h
--- a/editor/libeditor/base/nsEditor.cpp
+++ b/editor/libeditor/base/nsEditor.cpp
@@ -4348,19 +4348,17 @@ nsresult nsEditor::BeginUpdateViewBatch(
 
     if (selection) 
     {
       nsCOMPtr<nsISelectionPrivate> selPrivate(do_QueryInterface(selection));
       selPrivate->StartBatchChanges();
     }
 
     // Turn off view updating.
-
-    if (mViewManager)
-      mViewManager->BeginUpdateViewBatch();
+    mBatch.BeginUpdateViewBatch(mViewManager);
   }
 
   mUpdateCount++;
 
   return NS_OK;
 }
 
 
@@ -4406,17 +4404,17 @@ nsresult nsEditor::EndUpdateViewBatch()
       if (flags & nsIPlaintextEditor::eEditorUseAsyncUpdatesMask) {
         updateFlag = NS_VMREFRESH_DEFERRED;
       } else {
         // Flush out layout.  Need to do this because if we have no invalidates
         // to flush the viewmanager code won't flush our reflow here, and we
         // have selection code that does sync caret scrolling in this case.
         presShell->FlushPendingNotifications(Flush_Layout);
       }
-      mViewManager->EndUpdateViewBatch(updateFlag);
+      mBatch.EndUpdateViewBatch(updateFlag);
     }
 
     // Turn selection updating and notifications back on.
 
     nsCOMPtr<nsISelection>selection;
     GetSelection(getter_AddRefs(selection));
 
     if (selection) {
--- a/editor/libeditor/base/nsEditor.h
+++ b/editor/libeditor/base/nsEditor.h
@@ -60,21 +60,21 @@
 #include "nsIDocumentStateListener.h"
 #include "nsICSSStyleSheet.h"
 #include "nsIDOMElement.h"
 #include "nsSelectionState.h"
 #include "nsIEditorSpellCheck.h"
 #include "nsIInlineSpellChecker.h"
 #include "nsPIDOMEventTarget.h"
 #include "nsStubMutationObserver.h"
+#include "nsIViewManager.h"
 
 class nsIDOMCharacterData;
 class nsIDOMRange;
 class nsIPresShell;
-class nsIViewManager;
 class ChangeAttributeTxn;
 class CreateElementTxn;
 class InsertElementTxn;
 class DeleteElementTxn;
 class InsertTextTxn;
 class DeleteTextTxn;
 class SplitElementTxn;
 class JoinElementTxn;
@@ -592,16 +592,17 @@ protected:
 
   PRUint32        mModCount;		// number of modifications (for undo/redo stack)
   PRUint32        mFlags;		// behavior flags. See nsIPlaintextEditor.idl for the flags we use.
   
   nsWeakPtr       mPresShellWeak;   // weak reference to the nsIPresShell
   nsWeakPtr       mSelConWeak;   // weak reference to the nsISelectionController
   nsIViewManager *mViewManager;
   PRInt32         mUpdateCount;
+  nsIViewManager::UpdateViewBatch mBatch;
 
   // Spellchecking
   enum Tristate {
     eTriUnset,
     eTriFalse,
     eTriTrue
   }                 mSpellcheckCheckboxState;
   nsCOMPtr<nsIInlineSpellChecker> mInlineSpellChecker;
--- a/editor/libeditor/text/Makefile.in
+++ b/editor/libeditor/text/Makefile.in
@@ -53,16 +53,17 @@ REQUIRES	= xpcom \
 		  layout \
 		  content \
 		  txmgr \
 		  txtsvc \
 		  htmlparser \
 		  necko \
 		  pref \
 		  lwbrk \
+		  view \
 		  gfx \
 		  thebes \
 		  widget \
 		  unicharutil \
 		  $(NULL)
 
 CPPSRCS		=                           \
 		nsPlaintextDataTransfer.cpp \
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -9774,28 +9774,28 @@ ApplyRenderingChangeToTree(nsPresContext
 
   nsIViewManager* viewManager = aPresContext->GetViewManager();
 
   // Trigger rendering updates by damaging this frame and any
   // continuations of this frame.
 
   // XXX this needs to detect the need for a view due to an opacity change and deal with it...
 
-  viewManager->BeginUpdateViewBatch();
+  nsIViewManager::UpdateViewBatch batch(viewManager);
 
 #ifdef DEBUG
   gInApplyRenderingChangeToTree = PR_TRUE;
 #endif
   DoApplyRenderingChangeToTree(aFrame, viewManager, shell->FrameManager(),
                                aChange);
 #ifdef DEBUG
   gInApplyRenderingChangeToTree = PR_FALSE;
 #endif
   
-  viewManager->EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);
+  batch.EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);
 }
 
 /**
  * This method invalidates the canvas when frames are removed or added for a
  * node that might have its background propagated to the canvas, i.e., a
  * document root node or an HTML BODY which is a child of the root node.
  *
  * @param aFrame a frame for a content node about to be removed or a frme that
@@ -9845,21 +9845,20 @@ InvalidateCanvasIfNeeded(nsIFrame* aFram
     // viewport instead.
     ancestor = ancestor->GetParent();
   }
 
   if (ancestor != aFrame) {
     // Wrap this in a DEFERRED view update batch so we don't try to
     // flush out layout here
 
-    nsIViewManager* viewManager = presContext->GetViewManager();
-    viewManager->BeginUpdateViewBatch();  
+    nsIViewManager::UpdateViewBatch batch(presContext->GetViewManager());  
     ApplyRenderingChangeToTree(presContext, ancestor,
                                nsChangeHint_RepaintFrame);
-    viewManager->EndUpdateViewBatch(NS_VMREFRESH_DEFERRED);
+    batch.EndUpdateViewBatch(NS_VMREFRESH_DEFERRED);
   }
 }
 
 nsresult
 nsCSSFrameConstructor::StyleChangeReflow(nsIFrame* aFrame)
 {
   // If the frame hasn't even received an initial reflow, then don't
   // send it a style-change reflow!
--- a/layout/base/nsDocumentViewer.cpp
+++ b/layout/base/nsDocumentViewer.cpp
@@ -2659,37 +2659,33 @@ SetChildFullZoom(nsIMarkupDocumentViewer
 }
 
 NS_IMETHODIMP
 DocumentViewerImpl::SetTextZoom(float aTextZoom)
 {
   if (!GetIsPrintPreview()) {
     mTextZoom = aTextZoom;
   }
-  nsCOMPtr<nsIViewManager> vm = GetViewManager();
-  if (vm) {
-    vm->BeginUpdateViewBatch();
-  }
+
+  nsIViewManager::UpdateViewBatch batch(GetViewManager());
       
   // Set the text zoom on all children of mContainer (even if our zoom didn't
   // change, our children's zoom may be different, though it would be unusual).
   // Do this first, in case kids are auto-sizing and post reflow commands on
   // our presshell (which should be subsumed into our own style change reflow).
   struct ZoomInfo ZoomInfo = { aTextZoom };
   CallChildren(SetChildTextZoom, &ZoomInfo);
 
   // Now change our own zoom
   nsPresContext* pc = GetPresContext();
   if (pc && aTextZoom != mPresContext->TextZoom()) {
       pc->SetTextZoom(aTextZoom);
   }
 
-  if (vm) {
-    vm->EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);
-  }
+  batch.EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);
   
   return NS_OK;
 }
 
 NS_IMETHODIMP
 DocumentViewerImpl::GetTextZoom(float* aTextZoom)
 {
   NS_ENSURE_ARG_POINTER(aTextZoom);
@@ -2700,32 +2696,27 @@ DocumentViewerImpl::GetTextZoom(float* a
 
 NS_IMETHODIMP
 DocumentViewerImpl::SetFullZoom(float aFullZoom)
 {
   if (!GetIsPrintPreview()) {
     mPageZoom = aFullZoom;
   }
 
-  nsCOMPtr<nsIViewManager> vm = GetViewManager();
-  if (vm) {
-    vm->BeginUpdateViewBatch();
-  }
+  nsIViewManager::UpdateViewBatch batch(GetViewManager());
 
   struct ZoomInfo ZoomInfo = { aFullZoom };
   CallChildren(SetChildFullZoom, &ZoomInfo);
 
   nsPresContext* pc = GetPresContext();
   if (pc) {
     pc->SetFullZoom(aFullZoom);
   }
 
-  if (vm) {
-    vm->EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);
-  }
+  batch.EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 DocumentViewerImpl::GetFullZoom(float* aFullZoom)
 {
   NS_ENSURE_ARG_POINTER(aFullZoom);
--- a/layout/base/nsPresShell.cpp
+++ b/layout/base/nsPresShell.cpp
@@ -2497,18 +2497,18 @@ PresShell::ResizeReflow(nscoord aWidth, 
 
   mPresContext->SetVisibleArea(nsRect(0, 0, aWidth, aHeight));
 
   // There isn't anything useful we can do if the initial reflow hasn't happened
   if (!rootFrame)
     return NS_OK;
 
   NS_ASSERTION(mViewManager, "Must have view manager");
-  nsCOMPtr<nsIViewManager> viewManager = mViewManager;
-  viewManager->BeginUpdateViewBatch();
+  nsCOMPtr<nsIViewManager> viewManagerDeathGrip = mViewManager;
+  nsIViewManager::UpdateViewBatch batch(mViewManager);
 
   // Take this ref after viewManager so it'll make sure to go away first
   nsCOMPtr<nsIPresShell> kungFuDeathGrip(this);
 
   // Make sure style is up to date
   mFrameConstructor->ProcessPendingRestyles();
   if (!mIsDestroying) {
     // XXX Do a full invalidate at the beginning so that invalidates along
@@ -2526,17 +2526,17 @@ PresShell::ResizeReflow(nscoord aWidth, 
       DoReflow(rootFrame);
       mIsReflowing = PR_FALSE;
     }
 
     DidCauseReflow();
     DidDoReflow();
   }
 
-  viewManager->EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);
+  batch.EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);
 
   if (!mIsDestroying) {
     CreateResizeEventTimer();
   }
 
   return NS_OK; //XXX this needs to be real. MMP
 }
 
@@ -3335,31 +3335,31 @@ PresShell::RecreateFramesFor(nsIContent*
     // root we will crash.
     return NS_OK;
   }
 
   // Don't call RecreateFramesForContent since that is not exported and we want
   // to keep the number of entrypoints down.
 
   NS_ASSERTION(mViewManager, "Should have view manager");
-  mViewManager->BeginUpdateViewBatch();
+  nsIViewManager::UpdateViewBatch batch(mViewManager);
 
   // Have to make sure that the content notifications are flushed before we
   // start messing with the frame model; otherwise we can get content doubling.
   mDocument->FlushPendingNotifications(Flush_ContentAndNotify);
 
   nsStyleChangeList changeList;
   changeList.AppendChange(nsnull, aContent, nsChangeHint_ReconstructFrame);
 
   // Mark ourselves as not safe to flush while we're doing frame construction.
   ++mChangeNestCount;
   nsresult rv = mFrameConstructor->ProcessRestyledFrames(changeList);
   --mChangeNestCount;
   
-  mViewManager->EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);
+  batch.EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);
 #ifdef ACCESSIBILITY
   InvalidateAccessibleSubtree(aContent);
 #endif
   return rv;
 }
 
 NS_IMETHODIMP
 PresShell::ClearFrameRefs(nsIFrame* aFrame)
@@ -4451,28 +4451,26 @@ PresShell::DoFlushPendingNotifications(m
 {
   NS_ASSERTION(aType >= Flush_Frames, "Why did we get called?");
   
   PRBool isSafeToFlush;
   IsSafeToFlush(isSafeToFlush);
 
   NS_ASSERTION(!isSafeToFlush || mViewManager, "Must have view manager");
   // Make sure the view manager stays alive while batching view updates.
-  // XXX FIXME: If viewmanager hierarchy is modified while we're in update
-  //            batch... We need to address that somehow.  See bug 369165.
-  nsCOMPtr<nsIViewManager> viewManager = mViewManager;
-  if (isSafeToFlush && viewManager) {
+  nsCOMPtr<nsIViewManager> viewManagerDeathGrip = mViewManager;
+  if (isSafeToFlush && mViewManager) {
     // Processing pending notifications can kill us, and some callers only
     // hold weak refs when calling FlushPendingNotifications().  :(
     nsCOMPtr<nsIPresShell> kungFuDeathGrip(this);
 
     // Style reresolves not in conjunction with reflows can't cause
     // painting or geometry changes, so don't bother with view update
     // batching if we only have style reresolve
-    viewManager->BeginUpdateViewBatch();
+    nsIViewManager::UpdateViewBatch batch(mViewManager);
 
     // Force flushing of any pending content notifications that might have
     // queued up while our event was pending.  That will ensure that we don't
     // construct frames for content right now that's still waiting to be
     // notified on,
     mDocument->FlushPendingNotifications(Flush_ContentAndNotify);
 
     // Process pending restyles, since any flush of the presshell wants
@@ -4514,17 +4512,17 @@ PresShell::DoFlushPendingNotifications(m
       updateFlags = NS_VMREFRESH_IMMEDIATE;
     }
     else if (aType < Flush_Layout) {
       // Not flushing reflows, so do deferred invalidates.  This will keep us
       // from possibly flushing out reflows due to invalidates being processed
       // at the end of this view batch.
       updateFlags = NS_VMREFRESH_DEFERRED;
     }
-    viewManager->EndUpdateViewBatch(updateFlags);
+    batch.EndUpdateViewBatch(updateFlags);
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 PresShell::IsReflowLocked(PRBool* aIsReflowLocked) 
 {
@@ -6415,33 +6413,33 @@ PresShell::Observe(nsISupports* aSubject
 {
 #ifdef MOZ_XUL
   if (!nsCRT::strcmp(aTopic, "chrome-flush-skin-caches")) {
     nsIFrame *rootFrame = FrameManager()->GetRootFrame();
     // Need to null-check because "chrome-flush-skin-caches" can happen
     // at interesting times during startup.
     if (rootFrame) {
       NS_ASSERTION(mViewManager, "View manager must exist");
-      mViewManager->BeginUpdateViewBatch();
+      nsIViewManager::UpdateViewBatch batch(mViewManager);
 
       WalkFramesThroughPlaceholders(mPresContext, rootFrame,
                                     &ReResolveMenusAndTrees, nsnull);
 
       // Because "chrome:" URL equality is messy, reframe image box
       // frames (hack!).
       nsStyleChangeList changeList;
       WalkFramesThroughPlaceholders(mPresContext, rootFrame,
                                     ReframeImageBoxes, &changeList);
       // Mark ourselves as not safe to flush while we're doing frame
       // construction.
       ++mChangeNestCount;
       mFrameConstructor->ProcessRestyledFrames(changeList);
       --mChangeNestCount;
 
-      mViewManager->EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);
+      batch.EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);
 #ifdef ACCESSIBILITY
       InvalidateAccessibleSubtree(nsnull);
 #endif
     }
     return NS_OK;
   }
 #endif
 
--- a/view/public/nsIViewManager.h
+++ b/view/public/nsIViewManager.h
@@ -317,23 +317,44 @@ public:
   /**
    * allow the view manager to refresh. this may cause a synchronous
    * paint to occur inside the call.
    * @param aUpdateFlags see bottom of nsIViewManager.h for description
    * @return error status
    */
   NS_IMETHOD EnableRefresh(PRUint32 aUpdateFlags) = 0;
 
+  class UpdateViewBatch {
+  public:
+    UpdateViewBatch() {}
   /**
    * prevents the view manager from refreshing. allows UpdateView()
    * to notify widgets of damaged regions that should be repainted
-   * when the batch is ended.
+   * when the batch is ended. Call EndUpdateViewBatch on this object
+   * before it is destroyed
    * @return error status
    */
-  NS_IMETHOD BeginUpdateViewBatch(void) = 0;
+    UpdateViewBatch(nsIViewManager* aVM) {
+      if (aVM) {
+        mRootVM = aVM->BeginUpdateViewBatch();
+      }
+    }
+    ~UpdateViewBatch() {
+      NS_ASSERTION(!mRootVM, "Someone forgot to call EndUpdateViewBatch!");
+    }
+    
+    /**
+     * See the constructor, this lets you "fill in" a blank UpdateViewBatch.
+     */
+    void BeginUpdateViewBatch(nsIViewManager* aVM) {
+      NS_ASSERTION(!mRootVM, "already started a batch!");
+      if (aVM) {
+        mRootVM = aVM->BeginUpdateViewBatch();
+      }
+    }
 
   /**
    * allow the view manager to refresh any damaged areas accumulated
    * after the BeginUpdateViewBatch() call.  this may cause a
    * synchronous paint to occur inside the call if aUpdateFlags
    * NS_VMREFRESH_IMMEDIATE is set.
    *
    * If this is not the outermost view batch command, then this does
@@ -347,18 +368,38 @@ public:
    * Invalidate PLEvent)
    * -- Otherwise all batches specified NS_VMREFRESH_NO_SYNC and we honor
    * that; all widgets are invalidated normally and will be painted the next
    * time the toolkit chooses to update them.
    *
    * @param aUpdateFlags see bottom of nsIViewManager.h for
    * description @return error status
    */
+    void EndUpdateViewBatch(PRUint32 aUpdateFlags) {
+      if (!mRootVM)
+        return;
+      mRootVM->EndUpdateViewBatch(aUpdateFlags);
+      mRootVM = nsnull;
+    }
+
+  private:
+    UpdateViewBatch(const UpdateViewBatch& aOther) : mRootVM(aOther.mRootVM);
+    const UpdateViewBatch& operator=(const UpdateViewBatch& aOther);
+
+    nsCOMPtr<nsIViewManager> mRootVM;
+  };
+  
+private:
+  friend class UpdateViewBatch;
+
+  virtual nsIViewManager* BeginUpdateViewBatch(void) = 0;
   NS_IMETHOD EndUpdateViewBatch(PRUint32 aUpdateFlags) = 0;
 
+public:
+
   /**
    * set the view that is is considered to be the root scrollable
    * view for the document.
    * @param aScrollable root scrollable view
    * @return error status
    */
   NS_IMETHOD SetRootScrollableView(nsIScrollableView *aScrollable) = 0;
 
--- a/view/src/nsViewManager.cpp
+++ b/view/src/nsViewManager.cpp
@@ -1040,19 +1040,19 @@ NS_IMETHODIMP nsViewManager::DispatchEve
                 // the invalid event fires, which is later than the reflow
                 // event would fire and could end up being after some timer
                 // events, leading to frame dropping in DHTML).  Note that the
                 // observer may try to reenter this code from inside
                 // WillPaint() by trying to do a synchronous paint, but since
                 // refresh will be disabled it won't be able to do the paint.
                 // We should really sort out the rules on our synch painting
                 // api....
-                BeginUpdateViewBatch();
+                UpdateViewBatch batch(this);
                 observer->WillPaint();
-                EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);
+                batch.EndUpdateViewBatch(NS_VMREFRESH_NO_SYNC);
 
                 // Get the view pointer again since the code above might have
                 // destroyed it (bug 378273).
                 view = nsView::GetViewFor(aEvent->widget);
               }
             }
             // Make sure to sync up any widget geometry changes we
             // have pending before we paint.
@@ -1838,40 +1838,38 @@ NS_IMETHODIMP nsViewManager::EnableRefre
     PostInvalidateEvent();
   } else { // NO_SYNC
     FlushPendingInvalidates();
   }
 
   return NS_OK;
 }
 
-NS_IMETHODIMP nsViewManager::BeginUpdateViewBatch(void)
+nsIViewManager* nsViewManager::BeginUpdateViewBatch(void)
 {
   if (!IsRootVM()) {
     return RootViewManager()->BeginUpdateViewBatch();
   }
   
   nsresult result = NS_OK;
   
   if (mUpdateBatchCnt == 0) {
     mUpdateBatchFlags = 0;
     result = DisableRefresh();
   }
 
   if (NS_SUCCEEDED(result))
     ++mUpdateBatchCnt;
 
-  return result;
+  return this;
 }
 
 NS_IMETHODIMP nsViewManager::EndUpdateViewBatch(PRUint32 aUpdateFlags)
 {
-  if (!IsRootVM()) {
-    return RootViewManager()->EndUpdateViewBatch(aUpdateFlags);
-  }
+  NS_ASSERTION(IsRootVM(), "Should only be called on root");
   
   nsresult result = NS_OK;
 
   --mUpdateBatchCnt;
 
   NS_ASSERTION(mUpdateBatchCnt >= 0, "Invalid batch count!");
 
   if (mUpdateBatchCnt < 0)
--- a/view/src/nsViewManager.h
+++ b/view/src/nsViewManager.h
@@ -159,17 +159,17 @@ public:
   NS_IMETHOD  SetViewObserver(nsIViewObserver *aObserver);
   NS_IMETHOD  GetViewObserver(nsIViewObserver *&aObserver);
 
   NS_IMETHOD  GetDeviceContext(nsIDeviceContext *&aContext);
 
   NS_IMETHOD  DisableRefresh(void);
   NS_IMETHOD  EnableRefresh(PRUint32 aUpdateFlags);
 
-  NS_IMETHOD  BeginUpdateViewBatch(void);
+  virtual nsIViewManager* BeginUpdateViewBatch(void);
   NS_IMETHOD  EndUpdateViewBatch(PRUint32 aUpdateFlags);
 
   NS_IMETHOD  SetRootScrollableView(nsIScrollableView *aScrollable);
   NS_IMETHOD  GetRootScrollableView(nsIScrollableView **aScrollable);
 
   NS_IMETHOD GetWidget(nsIWidget **aWidget);
   nsIWidget* GetWidget() { return mRootView ? mRootView->GetWidget() : nsnull; }
   NS_IMETHOD ForceUpdate();