Closed
Bug 366200
Opened 18 years ago
Closed 17 years ago
Multiple assertions with testcase that removes documentElement and uses DOMNodeRemoved event, including "ASSERTION: initial containing block already created"
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
People
(Reporter: jruderman, Assigned: sicking)
References
Details
(Keywords: assertion, testcase)
Attachments
(2 files, 2 obsolete files)
|
710 bytes,
application/xhtml+xml
|
Details | |
|
52.80 KB,
patch
|
Details | Diff | Splinter Review |
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
|
Reporter | |
Comment 1•18 years ago
|
||
The "initial containing block already created" assertion also shows up in bug 366207.
|
||
Comment 2•18 years ago
|
||
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+
|
||
Comment 3•17 years ago
|
||
Should this still block?
|
Assignee | |
Comment 4•17 years ago
|
||
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.
|
|
||
Comment 5•17 years ago
|
||
> 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 | |
Updated•17 years ago
|
Assignee: nobody → jonas
Component: Layout → DOM
|
|
Reporter | |
Comment 6•17 years ago
|
||
Bug 386000 might be related.
|
|
Reporter | |
Comment 7•17 years ago
|
||
I think there's an sg:critical crash in here somewhere.
|
|
Assignee | |
Updated•17 years ago
|
Priority: -- → P2
|
|
Assignee | |
Comment 8•17 years ago
|
||
Attachment #291972 -
Flags: superreview?
Attachment #291972 -
Flags: review?(bzbarsky)
|
|
Assignee | |
Comment 9•17 years ago
|
||
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 10•17 years ago
|
||
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+
|
|
||
Comment 11•17 years ago
|
||
And I assume that none of the bug 326645 regressions happen with this patch?
|
|
Assignee | |
Comment 12•17 years ago
|
||
(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.
|
|
||
Comment 13•17 years ago
|
||
> 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();
+ }
|
|
Assignee | |
Comment 14•17 years ago
|
||
(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.
|
|
||
Comment 15•17 years ago
|
||
> 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.
|
|
Assignee | |
Comment 16•17 years ago
|
||
Fixed all review comments.
Attachment #292134 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•17 years ago
|
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
|
|
Reporter | |
Comment 18•17 years ago
|
||
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+
|
|
Assignee | |
Comment 19•17 years ago
|
||
I think it's fine.
|
||
Comment 20•17 years ago
|
||
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
|
||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•