Closed Bug 561981 Opened 10 years ago Closed 9 years ago

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

Categories

(Core :: Layout, defect, P2, critical)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: jruderman, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [sg:critical?][critsmash:patch])

Attachments

(4 files, 1 obsolete file)

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
Still happens on trunk (not fixed by the patch in bug 535926).
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:investigating]
Bisected locally, this is caused by bug 557398.
Blocks: 557398
blocking2.0: --- → ?
Timothy, would you mind taking this?
Assignee: nobody → tnikkel
Sure, could someone cc me on bug 557398 then? :)
you asked for it, you got it :)
This is pretty much minimal.
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.
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.
Attachment #445233 - Attachment mime type: application/octet-stream → text/plain
Attached patch patch (obsolete) — Splinter Review
Attachment #445234 - Flags: review?(Olli.Pettay)
Would it be enough to have scriptblocker in AsyncFrameInit::Run() ?
That would be enough to fix this bug, but that would regress bug 557398.
Ah, right. What about using scriptblockers in nsFrameLoader::Show?
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
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.
Whiteboard: [sg:critical?][critsmash:investigating] → [sg:critical?][critsmash:investigating][needs review]
I'll review this tomorrow.
Comment on attachment 445234 [details] [diff] [review]
patch

># HG changeset patch
># Parent 3035b2b9a98eecc216eff0229044557e6939d306
># User Timothy Nikkel <tnikkel@gmail.com>
>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
Attachment #445234 - Flags: review?(Olli.Pettay) → review+
(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.
Attached patch patch v2Splinter Review
smaug, could you look over the changes to make sure I did the stack class correctly?
Attachment #445234 - Attachment is obsolete: true
Attachment #447625 - Flags: review?(Olli.Pettay)
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.)
Attachment #447625 - Flags: review?(Olli.Pettay) → review+
(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.
Whiteboard: [sg:critical?][critsmash:investigating][needs review] → [sg:critical?][critsmash:patch][needs review]
Whiteboard: [sg:critical?][critsmash:patch][needs review] → [sg:critical?][critsmash:patch][needs landing]
Blocking until we have a resolution of some type.
blocking2.0: ? → final+
http://hg.mozilla.org/mozilla-central/rev/9fc485016d40
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [sg:critical?][critsmash:patch][needs landing] → [sg:critical?][critsmash:patch]
This will need to be rolled up into bug 557398 for wherever we take that bug.
Group: core-security
Added crashtest
http://hg.mozilla.org/mozilla-central/rev/e347b924383b
Flags: in-testsuite? → in-testsuite+
Blocks: 569674
You need to log in before you can comment on or make changes to this bug.