Multiple assertions with testcase that removes documentElement and uses DOMNodeRemoved event, including "ASSERTION: initial containing block already created"

VERIFIED FIXED

Status

()

defect
P2
normal
VERIFIED FIXED
13 years ago
5 months ago

People

(Reporter: jruderman, Assigned: sicking)

Tracking

(Blocks 1 bug, {assertion, testcase})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Posted file testcase
Loading the testcase triggers five assertions:

###!!! ASSERTION: initial containing block already created: 'nsnull == mInitialContainingBlock', file /Users/admin/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 8786

###!!! ASSERTION: Unexpected child of document element containing block: 'mDocElementContainingBlock->GetFirstChild(nsnull) == nsnull', file /Users/admin/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 8813

###!!! ASSERTION: already have a child frame: 'mFrames.IsEmpty()', file /Users/admin/trunk/mozilla/layout/generic/nsHTMLFrame.cpp, line 268

###!!! ASSERTION: Multiple element kids?: '!elementChild', file /Users/admin/trunk/mozilla/content/base/src/nsDocument.cpp, line 1985

###!!! ASSERTION: Incorrect mRootContent: 'mRootContent == elementChild', file /Users/admin/trunk/mozilla/content/base/src/nsDocument.cpp, line 1990

Reloading triggers an additional assertion:

###!!! ASSERTION: Some frame destructors were not called: 'mFrameCount == 0', file /Users/admin/trunk/mozilla/layout/base/nsPresShell.cpp, line 622
The "initial containing block already created" assertion also shows up in bug 366207.
Yech.  So the issue is that nsDocument::RemoveChildAt nulls out mRootContent before firing the DOM event (with a big comment explaining why), but IsAllowedAsChild assumes that it can trust the return value of GetRootContent...  In this case, the insertBefore call should probably throw, per spec, but because mRootContent is null we instead insert and try to construct frames for the <span> before the <html> has been removed.  :(
Flags: blocking1.9?
OS: Mac OS X → All
Hardware: Macintosh → All
Flags: blocking1.9? → blocking1.9+
Should this still block?
bz: so i don't fully understand that comment. It claims that we'll want to set mRootContent before since the event might set a new mRootContent. However when the event fires we still have the old element as documentElement, so the script shouldn't be able to set a new one. Or were you worried about the script replacing the old documentElement with a new one and then us nulling out the new one?

I wonder if we can revive the code that kills mRootContent in favor of walking the child list every time once Document::Destroy is fixed.
> Or were you worried about the script replacing the old documentElement with a
> new one and then us nulling out the new one?

Exactly.

> I wonder if we can revive the code that kills mRootContent in favor of walking
> the child list every time once Document::Destroy is fixed.

Hmm...  That seems very likely to me, yes!
Assignee: nobody → jonas
Component: Layout → DOM
Bug 386000 might be related.
Blocks: 386000
I think there's an sg:critical crash in here somewhere.
Posted patch Patch to fix (obsolete) — Splinter Review
Attachment #291972 - Flags: superreview?
Attachment #291972 - Flags: review?(bzbarsky)
Posted patch Patch to fix v1.1 (obsolete) — Splinter Review
I forgot to add the new mCachedRootContent member to the cycle collector
Attachment #291972 - Attachment is obsolete: true
Attachment #292134 - Flags: superreview?(bzbarsky)
Attachment #292134 - Flags: review?(bzbarsky)
Attachment #291972 - Flags: superreview?
Attachment #291972 - Flags: review?(bzbarsky)
Comment on attachment 292134 [details] [diff] [review]
Patch to fix v1.1

>Index: content/base/public/nsIDocument.h
>+  // This should be an nsIContent, but that would force us to pull in
>+  // nsIContent.h

I can't quite decide whether the cast this forces is worse than the manual refcounting we could do to make this an nsIContent*.  We'd just need to change GetRootContentInternal, the cycle collector stuff, and the destructor, right?  Might not be worth it...

>Index: content/base/src/nsDocument.cpp
>@@ -1209,17 +1200,16 @@ nsDocument::ResetToURI(nsIURI *aURI, nsI
>-  mRootContent = nsnull;

Worth clearing mCachedRootContent here?  I think it is; no reason to hold on to it (and hence keep it and its subtree alive in memory) if we know it'll be bogus.

>+nsDocument::GetRootContentInternal() const

>+  // are likly to appear before the root element.

"likely"

> nsDocument::RemoveChildAt(PRUint32 aIndex, PRBool aNotify)
>+  return nsGenericElement::doRemoveChildAt(aIndex, aNotify, oldKid,
>+                                           nsnull, this, mChildren);

I think we should null out the cached root content after that doRemoveChildAt call, for the same reason as in ResetToURI.  No need to keep it and its subtree alive.

>Index: content/html/document/src/nsHTMLDocument.cpp
> nsHTMLDocument::GetBodyContent()
>+  // are likly to appear before the root element.

"likely"

>Index: content/svg/content/src/nsSVGSVGElement.h
>+  PRBool IsRoot() {
>+    return IsInDoc() && !GetParent();

The patch in bug 326645 had an assert here, right?  Why did that go away?

r+sr=bzbarsky with the above changes.
Attachment #292134 - Flags: superreview?(bzbarsky)
Attachment #292134 - Flags: superreview+
Attachment #292134 - Flags: review?(bzbarsky)
Attachment #292134 - Flags: review+
And I assume that none of the bug 326645 regressions happen with this patch?
(In reply to comment #10)
> (From update of attachment 292134 [details] [diff] [review])
> >Index: content/base/public/nsIDocument.h
> >+  // This should be an nsIContent, but that would force us to pull in
> >+  // nsIContent.h
> 
> I can't quite decide whether the cast this forces is worse than the manual
> refcounting we could do to make this an nsIContent*.  We'd just need to change
> GetRootContentInternal, the cycle collector stuff, and the destructor, right? 
> Might not be worth it...

The bigger problem is that I'm calling mCachedRootContent->GetNodeParent(), which isn't going to compile if it's an nsIContent*

> Worth clearing mCachedRootContent here?  I think it is; no reason to hold on 
> to it (and hence keep it and its subtree alive in memory) if we know it'll be
> bogus.

Will do.

> > nsDocument::RemoveChildAt(PRUint32 aIndex, PRBool aNotify)
> >+  return nsGenericElement::doRemoveChildAt(aIndex, aNotify, oldKid,
> >+                                           nsnull, this, mChildren);
> 
> I think we should null out the cached root content after that doRemoveChildAt
> call, for the same reason as in ResetToURI.  No need to keep it and its 
> subtree alive.

Will do.

> >Index: content/svg/content/src/nsSVGSVGElement.h
> >+  PRBool IsRoot() {
> >+    return IsInDoc() && !GetParent();
> 
> The patch in bug 326645 had an assert here, right?  Why did that go away?

What assert are you talking about?

(In reply to comment #11)
> And I assume that none of the bug 326645 regressions happen with this patch?

I'll make sure to test that.
> The bigger problem is that I'm calling mCachedRootContent->GetNodeParent(),

Why can't you call GetNodeParent() on an nsIContent*?  Am I missing something?

> What assert are you talking about?

https://bugzilla.mozilla.org/attachment.cgi?id=216201 has:

+  PRBool IsRoot() {
+    NS_ASSERTION((IsInDoc() && !GetParent()) ==
+                 (GetOwnerDoc() && (GetOwnerDoc()->GetRootContent() == this)),
+                 "Can't determine if we're root");
+    return IsInDoc() && !GetParent();
+  }
(In reply to comment #13)
> > The bigger problem is that I'm calling mCachedRootContent->GetNodeParent(),
> 
> Why can't you call GetNodeParent() on an nsIContent*?  Am I missing something?

I can, but not without absolutely having to #include nsIContent.h. What I could do is reinterpret_cast to nsINode and then call, but that would still leave us with a reinterpret_cast. So I think the current solution is the best unfortunately. I really want to avoid pulling nsIContent.h into nsIDocument.h, as the latter is widely included all over the tree.

> +  PRBool IsRoot() {
> +    NS_ASSERTION((IsInDoc() && !GetParent()) ==
> +                 (GetOwnerDoc() && (GetOwnerDoc()->GetRootContent() == this)),
> +                 "Can't determine if we're root");
> +    return IsInDoc() && !GetParent();
> +  }

Not sure how I lost that. I'll definitely put it back.
> I can, but not without absolutely having to #include nsIContent.h.

Oh, right, because you're inlining it.  Ok, yeah.  Then your setup is good.
Posted patch Patch v2Splinter Review
Fixed all review comments.
Attachment #292134 - Attachment is obsolete: true
Fix checked in.
Flags: in-testsuite?
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I checked in the testcase as a crashtest.

Sicking, since this was a large patch, should it get more tests?
Flags: in-testsuite? → in-testsuite+
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050614 Minefield/3.0pre and the testcase from jesse, but when i load the testcase i get a new Assertion  - ###!!! ASSERTION: Mouse move must have some target content: 'targetElement', file c:/debug/mozilla/content/events/src/nsEventStateManager.cpp, line 3117 

filed bug 432826 for this assertion.
Status: RESOLVED → VERIFIED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.