56764 - Document appendChild, insertBefore fail to throw HIERARCHY_REQUEST_ERR when inserting Text Nodes

Status: RESOLVED FIXED

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

Description

[Bob Clary [:bc] (inactive)]

When attempting to add a Text Node to a Document's children, a
HIERARCHY_REQUEST_ERR exception should be thrown. I have used appendChild and
insertBefore to insert a Text Node into a Document's children. I have not tested
replaceChild due to a crasher Bug 48031, but it is probable that it has the same
problem.

Visit the above URL, select the tc_cdoc502 test case and submit. Click on links
in the left frame to display a report. Due to a scrollbar bug, you will need to
click into the right frame and use the keyboard to navigate within the report.

Comment 1

[Johnny Stenback (:jst)]

Thanks for catching this problem, however... --> Future.

Comment 2

[Bob Clary [:bc] (inactive)]

Document should have at most 1 DocumentType child.  It is currently possible to
use appendChild (at least) to give Document more than 1 DocumentType child.  It
should throw a HIERARCHY_REQUEST_ERR.  See the same URL and test case for an
example. Yeah, I know... future, I'm just making the report complete.

Comment 3

[Hixie (not reading bugmail)]

Bob: I agree with your interpretation, but can't actually find anywhere in DOM2
that specifically says that a Document node cannot have more than one child.
Could you point me to the relevant section? Thanks!

Comment 4

[Bob Clary [:bc] (inactive)]

http://www.w3.org/TR/DOM-Level-2-Core/core.html 

Section 1.1.1. The Dom Structure Model

Document -- Element (maximum of one), ProcessingInstruction, Comment, 
DocumentType (maximum of one) 

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: testcase: use appendChild to add Text Node to Document

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: testcase: add Second DocumentType child to Document

Comment 7

[Hixie (not reading bugmail)]

Thanks Bob!

Comment 8

[Prashant Desale]

QA contact Update

Comment 9

[Prashant Desale]

Updating QA contact to Shivakiran Tummala.

Comment 10

[Brian Clark]

I confirmed that this is still broken on Mozilla 0.9.3 on MacOS 9.1.  

I tested using the DOM HTML Level 2 Conformance tests provided by Netscape.  You
can check this by running the test "tc_hdoc502 Test Document children" from the
link for DOM HTML Level 2 Conformance on this page:

http://developer.netscape.com/evangelism/tools/testsuites/

Comment 11

[Fabian Guisset]

Attachment: Proposed fix.

The attached patch fixes both issues in this bug: you can't append (or replace,
for that matter) text nodes as direct child of a Document, and you can't insert
a second documentType. It doesn't fix the testcases attached in this bug for a
reason I haven't figured out yet (probably because of the use of
document.implementation.createDocument()), but it works in all my tests which
used document.appendChild() instead. If you have any idea let me know.
I added a convenience IsAllowedAsChild, if you prefer that I don't add this
method, no biggie, I'll just remove it :-)

Comment 12

[Bob Clary [:bc] (inactive)]

the document.implementation.createDocument will return a pure xml doc without
doctype. did you test with an HTMLDocument?

Comment 13

[Johnny Stenback (:jst)]

- No need for nsDocument::IsAllowedAsChild() to be part of nsDocument, or if you
want to leave it as part of nsDocument then make it static (and add a // static
comment above the method). I'd suggest making IsAllowedAsChild() a file static
inline function.

- No need for the extra parens here:

+ if ((!aNewChild) || (!aOldChild)) {

Fix that and I'll sr.

Comment 14

[Fabian Guisset]

Attachment: Modified fix.

I left IsAllowedAsChild as member of nsDocument because it is a method specific
to nsDocument, other type of nodes allow other children (but I don't know if
the reason is compelling enough to justify keeping it in nsDocument). I also
tried to do as you said, make it a "file static inline function", but that was
not in my C++ book, and all my attempts have resulted in compile errors ;-) So
if you want to have a "file static inline function", you'll have to tell me how
to do it :-)

Comment 15

[Fabian Guisset]

btw I haven't figured out why it doesn't work with pure XML documents yet -- yes
I was testing with a HTMLDocument, but they share the same code (nsDocument), so...

Comment 16

[Johnny Stenback (:jst)]

By file static I meant a old C-style static function (i.e. not a member of a
class). This in nsDocument.cpp would do it, this function should not be
mentioned in nsDocument.h at all:

// IsAllowedAsChild() returns true if this type of node is allowed
// as child of Document, false otherwise.
static inline PRBool
IsAllowedAsChild(PRUint16 aNodeType)
{
  ...
}

Then when you call it, you can call it as ::IsAllowedAsChild(), or you can loose
the :: if you want to... :-)

Comment 17

[Fabian Guisset]

Attachment: Proposed fix v3

Comment 18

[Fabian Guisset]

Attachment: Testcase using a HTML document

This is the testcase I used to test the fix... it uses a HTML document. I still
don't know why it doesn't work with the XML document created with
implementation.createDocument() (see the other testcases in this bug)

Comment 19

[Fabian Guisset]

Nominating for 1.0 since there is a patch, marking P4, low visiblity and low
severity, but standards-compliance issue.

Comment 20

[Fabian Guisset]

Reassigning to myself now that I have my account. Woohoo!
This patch needs review :-)

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 67750 [details] [diff] [review]
Proposed fix v3

sr=jst

Comment 22

[Axel Hecht]

I have seen whitespace-only textnodes in the DOM, and I guess we break
that part of the spec with serializers in mind. Should the appendChild and
friend methods be unable to generate a document we would like to pass to a
serializer?

Comment 23

[Fabian Guisset]

Marking 1.0, this is a compliance issue. Might even be later if the drivers
don't like it. jst could you please answer Pike's question? I can't answer it.
Accepting bug.

Comment 24

[Johnny Stenback (:jst)]

The DOM spec explicitly says that text nodes are not allowed as part of a
document object, thus we should not allow that either. The serializers can do
their best to make up whitespace between immediate children of the document.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 67750 [details] [diff] [review]
Proposed fix v3

r=sicking

Comment 26

[Fabian Guisset]

This is not worth the risk for 1.0, I'm pushing to 1.0.1. God knows what ugly
regression crept in here, and I'd like to fix the XML issue (bclary's testcases)
before checking it in.

Comment 27

[Fabian Guisset]

Future, unless someone wants to update the patch (that needs testing)

Comment 28

[Axel Hecht]

Is this coordinated with the editor folks?

Comment 29

[Fabian Guisset]

not that I know.. do they need this patch?

Comment 30

[Peter Van der Beken [:peterv]]

Taking this. Fabian: shout if you disagree.

Comment 31

[Peter Van der Beken [:peterv]]

See also bug 199613.

Comment 32

[Peter Van der Beken [:peterv]]

Attachment: v4

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 131819 [details] [diff] [review]
v4

>@@ -3203,56 +3231,49 @@ nsDocument::InsertBefore(nsIDOMNode* aNe
>     return rv;
>   }
> 
>-  // If it's a child type we can't handle (per DOM spec), or if it's an
>-  // element and we already have a root (our addition to DOM spec), throw
>-  // HIERARCHY_REQUEST_ERR.
>+  PRUint16 nodeType;
>   aNewChild->GetNodeType(&nodeType);
>-  if (((nodeType != COMMENT_NODE) &&
>-       (nodeType != TEXT_NODE) &&
>-       (nodeType != PROCESSING_INSTRUCTION_NODE) &&
>-       (nodeType != DOCUMENT_TYPE_NODE) &&
>-       (nodeType != ELEMENT_NODE)) ||
>-      ((nodeType == ELEMENT_NODE) && mRootContent)) {
>-    return NS_ERROR_DOM_HIERARCHY_REQUEST_ERR;
>+
>+  rv = IsAllowedAsChild(nodeType, nsnull);
>+  if (NS_FAILED(rv)) {
>+    return rv;

please use NS_ENSURE_SUCCESS(rv, rv)

>     indx = mChildren.IndexOf(refContent);
>-    if (indx != -1) {
>-      mChildren.InsertObjectAt(content, indx);
>-    } else {
>+    if (indx == -1) {
>       // couldn't find refChild
>       return NS_ERROR_DOM_NOT_FOUND_ERR;
>     }
>+
>+    mChildren.InsertObjectAt(content, indx);

this could use NS_ENSURE_FALSE(indx == -1, ...
(or the other way around of course)

>@@ -3263,40 +3284,30 @@ nsDocument::ReplaceChild(nsIDOMNode* aNe
> {
>   *aReturn = nsnull;
> 
>-  NS_ENSURE_TRUE(aNewChild && aOldChild, NS_ERROR_NULL_POINTER);
>+  if (!aNewChild || !aOldChild) {
>+    return NS_ERROR_NULL_POINTER;
>+  }

Hmm.. you've got something against NS_ENSURE?

>-  nsresult rv = NS_OK;
>-  PRInt32 indx;
>-  PRUint16 nodeType;
>+  nsCOMPtr<nsIContent> refContent(do_QueryInterface(aOldChild));
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(aNewChild);
>+  if (!refContent || !content) {
>+    return NS_ERROR_DOM_HIERARCHY_REQUEST_ERR;
>+  }

ENSURE would work here too. Should you throw an NS_ERROR_DOM_NOT_FOUND_ERR for
!refContent?

>+  rv = IsAllowedAsChild(nodeType, refContent);
>+  if (NS_FAILED(rv)) {
>+    return rv;
>   }

ENSURE?

r=sicking though i'm not sure i like this spreading hatred against NS_ENSURE,
it is our friend that we should love and cherish.

Comment 34

[Peter Van der Beken [:peterv]]

Well, normally we don't use NS_ENSURE for DOM errors since they're usually not
an error in Mozilla but could be just an error in a webpage and we don't want to
warn for every one of those :-). I'll make the NOT_FOUND_ERR change.

Comment 35

[Johnny Stenback (:jst)]

Comment on attachment 131819 [details] [diff] [review]
v4

sr=jst

Comment 36

[Peter Van der Beken [:peterv]]

Checked in attachment 131819 [details] [diff] [review].
The testcase in attachment 17742 [details] fails for a different reason,
createDocument(null, 'TEST', null) creates a document with a documentElement so
calling doc.appendChild(type1) should throw HIERARCHY_REQUEST_ERR because the
doctype needs to be before the documentElement. Small patch coming up to fix that.

Comment 37

[Peter Van der Beken [:peterv]]

Attachment: Throw exception if trying to insert docType after documentElement

Comment 38

[Johnny Stenback (:jst)]

Comment on attachment 133757 [details] [diff] [review]
Throw exception if trying to insert docType after documentElement

sr=jst

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 133757 [details] [diff] [review]
Throw exception if trying to insert docType after documentElement

>@@ -3207,6 +3212,12 @@ nsDocument::InsertBefore(nsIDOMNode* aNe
>       return NS_ERROR_DOM_NOT_FOUND_ERR;
>     }
> 
>+    if (nodeType == nsIDOMNode::DOCUMENT_TYPE_NODE &&
>+        indx > mChildren.IndexOf(mRootContent)) {

Don't you need to test that mRootContent is non-null too? Same in ReplaceChild.

With that, r=sicking

Comment 40

[Peter Van der Beken [:peterv]]

Checked in.

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 133757 [details] [diff] [review]
Throw exception if trying to insert docType after documentElement

wait, where in the dom-spec does it say that doctypes arn't allowed after the
documentelement?

Comment 42

[Peter Van der Beken [:peterv]]

The DOM spec doesn't, the XML spec does though and our code relies on it (see
for example GetDoctype).

Comment 43

[Peter Van der Beken [:peterv]]

Moved the issue to bug 223189.

Comment 44

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

Added a test after this was regressed in bug 540848 and refixed in bug 761120.