16605 - text node child-adding functions throw wrong exception

Status: VERIFIED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

DESCRIPTION:  When the functions insertBefore(), replaceChild(), and
appendChild() are called on a text node, they are currently throwing
NO_MODIFICATION_ALLOWED_ERR.  Really, from my reading of the spec they should be
throwing HIERARCHY_REQUEST_ERR (in the case of replaceChild(), for 2 reasons:
the oldChild isn't a child of the text node and it can't take children!).

STEPS TO REPRODUCE:
 * Load http://www.fas.harvard.edu/~dbaron/dom/test/one-core-html/exceptions or
   http://www.fas.harvard.edu/~dbaron/dom/test/one-core-xml/exceptions
 * Hit "test"

ACTUAL RESULTS:
 * The first test under each of the following bold headers is red:
   + "Testing exception throwing in Node::insertBefore():"
   + "Testing exception throwing in Node::replaceChild():"
   + "Testing exception throwing in Node::appendChild():"

EXPECTED RESULTS:
 * Those 3 tests should be green

DOES NOT WORK CORRECTLY ON:
 * Linux, viewer, 1999-10-15-11-M11

Comment 1

[vidur (gone)]

In an attempt to get my bug list in order again, marking all the bugs I have
currently as ASSIGNED.

Comment 2

[jst]

From looking at the code it seems like RemoveChild() has the same problem, IMO
RemoveChild() should return NOT_FOUND_ERR and not NO_MODIFICATION_ERR since the
child simply is not there!

I created a patch, I'll attach it as soon as I can verify that it works, right
now I'm unable to access harvard.edu!

Comment 3

[jst]

This is not linux specific.

Comment 4

[jst]

I'll attach a patch soon that fixes these problems (and adds some NULL pointer

checks). The patch does not completely fix dbarons tests because IMO

ReplaceChild() on a text node should throw NOT_FOUND_ERR and not

HERARCHY_REQUEST_ERR since the child is simply not there! The patch also fixes

RemoveChild().



I don't know if this ReplaceChild() behavior is correct or not, comments?

Comment 5

[jst]

Attachment: Proposed fix.

Comment 6

[vidur (gone)]

Johnny, your patch looks fine, but you should consider using the macros 
NS_ENSURE_ARG and NS_ENSURE_ARG_POINTER.

Comment 7

[Johnny Stenback (:jst)]

The fix for this is checked in now. Marking FIXED.

Comment 8

[Jan Carpenter]

Mass update of qa contact

Comment 9

[Prashant Desale]

QA contact Update

Comment 10

[Prashant Desale]

Updating QA contact to Shivakiran Tummala.

Comment 11

[Sivakiran Tummala]

replaceChild does not throw the rite exception ...used the testcases provided at
the URL http://www.fas.harvard.edu/~dbaron/dom/test/one-core-html/exceptions and
http://www.fas.harvard.edu/~dbaron/dom/test/one-core-xml/exceptions ...

reopening bug

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

0x80530008 looks like NS_ERROR_DOM_NOT_FOUND_ERR.

Comment 13

[vidur (gone)]

Reassigning to jst.

Comment 14

[Fabian Guisset]

simply change NS_ERROR_DOM_NOT_FOUND_ERR to NS_ERROR_DOM_HIERARCHY_REQUEST_ERR
at http://lxr.mozilla.org/mozilla/source/content/base/src/nsGenericDOMDataNode.h#102

Comment 15

[Fabian Guisset]

jst, I don't know what you think, but it seems the spec is unclear here
dbaron's test does the following:
textNode.replaceChild(document.createElement("p"), null);

We throw NOT_FOUND_ERR all the time because it's impossible that oldChild (null
in the testcase) be a child of this text node.

However we could as well throw HIERARCHY_REQUEST_ERR since newChild (a paragraph
element in this case) can never become a child of this node.

We can chose...

Comment 16

[Fabian Guisset]

More realistic milestone (M15? Sounds like stone age). Adding [invalid?] to
whiteboard to remember to take a decision. Priority P4. Sending to DOM Core, not
a DOM HTML issue.

Comment 17

[Johnny Stenback (:jst)]

Fabian, we should have a look at the W3C tests and see what they expect, if they
don't care, then we can go either way...

I'm handing this over to you, and it's up to you to decide what to do about this
now :-)

Comment 18

[Fabian Guisset]

I just realized my previous comment did not make sense: when null is passed as
refChild, we have to appendChild(), so we should not throw NOT_FOUND_ERR in any
case. Even though the DOM1 test suite does not explicitely test this, they make
it clear they expect HIERARCHY_REQUEST_ERR.
Patch coming up.

Comment 19

[Fabian Guisset]

Attachment: Proposed fix.

Patch modifies nsGenericDOMDataNode::ReplaceChild to throw
HIERARCHY_REQUEST_ERR instead of NOT_FOUND_ERR

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 70091 [details] [diff] [review]
Proposed fix.

sr=jst. IMO this can go in as is, no r= needed for this trivial fix.

Comment 21

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.9

Comment 22

[Fabian Guisset]

Checked in, marking FIXED. Off with this one's head.

Comment 23

[Sivakiran Tummala]

verified fixed. insertBefore(), replaceChild nad appendChild() are throwing
right exceptions.