47733 - Document == Document.documentElement.parentNode fails for created Documents

Status: VERIFIED FIXED

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

Description

[Bob Clary [:bc] (inactive)]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; m18) Gecko/20000730
BuildID:    2000073020

Document == Document.documentElement.parentNode should be an invariant

When creating Documents with or without a DTD,
Document.documentElement.parentNode returns null instead of the Document.


Reproducible: Always
Steps to Reproduce:
1. doc = document.implementation.createDocument(null, 'doc', null);
2. alert(doc.documentElement.parentNode)


Actual Results:  returns null

Expected Results:  returns Document

The native HTMLDocument document does satisfy
document.documentElement.parentNode == document

Comment 1

[Tomer Brisker]

Confirmed on Linux 2000080308.

Comment 2

[Johnny Stenback (:jst)]

This bug has been marked "future" because the original netscape engineer
working on this is over-burdened. If you feel this is an error, that you or
another known resource will be working on this bug,or if it blocks your work
in some way -- please attach your concern to the bug for reconsideration.

Comment 3

[Uriel]

Reproducible with Build: 2000090104

I'll attach a test case(based in the original report)

I'm not sure about the severity of this bug, but it looks to me like standards
compliance problem, maybe we must reconsider? ;)

Comment 4

[Uriel]

Attachment: test case

Comment 5

[Johnny Stenback (:jst)]

This patch fixes this bug.

Index: layout/xml/document/src/nsXMLDocument.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/xml/document/src/nsXMLDocument.cpp,v
retrieving revision 1.94
diff -u -r1.94 nsXMLDocument.cpp
--- nsXMLDocument.cpp   2001/01/05 00:35:20     1.94
+++ nsXMLDocument.cpp   2001/01/18 08:35:30
@@ -146,14 +146,18 @@

   if (aQualifiedName.Length() > 0) {
     nsCOMPtr<nsIDOMElement> root;
-    rv = doc->CreateElementNS(aNamespaceURI, aQualifiedName, getter_AddRefs(roo
t));
+    rv = doc->CreateElementNS(aNamespaceURI, aQualifiedName,
+                              getter_AddRefs(root));
+
     if (NS_SUCCEEDED(rv)) {
-      nsCOMPtr<nsIContent> content = do_QueryInterface(root);
-      doc->SetRootContent(content);
+      nsCOMPtr<nsIDOMNode> tmpNode;
+
+      doc->AppendChild(root, getter_AddRefs(tmpNode));
     }
   }

-  return doc->QueryInterface(NS_GET_IID(nsIDOMDocument), (void**) aInstancePtrR
esult);
+  return doc->QueryInterface(NS_GET_IID(nsIDOMDocument),
+                             (void**)aInstancePtrResult);
 }

Heikki, r=?

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Is doc a nsCOMPtr? If not, the final return would leak in the unlikely case that
QI failed. You need to explain to me why AppendChild() works and
SetRootContent() does not, and convince me that this does not break anything
else... Also, I think it would be better to return rv if it NS_FAILED(), in this
case it would be most likely because of out of memory situation. We really
should be returning errors in critical failures...

Comment 7

[Johnny Stenback (:jst)]

Attachment: Proposed fix

Comment 8

[Johnny Stenback (:jst)]

AppendChild() will not fail in the current code, it doesn't do any allocations
or nothing, but you're right, I should've checked the return value and the new
diff does that, and I know AppendChild() will potentially do an allocation in
the near future.

The difference between AppendChild() and SetRootContent() is that AppendChild()
does the things needed when a node is added to the document, such as setting the
parent of the new node, setting the document in the new node and removing the
node from it's old parent and SetRootContent() is simply a root pointer setter
in our document implementations (read the source :-).

As for the potential memory leak if QI fails (which it doesn't), we do that in,
like, all our NS_NewXXX methods (except the ones I've fixed :-), I also fixed
this one, I added a kung fu death grip and removed the unnecessary QI.

Please have a look at the attached patch.

Comment 9

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Now it looks good, r=heikki.

Comment 10

[Johnny Stenback (:jst)]

Fix checked in, marking FIXED.

Comment 11

[Prashant Desale]

QA contact Update

Comment 12

[Prashant Desale]

Updating QA contact to Shivakiran Tummala.

Comment 13

[Sivakiran Tummala]

verified