Bug 1433056: Account for pres shell destruction. r?smaug draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 25 Jan 2018 15:56:47 +0100
changeset 737989 fed48bc65af8cfb60ccf3d855cde590bd61fa42e
parent 737988 69c8f588d8f115f82e525c278318dd534d6f3555
child 738043 f7119005531e17d84f52f34de7d1eb9158e5cbd1
push id96817
push userbmo:emilio@crisal.io
push dateThu, 25 Jan 2018 15:10:30 +0000
reviewerssmaug
bugs1433056
milestone60.0a1
Bug 1433056: Account for pres shell destruction. r?smaug The underlying issue here is that the pres shell was already being torn down when we flush. The less risky change here is to just change the assertions instead of IsSafeToFlush too, but the !isSafeToFlush || mViewManager assertion was also firing already, because we null out the view manager from Destroy. So this change is effectively idempotent behavior-wise (we tighten a bit the view manager assertion, but that should be fine). Just let me know if you want me to just loosen the assertions, but I think the IsSafeToFlush change is nicer too. MozReview-Commit-ID: 240qLlLi7RE
layout/base/PresShell.cpp
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -4057,18 +4057,19 @@ PresShell::HandlePostedReflowCallbacks(b
    if (shouldFlush && !mIsDestroying) {
      FlushPendingNotifications(flushType);
    }
 }
 
 bool
 nsIPresShell::IsSafeToFlush() const
 {
-  // Not safe if we are reflowing or in the middle of frame construction
-  if (mIsReflowing || mChangeNestCount) {
+  // Not safe if we are getting torn down, reflowing, or in the middle of frame
+  // construction.
+  if (mIsReflowing || mChangeNestCount || mIsDestroying) {
     return false;
   }
 
     // Not safe if we are painting
   if (nsViewManager* viewManager = GetViewManager()) {
     bool isPainting = false;
     viewManager->IsPainting(isPainting);
     if (isPainting) {
@@ -4097,18 +4098,16 @@ PresShell::DoFlushPendingNotifications(m
   /**
    * 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;
 
   MOZ_ASSERT(NeedFlush(flushType), "Why did we get called?");
-  MOZ_DIAGNOSTIC_ASSERT(mDocument->HasShellOrBFCacheEntry());
-  MOZ_DIAGNOSTIC_ASSERT(mDocument->GetShell() == this);
 
 #ifdef MOZ_GECKO_PROFILER
   static const EnumeratedArray<FlushType,
                                FlushType::Count,
                                const char*> flushTypeNames = {
     "",
     "Event",
     "Content",
@@ -4148,22 +4147,26 @@ PresShell::DoFlushPendingNotifications(m
   // If layout could possibly trigger scripts, then it's only safe to flush if
   // it's safe to run script.
   bool hasHadScriptObject;
   if (mDocument->GetScriptHandlingObject(hasHadScriptObject) ||
       hasHadScriptObject) {
     isSafeToFlush = isSafeToFlush && nsContentUtils::IsSafeToRunScript();
   }
 
-  NS_ASSERTION(!isSafeToFlush || mViewManager, "Must have view manager");
+  MOZ_DIAGNOSTIC_ASSERT(!mIsDestroying || !isSafeToFlush);
+  MOZ_DIAGNOSTIC_ASSERT(mIsDestroying || mViewManager);
+  MOZ_DIAGNOSTIC_ASSERT(mIsDestroying || mDocument->HasShellOrBFCacheEntry());
+  MOZ_DIAGNOSTIC_ASSERT(mIsDestroying || mDocument->GetShell() == this);
+
   // Make sure the view manager stays alive.
   RefPtr<nsViewManager> viewManager = mViewManager;
   bool didStyleFlush = false;
   bool didLayoutFlush = false;
-  if (isSafeToFlush && viewManager) {
+  if (isSafeToFlush) {
     // 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;