561981 - "ABORT: PresArena: poison overwritten" with XBL, iframe, contentEditable

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: semi-reduced testcase (crashes Firefox within a few seconds)

Steps:
1. Set layout.debug.enable_data_xbl to true.
2. Load the testcase in a debug build of Firefox.

Result: Abort within a few seconds.  You may see an assertion before the abort, and you may suddenly feel like you need to take a shower.

###!!! ASSERTION: CreateRenderingContext failure: 'Not Reached', file /Users/jruderman/central/layout/base/nsPresShell.cpp, line 7152

###!!! ABORT: PresArena: poison overwritten: '*reinterpret_cast<PRUword*>(p) == ARENA_POISON', file /Users/jruderman/central/layout/base/nsPresArena.cpp, line 331

Comment 1

[Jesse Ruderman]

Still happens on trunk (not fixed by the patch in bug 535926).

Comment 2

[Timothy Nikkel (:tnikkel)]

Bisected locally, this is caused by bug 557398.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Timothy, would you mind taking this?

Comment 4

[Timothy Nikkel (:tnikkel)]

Sure, could someone cc me on bug 557398 then? :)

Comment 5

[Zack Weinberg (:zwol)]

you asked for it, you got it :)

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: reduced testcase (crashes firefox)

This is pretty much minimal.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: stack showing the problem

This is a partly incorrect stack (because it was produced via XPCOM_DEBUG_BREAK=stack and fix-linux-stack.pl) showing where the problem occurs.

Comment 8

[Timothy Nikkel (:tnikkel)]

After setting the binding on the iframe but before the binding document loads (nsXBLStreamListener::Load) we set the binding on the root element which posts a restyle for the inline style change. Then the binding document for the iframe loads (nsXBLStreamListener::Load) and calls PresShell::RecreateFramesFor, this creates the new subdocument frame and creates a AsyncFrameInit script runner. When the script blocker in PresShell::RecreateFramesFor is removed we run the AsyncFrameInit and call nsSubDocumentFrame::ShowViewer. The SetDesignMode calls in nsFrameLoader::Show end up flushing via the path nsHTMLDocument::SetDesignMode -> nsHTMLDocument::EditingStateChanged -> nsEditingSession::MakeWindowEditable -> nsEditingSession::SetupEditorOnWindow. If this flush call would have happened via nsSubDocumentFrame::Init like before bug 557398 then it would have been a no-op because we had a script blocker and were in frame construction. But now we process the restyle on the root element which reconstructs and destroys the subdocument frame. The destroy-caused nsSubDocumentFrame::HideViewer call does nothing because we haven't finished calling nsFrameLoader::Show yet. This causes the "CreateRenderingContext failure" assertion, because we find the old presshell when the new subdocument frame calls nsFrameLoader::Show and it exits and doesn't create the views. And then when the original nsSubDocumentFrame::ShowViewer finishes we write to already freed memory and get the poison assert.

So I think we need to guard against nsFrameLoader::Show destroying us and re-entering into Show or Hide calls.

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Would it be enough to have scriptblocker in AsyncFrameInit::Run() ?

Comment 11

[Timothy Nikkel (:tnikkel)]

That would be enough to fix this bug, but that would regress bug 557398.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Ah, right. What about using scriptblockers in nsFrameLoader::Show?

Comment 13

[Timothy Nikkel (:tnikkel)]

Around the SetDesignMode calls? I think that would work.

I avoided this approach because I didn't want to block flushing using a script blocker for some of the reasons in this thread http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/d8af77e8fd44dc12

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think in general we should make flushes work instead of blocking them, because blocking a flush that we actually need for correct operation will obviously cause incorrect operation.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

I'll review this tomorrow.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 445234 [details] [diff] [review]
patch

># HG changeset patch
># Parent 3035b2b9a98eecc216eff0229044557e6939d306
># User Timothy Nikkel <[e-mail]>
>Bug 561981. Guard against nsFrameLoader::Show flushing and re-entering or destroying us.
>
>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -494,35 +494,40 @@ AllDescendantsOfType(nsIDocShellTreeItem
>     if (kidType != aType || !AllDescendantsOfType(kid, aType)) {
>       return PR_FALSE;
>     }
>   }
> 
>   return PR_TRUE;
> }
> 
>-bool
>+PRBool
> nsFrameLoader::Show(PRInt32 marginWidth, PRInt32 marginHeight,
>                     PRInt32 scrollbarPrefX, PRInt32 scrollbarPrefY,
>                     nsIFrameFrame* frame)
> {
>+  if (mInShow) {
>+    return PR_FALSE;
>+  }
>+  mInShow = PR_TRUE;
So whenever something fails in this method, mInShow stays PR_TRUE.
Please use some helper class in stack.

> void
> nsFrameLoader::Hide()
> {
>+  if (mHideCalled) {
>+    return;
>+  }
>+  if (mInShow) {
>+    mHideCalled = PR_TRUE;
>+    return;
>+  }
Hmmm, so have you tested something like
iframe.style.display = "none";
iframe.style.display = "";
or
iframe.parentNode.style.display = "none";
iframe.parentNode.style.display = "";


>-  if (mInSwap || aOther->mInSwap) {
>+  if (mInSwap || aOther->mInSwap || mInShow || aOther->mInShow) {
>     return NS_ERROR_NOT_IMPLEMENTED;
>   }
Hmm, the error is actually a bit wrong in this case.
if mInShow, we should return NS_ERROR_FAILURE or NS_ERROR_UNEXPECTED
So perhaps just
NS_ENSURE_STATE(!mInShow && !aOther->mInShow);

>+      PRBool didCreateDoc =
>+                      frameloader->Show(margin.width, margin.height,
Strange indentation

Those fixed, and tests added for style.display case, then r=me

Comment 17

[Timothy Nikkel (:tnikkel)]

(In reply to comment #16)
> So whenever something fails in this method, mInShow stays PR_TRUE.
> Please use some helper class in stack.

Good catch.

> Hmmm, so have you tested something like
> iframe.style.display = "none";
> iframe.style.display = "";
> or
> iframe.parentNode.style.display = "none";
> iframe.parentNode.style.display = "";

I added 8 reftests for each combination of the following 3 variables
1) toggling display on iframe or parent
2) display none -> "" -> none vs display "" -> none -> ""
3) make the changes from onload vs in an inline script

> Hmm, the error is actually a bit wrong in this case.
> if mInShow, we should return NS_ERROR_FAILURE or NS_ERROR_UNEXPECTED
> So perhaps just
> NS_ENSURE_STATE(!mInShow && !aOther->mInShow);

Made that change.

> Strange indentation

Fixed.

Comment 18

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

smaug, could you look over the changes to make sure I did the stack class correctly?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 447625 [details] [diff] [review]
patch v2

>+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;

Couldn't you just use AutoRestore?
AutoRestore<PRBool> InShow(mInShow);
mInShow = PR_TRUE;

> 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();
>     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;
Actually, frameLoader should be nsRefPtr. Could you change that.
(Frameloader *must* be kept alive during show.)

Comment 20

[Timothy Nikkel (:tnikkel)]

(In reply to comment #19)
> Couldn't you just use AutoRestore?
> AutoRestore<PRBool> InShow(mInShow);
> mInShow = PR_TRUE;

AutoRestore doesn't work because mInShow is a PRPackedBool and AutoRestore can't save a pointer to a bit in a bitfield.

> Actually, frameLoader should be nsRefPtr. Could you change that.
> (Frameloader *must* be kept alive during show.)

Done.

Comment 21

[Damon Sicore (:damons)]

Blocking until we have a resolution of some type.

Comment 22

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/9fc485016d40

Comment 23

[Timothy Nikkel (:tnikkel)]

This will need to be rolled up into bug 557398 for wherever we take that bug.

Comment 24

[Timothy Nikkel (:tnikkel)]

Added crashtest
http://hg.mozilla.org/mozilla-central/rev/e347b924383b