Bug 1339891 part 1. Make the invariants around nsIPresShell::FlushPendingNotifications clearer. r=mats
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 17 Feb 2017 13:38:44 -0500
changeset 372799 32242abfde5d17bcc1c7c82df0d395c1cc64f955
parent 372798 dc9764f81e1a14a187331a873300f181f92d7e88
child 372800 44d588184435c92321e7a0d207d57dd2012c1e81
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats
bugs1339891
milestone54.0a1
Bug 1339891 part 1. Make the invariants around nsIPresShell::FlushPendingNotifications clearer. r=mats
layout/base/PresShell.cpp
layout/base/nsIPresShell.h
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -1957,17 +1957,16 @@ PresShell::ResizeReflowIgnoreOverride(ns
   NS_PRECONDITION((wm.IsVertical() ? aHeight : aWidth) != NS_UNCONSTRAINEDSIZE,
                   "shouldn't use unconstrained isize anymore");
 
   const bool isBSizeChanging = wm.IsVertical()
                                ? aOldWidth != aWidth
                                : aOldHeight != aHeight;
 
   RefPtr<nsViewManager> viewManager = mViewManager;
-  // Take this ref after viewManager so it'll make sure to go away first.
   nsCOMPtr<nsIPresShell> kungFuDeathGrip(this);
 
   if (!GetPresContext()->SuppressingResizeReflow()) {
     // 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(FlushType::ContentAndNotify);
 
     // Make sure style is up to date
@@ -4071,16 +4070,19 @@ PresShell::FlushPendingNotifications(Flu
   // by default, flush animations if aType >= FlushType::Style
   mozilla::ChangesToFlush flush(aType, aType >= FlushType::Style);
   FlushPendingNotifications(flush);
 }
 
 void
 PresShell::FlushPendingNotifications(mozilla::ChangesToFlush aFlush)
 {
+  // Per our API contract, hold a strong ref to ourselves until we return.
+  nsCOMPtr<nsIPresShell> kungFuDeathGrip = this;
+
   /**
    * VERY IMPORTANT: If you add some sort of new flushing to this
    * method, make sure to add the relevant SetNeedLayoutFlush or
    * SetNeedStyleFlush calls on the shell.
    */
   FlushType flushType = aFlush.mFlushType;
 
 #ifdef MOZ_GECKO_PROFILER
@@ -4129,30 +4131,25 @@ PresShell::FlushPendingNotifications(moz
     isSafeToFlush = isSafeToFlush && nsContentUtils::IsSafeToRunScript();
   }
 
   NS_ASSERTION(!isSafeToFlush || mViewManager, "Must have view manager");
   // Make sure the view manager stays alive.
   RefPtr<nsViewManager> viewManager = mViewManager;
   bool didStyleFlush = false;
   bool didLayoutFlush = false;
-  nsCOMPtr<nsIPresShell> kungFuDeathGrip;
   if (isSafeToFlush && viewManager) {
     // Record that we are in a flush, so that our optimization in
     // nsDocument::FlushPendingNotifications doesn't skip any re-entrant
     // calls to us.  Otherwise, we might miss some needed flushes, since
     // we clear mNeedStyleFlush / mNeedLayoutFlush here at the top of
     // the function but we might not have done the work yet.
     AutoRestore<bool> guard(mInFlush);
     mInFlush = true;
 
-    // Processing pending notifications can kill us, and some callers only
-    // hold weak refs when calling FlushPendingNotifications().  :(
-    kungFuDeathGrip = this;
-
     if (mResizeEvent.IsPending()) {
       FireResizeEvent();
       if (mIsDestroying) {
         return;
       }
     }
 
     // We need to make sure external resource documents are flushed too (for
--- a/layout/base/nsIPresShell.h
+++ b/layout/base/nsIPresShell.h
@@ -575,16 +575,22 @@ public:
 
   /**
    * Flush pending notifications of the type specified.  This method
    * will not affect the content model; it'll just affect style and
    * frames. Callers that actually want up-to-date presentation (other
    * than the document itself) should probably be calling
    * nsIDocument::FlushPendingNotifications.
    *
+   * This method can execute script, which can destroy this presshell object
+   * unless someone is holding a reference to it on the stack.  The presshell
+   * itself will ensure it lives up until the method returns, but callers who
+   * plan to use the presshell after this call should hold a strong ref
+   * themselves!
+   *
    * @param aType the type of notifications to flush
    */
   virtual void FlushPendingNotifications(mozilla::FlushType aType) = 0;
   virtual void FlushPendingNotifications(mozilla::ChangesToFlush aType) = 0;
 
   /**
    * Whether we might need a flush for the given flush type.  If this
    * function returns false, we definitely don't need to flush.