Bug 561981. Guard against nsFrameLoader::Show flushing and re-entering or destroying us. r=smaug
authorTimothy Nikkel <tnikkel@gmail.com>
Fri, 28 May 2010 14:34:50 -0500
changeset 42907 9fc485016d40233cf94f277d9ba8066938e2e862
parent 42906 e193ca042f864739341b80b42fde624a89a0ac42
child 42908 c6009202e1e1f0eeaf9f91a82ed8331d8760d109
push idunknown
push userunknown
push dateunknown
reviewerssmaug
bugs561981
milestone1.9.3a5pre
Bug 561981. Guard against nsFrameLoader::Show flushing and re-entering or destroying us. r=smaug
content/base/src/nsFrameLoader.cpp
content/base/src/nsFrameLoader.h
layout/generic/nsFrameFrame.cpp
layout/reftests/bugs/561981-1-ref.html
layout/reftests/bugs/561981-1.html
layout/reftests/bugs/561981-2-ref.html
layout/reftests/bugs/561981-2.html
layout/reftests/bugs/561981-3-ref.html
layout/reftests/bugs/561981-3.html
layout/reftests/bugs/561981-4-ref.html
layout/reftests/bugs/561981-4.html
layout/reftests/bugs/561981-5-ref.html
layout/reftests/bugs/561981-5.html
layout/reftests/bugs/561981-6-ref.html
layout/reftests/bugs/561981-6.html
layout/reftests/bugs/561981-7-ref.html
layout/reftests/bugs/561981-7.html
layout/reftests/bugs/561981-8-ref.html
layout/reftests/bugs/561981-8.html
layout/reftests/bugs/reftest.list
--- a/content/base/src/nsFrameLoader.cpp
+++ b/content/base/src/nsFrameLoader.cpp
@@ -84,16 +84,17 @@
 
 #include "nsGkAtoms.h"
 #include "nsINameSpaceManager.h"
 
 #include "nsThreadUtils.h"
 #include "nsIContentViewer.h"
 #include "nsIDOMChromeWindow.h"
 #include "nsInProcessTabChildGlobal.h"
+#include "mozilla/AutoRestore.h"
 
 class nsAsyncDocShellDestroyer : public nsRunnable
 {
 public:
   nsAsyncDocShellDestroyer(nsIDocShell* aDocShell)
     : mDocShell(aDocShell)
   {
   }
@@ -509,35 +510,60 @@ AllDescendantsOfType(nsIDocShellTreeItem
     if (kidType != aType || !AllDescendantsOfType(kid, aType)) {
       return PR_FALSE;
     }
   }
 
   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);
 
   nsCOMPtr<nsIScrollable> sc = do_QueryInterface(mDocShell);
   if (sc) {
     sc->SetDefaultScrollbarPreferences(nsIScrollable::ScrollOrientation_X,
                                        scrollbarPrefX);
@@ -558,17 +584,17 @@ nsFrameLoader::Show(PRInt32 marginWidth,
   else {
     nsCOMPtr<nsIDocShellTreeItem> sameTypeParent;
     treeItem->GetSameTypeParent(getter_AddRefs(sameTypeParent));
     contentType = sameTypeParent ? eContentTypeContentFrame : eContentTypeContent;
   }
 
   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.");
   baseWindow->InitWindow(nsnull, view->GetWidget(), 0, 0, 10, 10);
   // This is kinda whacky, this "Create()" call doesn't really
   // create anything, one starts to wonder why this was named
   // "Create"...
   baseWindow->Create();
@@ -589,22 +615,36 @@ nsFrameLoader::Show(PRInt32 marginWidth,
 
       if (designMode.EqualsLiteral("on")) {
         doc->SetDesignMode(NS_LITERAL_STRING("off"));
         doc->SetDesignMode(NS_LITERAL_STRING("on"));
       }
     }
   }
 
-  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;
 
   nsCOMPtr<nsIContentViewer> contentViewer;
   mDocShell->GetContentViewer(getter_AddRefs(contentViewer));
   if (contentViewer)
     contentViewer->SetSticky(PR_FALSE);
 
@@ -618,16 +658,17 @@ nsFrameLoader::Hide()
 nsresult
 nsFrameLoader::SwapWithOtherLoader(nsFrameLoader* aOther,
                                    nsRefPtr<nsFrameLoader>& aFirstToSwap,
                                    nsRefPtr<nsFrameLoader>& aSecondToSwap)
 {
   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;
 
   if (!ourContent || !otherContent) {
     // Can't handle this
     return NS_ERROR_NOT_IMPLEMENTED;
   }
--- a/content/base/src/nsFrameLoader.h
+++ b/content/base/src/nsFrameLoader.h
@@ -49,27 +49,32 @@
 #include "nsIFrameLoader.h"
 #include "nsIURI.h"
 #include "nsFrameMessageManager.h"
 
 class nsIContent;
 class nsIURI;
 class nsIFrameFrame;
 class nsIInProcessContentFrameMessageManager;
+class AutoResetInShow;
 
 class nsFrameLoader : public nsIFrameLoader
 {
+  friend class AutoResetInShow;
+
 protected:
   nsFrameLoader(nsIContent *aOwner) :
     mOwnerContent(aOwner),
     mDepthTooGreat(PR_FALSE),
     mIsTopLevelContent(PR_FALSE),
     mDestroyCalled(PR_FALSE),
     mNeedsAsyncDestroy(PR_FALSE),
-    mInSwap(PR_FALSE)
+    mInSwap(PR_FALSE),
+    mInShow(PR_FALSE),
+    mHideCalled(PR_FALSE)
   {}
 
 public:
   ~nsFrameLoader() {
     mNeedsAsyncDestroy = PR_TRUE;
     nsFrameLoader::Destroy();
   }
 
@@ -84,19 +89,19 @@ public:
   nsIDocShell* GetExistingDocShell() { return mDocShell; }
   nsPIDOMEventTarget* GetTabChildGlobalAsEventTarget();
   nsresult CreateStaticClone(nsIFrameLoader* aDest);
 
   /**
    * 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
    * the frame is being torn down; this notifies us that out widget and view
    * are going away and we should unhook from them.
    */
   void Hide();
 
@@ -125,11 +130,13 @@ public:
   nsRefPtr<nsFrameMessageManager> mMessageManager;
   nsCOMPtr<nsIInProcessContentFrameMessageManager> mChildMessageManager;
 private:
   PRPackedBool mDepthTooGreat : 1;
   PRPackedBool mIsTopLevelContent : 1;
   PRPackedBool mDestroyCalled : 1;
   PRPackedBool mNeedsAsyncDestroy : 1;
   PRPackedBool mInSwap : 1;
+  PRPackedBool mInShow : 1;
+  PRPackedBool mHideCalled : 1;
 };
 
 #endif
--- a/layout/generic/nsFrameFrame.cpp
+++ b/layout/generic/nsFrameFrame.cpp
@@ -216,24 +216,26 @@ protected:
    * says it should be called ObtainDocShell because of it's side effects.
    */
   nsIFrame* ObtainIntrinsicSizeFrame();
 
   nsRefPtr<nsFrameLoader> mFrameLoader;
   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)
 {
 }
 
 #ifdef ACCESSIBILITY
 NS_IMETHODIMP nsSubDocumentFrame::GetAccessible(nsIAccessible** aAccessible)
 {
   nsCOMPtr<nsIAccessibilityService> accService = do_GetService("@mozilla.org/accessibilityService;1");
 
@@ -322,29 +324,41 @@ inline PRInt32 ConvertOverflow(PRUint8 a
   }
   NS_NOTREACHED("invalid overflow value passed to ConvertOverflow");
   return nsIScrollable::Scrollbar_Auto;
 }
 
 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;
     }
   }
 }
 
 PRIntn
 nsSubDocumentFrame::GetSkipSides() const
 {
   return 0;
@@ -820,17 +834,17 @@ nsSubDocumentFrame::DestroyFrom(nsIFrame
   HideViewer();
 
   nsLeafFrame::DestroyFrom(aDestructRoot);
 }
 
 void
 nsSubDocumentFrame::HideViewer()
 {
-  if (mFrameLoader && mDidCreateDoc)
+  if (mFrameLoader && (mDidCreateDoc || mCallingShow))
     mFrameLoader->Hide();
 }
 
 nsIntSize
 nsSubDocumentFrame::GetMarginAttributes()
 {
   nsIntSize result(-1, -1);
   nsGenericHTMLElement *content = nsGenericHTMLElement::FromContent(mContent);
@@ -877,46 +891,51 @@ nsSubDocumentFrame::GetDocShell(nsIDocSh
 NS_IMETHODIMP
 nsSubDocumentFrame::BeginSwapDocShells(nsIFrame* aOther)
 {
   if (!aOther || aOther->GetType() != nsGkAtoms::subDocumentFrame) {
     return NS_ERROR_NOT_IMPLEMENTED;
   }
 
   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;
   }
 
   HideViewer();
   other->HideViewer();
 
   mFrameLoader.swap(other->mFrameLoader);
   return NS_OK;
 }
 
 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*
 nsSubDocumentFrame::CreateViewAndWidget(nsContentType aContentType)
 {
   if (mInnerView) {
     // Nothing to do here
     return mInnerView;
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -1424,13 +1424,21 @@ random-if(!haveTestPlugin) == 546071-1.h
 == 550716-1.html 550716-1-ref.html
 == 551463-1.html 551463-1-ref.html
 == 551699-1.html 551699-1-ref.html
 == 552334-1.html 552334-1-ref.html
 == 555388-1.html 555388-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
 == 565819-1.html 565819-ref.html
 == 565819-2.html 565819-ref.html