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 id13510
push usertnikkel@gmail.com
push dateFri, 28 May 2010 19:36:33 +0000
treeherdermozilla-central@9fc485016d40 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs561981
milestone1.9.3a5pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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