613793 - "ASSERTION: Not an element?" with doctype, document fragment

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! 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

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

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.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

The included test passes. I might have time to do a full test run later this week, but no promises.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 492212 [details] [diff] [review]
Patch v1

No, I really do think we should whitelist (incidentally, that would be faster), not blacklist.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Something like this?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

[Boris Zbarsky [:bzbarsky]]

Which test has the unexpected pass?

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Will do.

Comment 14

[Johnny Stenback (:jst)]

Not sure I can say this is a blocker, but I'll approve the patch.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch for checkin

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/583c8c32b487