Closed
Bug 613793
Opened 14 years ago
Closed 14 years ago
"ASSERTION: Not an element?" with doctype, document fragment
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla2.0b8
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: jruderman, Assigned: Ms2ger)
References
Details
(Keywords: assertion, testcase)
Attachments
(3 files, 2 obsolete files)
256 bytes,
text/html
|
Details | |
6.38 KB,
text/plain
|
Details | |
4.96 KB,
patch
|
Details | Diff | Splinter Review |
###!!! ASSERTION: Nodes that are not documents, document fragments or elements can't be parents!: 'aParent->IsNodeOfType(nsINode::eDOCUMENT) || aParent->IsNodeOfType(nsINode::eDOCUMENT_FRAGMENT) || aParent->IsElement()', file content/base/src/nsGenericElement.cpp, line 3827 (like in bug 564047), followed by an additional assertion: ###!!! ASSERTION: Not an element?: 'IsElement()', file Element.h, line 95
Reporter | ||
Comment 1•14 years ago
|
||
Comment 2•14 years ago
|
||
Hmm. In this case aParent is of course an nsDOMDocumentType. nsINode::ReplaceOrInsertBefore should probably check for document/fragment/element at the beginning, I think, instead of checking for "not data and not attribute"...
Comment 3•14 years ago
|
||
Oh, but that second assert is bad; we really do need to fix this. Ms2ger, if you don't have time let me know and I'll do it.
blocking2.0: --- → ?
Assignee | ||
Comment 4•14 years ago
|
||
This is untested, but I believe it would fix the bug. I'm not sure when I'll have time to test it, so I'm putting it up already.
Attachment #492212 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•14 years ago
|
||
The included test passes. I might have time to do a full test run later this week, but no promises.
Comment 6•14 years ago
|
||
Comment on attachment 492212 [details] [diff] [review] Patch v1 No, I really do think we should whitelist (incidentally, that would be faster), not blacklist.
Attachment #492212 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 7•14 years ago
|
||
Something like this?
Attachment #492212 -
Attachment is obsolete: true
Attachment #493592 -
Flags: review?(bzbarsky)
Comment 8•14 years ago
|
||
Comment on attachment 493592 [details] [diff] [review] Patch v2 Yes, though you should take the eAttribute check out too, and drop the "nsINode::" bits. r=me with those changes.
Attachment #493592 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 9•14 years ago
|
||
Comment on attachment 493592 [details] [diff] [review] Patch v2 Dropping the Attr check causes an unexpected pass in the dom test suite. Can I just disable it?
Comment 10•14 years ago
|
||
Which test has the unexpected pass?
Assignee | ||
Comment 11•14 years ago
|
||
That would be <http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/dom-level1-core/test_hc_attrappendchild2.html?force=1>, IIRC.
Comment 12•14 years ago
|
||
Ah. That test is testing that appending an element to an Attr throws HIERARCHY_REQUEST_ERR. Which we didn't use to do, and which we now do. So the right thing is to just remove this test from the attributeModTests array in dom/tests/mochitest/dom-level1-core/exclusions.js, I think.
Assignee | ||
Comment 13•14 years ago
|
||
Will do.
Comment 14•14 years ago
|
||
Not sure I can say this is a blocker, but I'll approve the patch.
blocking2.0: ? → -
Updated•14 years ago
|
Attachment #493592 -
Flags: approval2.0+
Assignee | ||
Comment 15•14 years ago
|
||
Keywords: checkin-needed
Comment 16•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/583c8c32b487
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•