Bug 561981. Guard against nsFrameLoader::Show flushing and re-entering or destroying us. r=smaug

content/base/src/nsFrameLoader.cpp

@@ -89,6 +89,7 @@
 #include "nsIContentViewer.h"
 #include "nsIDOMChromeWindow.h"
 #include "nsInProcessTabChildGlobal.h"
+#include "mozilla/AutoRestore.h"
 
 class nsAsyncDocShellDestroyer : public nsRunnable
 {
@@ -514,25 +515,50 @@ AllDescendantsOfType(nsIDocShellTreeItem* aParentItem, PRInt32 aType)
   return PR_TRUE;
 }
 
-bool
+/**
+ * A class that automatically sets mInShow to false when it goes
+ * out of scope.
+ */
+class NS_STACK_CLASS AutoResetInShow {
+  private:
+    nsFrameLoader* mFrameLoader;
+    MOZILLA_DECL_USE_GUARD_OBJECT_NOTIFIER
+  public:
+    AutoResetInShow(nsFrameLoader* aFrameLoader MOZILLA_GUARD_OBJECT_NOTIFIER_PARAM)
+      : mFrameLoader(aFrameLoader)
+    {
+      MOZILLA_GUARD_OBJECT_NOTIFIER_INIT;
+    }
+    ~AutoResetInShow() { mFrameLoader->mInShow = PR_FALSE; }
+};
+
+
+PRBool
 nsFrameLoader::Show(PRInt32 marginWidth, PRInt32 marginHeight,
                     PRInt32 scrollbarPrefX, PRInt32 scrollbarPrefY,
                     nsIFrameFrame* frame)
 {
+  if (mInShow) {
+    return PR_FALSE;
+  }
+  // Reset mInShow if we exit early.
+  AutoResetInShow resetInShow(this);
+  mInShow = PR_TRUE;
+
   nsContentType contentType;
 
   nsresult rv = EnsureDocShell();
   if (NS_FAILED(rv)) {
-    return false;
+    return PR_FALSE;
   }
 
   if (!mDocShell)
-    return false;
+    return PR_FALSE;
 
   nsCOMPtr<nsIPresShell> presShell;
   mDocShell->GetPresShell(getter_AddRefs(presShell));
   if (presShell)
-    return true;
+    return PR_TRUE;
 
   mDocShell->SetMarginWidth(marginWidth);
   mDocShell->SetMarginHeight(marginHeight);
@@ -563,7 +589,7 @@ nsFrameLoader::Show(PRInt32 marginWidth, PRInt32 marginHeight,
 
   nsIView* view = frame->CreateViewAndWidget(contentType);
   if (!view)
-    return false;
+    return PR_FALSE;
 
   nsCOMPtr<nsIBaseWindow> baseWindow = do_QueryInterface(mDocShell);
   NS_ASSERTION(baseWindow, "Found a nsIDocShell that isn't a nsIBaseWindow.");
@@ -594,12 +620,26 @@ nsFrameLoader::Show(PRInt32 marginWidth, PRInt32 marginHeight,
     }
   }
 
-  return true;
+  mInShow = PR_FALSE;
+  if (mHideCalled) {
+    mHideCalled = PR_FALSE;
+    Hide();
+    return PR_FALSE;
+  }
+  return PR_TRUE;
 }
 
 void
 nsFrameLoader::Hide()
 {
+  if (mHideCalled) {
+    return;
+  }
+  if (mInShow) {
+    mHideCalled = PR_TRUE;
+    return;
+  }
+
   if (!mDocShell)
     return;
 
@@ -623,6 +663,7 @@ nsFrameLoader::SwapWithOtherLoader(nsFrameLoader* aOther,
   NS_PRECONDITION((aFirstToSwap == this && aSecondToSwap == aOther) ||
                   (aFirstToSwap == aOther && aSecondToSwap == this),
                   "Swapping some sort of random loaders?");
+  NS_ENSURE_STATE(!mInShow && !aOther->mInShow);
 
   nsIContent* ourContent = mOwnerContent;
   nsIContent* otherContent = aOther->mOwnerContent;

content/base/src/nsFrameLoader.h

@@ -54,9 +54,12 @@ class nsIContent;
 class nsIURI;
 class nsIFrameFrame;
 class nsIInProcessContentFrameMessageManager;
+class AutoResetInShow;
 
 class nsFrameLoader : public nsIFrameLoader
 {
+  friend class AutoResetInShow;
+
 protected:
   nsFrameLoader(nsIContent *aOwner) :
     mOwnerContent(aOwner),
@@ -64,7 +67,9 @@ protected:
     mIsTopLevelContent(PR_FALSE),
     mDestroyCalled(PR_FALSE),
     mNeedsAsyncDestroy(PR_FALSE),
-    mInSwap(PR_FALSE)
+    mInSwap(PR_FALSE),
+    mInShow(PR_FALSE),
+    mHideCalled(PR_FALSE)
   {}
 
 public:
@@ -89,9 +94,9 @@ public:
    * Called from the layout frame associated with this frame loader;
    * this notifies us to hook up with the widget and view.
    */
-  bool Show(PRInt32 marginWidth, PRInt32 marginHeight,
-            PRInt32 scrollbarPrefX, PRInt32 scrollbarPrefY,
-            nsIFrameFrame* frame);
+  PRBool Show(PRInt32 marginWidth, PRInt32 marginHeight,
+              PRInt32 scrollbarPrefX, PRInt32 scrollbarPrefY,
+              nsIFrameFrame* frame);
 
   /**
    * Called from the layout frame associated with this frame loader, when
@@ -130,6 +135,8 @@ private:
   PRPackedBool mDestroyCalled : 1;
   PRPackedBool mNeedsAsyncDestroy : 1;
   PRPackedBool mInSwap : 1;
+  PRPackedBool mInShow : 1;
+  PRPackedBool mHideCalled : 1;
 };
 
 #endif

layout/generic/nsFrameFrame.cpp

@@ -221,14 +221,16 @@ protected:
   nsIView* mInnerView;
   PRPackedBool mIsInline;
   PRPackedBool mPostedReflowCallback;
-  bool mDidCreateDoc;
+  PRPackedBool mDidCreateDoc;
+  PRPackedBool mCallingShow;
 };
 
 nsSubDocumentFrame::nsSubDocumentFrame(nsStyleContext* aContext)
   : nsLeafFrame(aContext)
   , mIsInline(PR_FALSE)
   , mPostedReflowCallback(PR_FALSE)
-  , mDidCreateDoc(false)
+  , mDidCreateDoc(PR_FALSE)
+  , mCallingShow(PR_FALSE)
 {
 }
 
@@ -327,19 +329,31 @@ inline PRInt32 ConvertOverflow(PRUint8 aOverflow)
 void
 nsSubDocumentFrame::ShowViewer()
 {
+  if (mCallingShow) {
+    return;
+  }
+
   if (!PresContext()->IsDynamic()) {
     // We let the printing code take care of loading the document; just
     // create a widget for it to use
     (void) CreateViewAndWidget(eContentTypeContent);
   } else {
-    nsFrameLoader* frameloader = FrameLoader();
+    nsRefPtr<nsFrameLoader> frameloader = FrameLoader();
     if (frameloader) {
       nsIntSize margin = GetMarginAttributes();
       const nsStyleDisplay* disp = GetStyleDisplay();
-      mDidCreateDoc = frameloader->Show(margin.width, margin.height,
-                                        ConvertOverflow(disp->mOverflowX),
-                                        ConvertOverflow(disp->mOverflowY),
-                                        this);
+      nsWeakFrame weakThis(this);
+      mCallingShow = PR_TRUE;
+      PRBool didCreateDoc =
+        frameloader->Show(margin.width, margin.height,
+                          ConvertOverflow(disp->mOverflowX),
+                          ConvertOverflow(disp->mOverflowY),
+                          this);
+      if (!weakThis.IsAlive()) {
+        return;
+      }
+      mCallingShow = PR_FALSE;
+      mDidCreateDoc = didCreateDoc;
     }
   }
 }
@@ -825,7 +839,7 @@ nsSubDocumentFrame::DestroyFrom(nsIFrame* aDestructRoot)
 void
 nsSubDocumentFrame::HideViewer()
 {
-  if (mFrameLoader && mDidCreateDoc)
+  if (mFrameLoader && (mDidCreateDoc || mCallingShow))
     mFrameLoader->Hide();
 }
 
@@ -882,8 +896,8 @@ nsSubDocumentFrame::BeginSwapDocShells(nsIFrame* aOther)
   }
 
   nsSubDocumentFrame* other = static_cast<nsSubDocumentFrame*>(aOther);
-  if (!mFrameLoader || !mDidCreateDoc || !other->mFrameLoader ||
-      !other->mDidCreateDoc) {
+  if (!mFrameLoader || !mDidCreateDoc || mCallingShow ||
+      !other->mFrameLoader || !other->mDidCreateDoc) {
     return NS_ERROR_NOT_IMPLEMENTED;
   }
 
@@ -898,20 +912,25 @@ void
 nsSubDocumentFrame::EndSwapDocShells(nsIFrame* aOther)
 {
   nsSubDocumentFrame* other = static_cast<nsSubDocumentFrame*>(aOther);
+  nsWeakFrame weakThis(this);
+  nsWeakFrame weakOther(aOther);
   ShowViewer();
   other->ShowViewer();
 
   // Now make sure we reflow both frames, in case their contents
   // determine their size.
-  PresContext()->PresShell()->
-    FrameNeedsReflow(this, nsIPresShell::eTreeChange, NS_FRAME_IS_DIRTY);
-  other->PresContext()->PresShell()->
-    FrameNeedsReflow(other, nsIPresShell::eTreeChange, NS_FRAME_IS_DIRTY);
-
   // And repaint them, for good measure, in case there's nothing
   // interesting that happens during reflow.
-  InvalidateOverflowRect();
-  other->InvalidateOverflowRect();
+  if (weakThis.IsAlive()) {
+    PresContext()->PresShell()->
+      FrameNeedsReflow(this, nsIPresShell::eTreeChange, NS_FRAME_IS_DIRTY);
+    InvalidateOverflowRect();
+  }
+  if (weakOther.IsAlive()) {
+    other->PresContext()->PresShell()->
+      FrameNeedsReflow(other, nsIPresShell::eTreeChange, NS_FRAME_IS_DIRTY);
+    other->InvalidateOverflowRect();
+  }
 }
 
 nsIView*

layout/reftests/bugs/561981-1-ref.html

@@ -0,0 +1,7 @@
+<html>
+<head>
+</head>
+<body>
+<iframe id="i" src="data:text/html,text">
+</body>
+</html>

layout/reftests/bugs/561981-1.html

@@ -0,0 +1,16 @@
+<html class="reftest-wait">
+<head>
+<script>
+function runtest() {
+  var i = document.getElementById("i");
+  i.style.display = "none";
+  i.style.display = "";
+  document.documentElement.offsetLeft;
+  document.documentElement.className = "";
+}
+</script>
+</head>
+<body onload="runtest();">
+<iframe id="i" src="data:text/html,text">
+</body>
+</html>

layout/reftests/bugs/561981-2-ref.html

@@ -0,0 +1,7 @@
+<html>
+<head>
+</head>
+<body>
+<iframe id="i" style="display: none;" src="data:text/html,text">
+</body>
+</html>

layout/reftests/bugs/561981-2.html

@@ -0,0 +1,16 @@
+<html class="reftest-wait">
+<head>
+<script>
+function runtest() {
+  var i = document.getElementById("i");
+  i.style.display = "";
+  i.style.display = "none";
+  document.documentElement.offsetLeft;
+  document.documentElement.className = "";
+}
+</script>
+</head>
+<body onload="runtest();">
+<iframe id="i" style="display: none;" src="data:text/html,text">
+</body>
+</html>

layout/reftests/bugs/561981-3-ref.html

@@ -0,0 +1,7 @@
+<html>
+<head>
+</head>
+<body>
+<iframe id="i" src="data:text/html,text">
+</body>
+</html>

layout/reftests/bugs/561981-3.html

@@ -0,0 +1,16 @@
+<html class="reftest-wait">
+<head>
+<script>
+function runtest() {
+  var i = document.getElementById("i");
+  i.parentNode.style.display = "none";
+  i.parentNode.style.display = "";
+  document.documentElement.offsetLeft;
+  document.documentElement.className = "";
+}
+</script>
+</head>
+<body onload="runtest();">
+<iframe id="i" src="data:text/html,text">
+</body>
+</html>

layout/reftests/bugs/561981-4-ref.html

@@ -0,0 +1,7 @@
+<html>
+<head>
+</head>
+<body style="display: none;">
+<iframe id="i" src="data:text/html,text">
+</body>
+</html>

layout/reftests/bugs/561981-4.html

@@ -0,0 +1,16 @@
+<html class="reftest-wait">
+<head>
+<script>
+function runtest() {
+  var i = document.getElementById("i");
+  i.parentNode.style.display = "";
+  i.parentNode.style.display = "none";
+  document.documentElement.offsetLeft;
+  document.documentElement.className = "";
+}
+</script>
+</head>
+<body style="display: none;" onload="runtest();">
+<iframe id="i" src="data:text/html,text">
+</body>
+</html>

layout/reftests/bugs/561981-5-ref.html

@@ -0,0 +1,7 @@
+<html>
+<head>
+</head>
+<body>
+<iframe id="i" src="data:text/html,text">
+</body>
+</html>

layout/reftests/bugs/561981-5.html

@@ -0,0 +1,12 @@
+<html>
+<head>
+</head>
+<body>
+<iframe id="i" src="data:text/html,text">
+<script>
+var i = document.getElementById("i");
+i.style.display = "none";
+i.style.display = "";
+</script>
+</body>
+</html>

layout/reftests/bugs/561981-6-ref.html

@@ -0,0 +1,7 @@
+<html>
+<head>
+</head>
+<body>
+<iframe id="i" style="display: none;" src="data:text/html,text">
+</body>
+</html>

layout/reftests/bugs/561981-6.html

@@ -0,0 +1,12 @@
+<html>
+<head>
+</head>
+<body>
+<iframe id="i" style="display: none;" src="data:text/html,text">
+<script>
+var i = document.getElementById("i");
+i.style.display = "";
+i.style.display = "none";
+</script>
+</body>
+</html>

layout/reftests/bugs/561981-7-ref.html

@@ -0,0 +1,7 @@
+<html>
+<head>
+</head>
+<body>
+<iframe id="i" src="data:text/html,text">
+</body>
+</html>

layout/reftests/bugs/561981-7.html

@@ -0,0 +1,12 @@
+<html>
+<head>
+</head>
+<body>
+<iframe id="i" src="data:text/html,text">
+<script>
+var i = document.getElementById("i");
+i.parentNode.style.display = "none";
+i.parentNode.style.display = "";
+</script>
+</body>
+</html>

layout/reftests/bugs/561981-8-ref.html

@@ -0,0 +1,7 @@
+<html>
+<head>
+</head>
+<body style="display: none;">
+<iframe id="i" src="data:text/html,text">
+</body>
+</html>

layout/reftests/bugs/561981-8.html

@@ -0,0 +1,12 @@
+<html>
+<head>
+</head>
+<body style="display: none;">
+<iframe id="i" src="data:text/html,text">
+<script>
+  var i = document.getElementById("i");
+  i.parentNode.style.display = "";
+  i.parentNode.style.display = "none";
+</script>
+</body>
+</html>

layout/reftests/bugs/reftest.list

@@ -1429,6 +1429,14 @@ random-if(!haveTestPlugin) == 546071-1.html 546071-1-ref.html
 == 556661-1.html 556661-1-ref.html
 == 557736-1.html 557736-1-ref.html
 == 559284-1.html 559284-1-ref.html
+== 561981-1.html 561981-1-ref.html
+== 561981-2.html 561981-2-ref.html
+== 561981-3.html 561981-3-ref.html
+== 561981-4.html 561981-4-ref.html
+== 561981-5.html 561981-5-ref.html
+== 561981-6.html 561981-6-ref.html
+== 561981-7.html 561981-7-ref.html
+== 561981-8.html 561981-8-ref.html
 == 562835-1.html 562835-ref.html
 == 562835-2.html 562835-ref.html
 == 564054-1.html 564054-1-ref.html