813919 - "Assertion failure: aFirstNewContent->IsNodeOfType(nsINode::eTEXT)"

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

Assertion failure: aFirstNewContent->IsNodeOfType(nsINode::eTEXT), at content/base/src/nsRange.cpp:562

This assertion is part of code added in bug 803924.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Lovely.
Totally forgot comment node.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Mats, could you look at this? If not, assign to me.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Though, I believe it is just the assertion check which is wrong.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Bah, I started to look at this. Taking.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Or, no. Need to figure out what the spec says about this.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

In Gecko "data nodes" have SplitData, and deleteContents/extractContents end up using that
if a data node needs to be split to two.
I *think* spec should have similar split handling, but need to investigate this some more.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Hmm, the offset change because of split handling should be just temporary Gecko implementation detail in this case.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Same problem with ProcessingInstruction

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Mats, I think we should just fix the assertion to check eDATA_NODE, and add plenty of tests
for comment and pi. Sounds right?

Comment 11

[Mats Palmgren (inactive)]

Yeah, I think the code is correct, it's just that the assertion is too strict.
So I think we should change it to something like this:
MOZ_ASSERT(aFirstNewContent->IsNodeOfType(nsINode::eTEXT) ||
           aFirstNewContent->IsNodeOfType(nsINode::eCOMMENT) ||
           aFirstNewContent->IsNodeOfType(nsINode::ePROCESSING_INSTRUCTION));

We do handle PI although the spec only says we should split Text and Comment.
File a separate bug on that?

Comment 12

[Mats Palmgren (inactive)]

Yes, eDATA_NODE should work too, until we decide what to do with PI.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Didn't we change PI handling explicitly to be more consistent with
text and comment, and someone filed a spec bug... or was it just about comment.
Can't remember now.

Comment 14

[Jesse Ruderman]

Same problem in nsRange::ContentInserted

  MOZ_ASSERT(aChild->IsNodeOfType(nsINode::eTEXT));

Assertion failure: aChild->IsNodeOfType(nsINode::eTEXT), at content/base/src/nsRange.cpp:599

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Sorry, took some time. Was busy with other stuff.

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 684695 [details] [diff] [review]
patch

Looks good, r=mats

Please file a follow-up bug on what to do about PI.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/e685ffb06975

Followup https://bugzilla.mozilla.org/show_bug.cgi?id=814706