594767 - importNode fails when using a document created with createHTMLDocument

Status: RESOLVED FIXED

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

Description

[Eric Shepherd [:sheppy]]

Attachment: Test case

Attempting to import nodes from a document created using createHTMLDocument is failing with NS_ERROR_DOM_PROP_ACCESS_DENIED, code 1010.

Attached is a test case.

Comment 1

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

Seems like there are also other problems with CreateHTMLDocument.
Well, at least one. It doesn't seem to mark the created document as data document, if I read the code correctly.

Comment 2

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

Where is the principal set the the created document?

Comment 3

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

And seems like .innerHTML doesn't work with created document either.

Comment 4

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

So, the create html document doesn't have right principal and documentURI,
and it is not marked as datadocument.

Comment 5

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

Doh! Sorry guys, should have caught this during review.

Comment 6

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

The tricky thing is that documentURI should return null, but content sink
assumes that it is non null.

I could add a hack that nsDocument::GetDocumentURI would return null also
when some specific flag is set to document.
bool mNullDocument;
mDocumentURI could point to about:blank, I think.

Ugly and hackish, but what else could be done easily?

Comment 7

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

Or perhaps I just fix ContentSink.
It doesn't seem to use its mDocumentURI in too many places.

Comment 8

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

What do we do for XML documents created using DOMImplementation? Or HTML documents created using XSLTProcessor?

Comment 9

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

Attachment: use the normal way to create data documents

Comment 10

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

Comment on attachment 475060 [details] [diff] [review]
use the normal way to create data documents

Um, this seems to regress something in the testcase test_bug396843.html

Comment 11

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

Actually, the patch reveals another (small) bug in nsDOMClassInfo.cpp.
Since that is trivial to fix, I'll

Comment 12

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

...fix it in this bug.

Comment 13

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

Attachment: patch

Fix the testcase.
This makes implementation.create*Document().documentURI to work per DOM 3 Core.

Comment 14

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

And based on testing webkit and opera return null too for documentURI.

Comment 15

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

Are you sure we're not doing any security checks based on the document URI which will behave poorly, i.e. crash or allow things it shouldn't?

Haven't we previously made DOM created documents have the URI of creating document?

Comment 16

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

(In reply to comment #15)
> Are you sure we're not doing any security checks based on the document URI
> which will behave poorly, i.e. crash or allow things it shouldn't?
nsIDocument::GetDocumentURI() clearly says that document uri can be nsnull.

And I'd be surprised if any did security checks based on document uri.
But I'll check.

> Haven't we previously made DOM created documents have the URI of creating
> document?
DOMParser does that. implementation.createDocument creates about:blank documents.

Comment 17

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

Attachment: safer

Comment 18

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

Comment on attachment 476199 [details] [diff] [review]
safer

>diff --git a/content/xml/document/src/nsXMLDocument.cpp b/content/xml/document/src/nsXMLDocument.cpp
>--- a/content/xml/document/src/nsXMLDocument.cpp
>+++ b/content/xml/document/src/nsXMLDocument.cpp
>@@ -116,19 +116,23 @@ NS_NewDOMDocument(nsIDOMDocument** aInst
>   nsresult rv;
> 
>   *aInstancePtrResult = nsnull;
> 
>   nsCOMPtr<nsIDocument> d;
>   PRBool isHTML = PR_FALSE;
>   PRBool isXHTML = PR_FALSE;
>   if (aDoctype) {
>-    nsAutoString publicId;
>+    nsAutoString publicId, name;
>     aDoctype->GetPublicId(publicId);
>-    if (publicId.EqualsLiteral("-//W3C//DTD HTML 4.01//EN") ||
>+    if (publicId.IsEmpty()) {
>+      aDoctype->GetName(name);
>+    }
>+    if (name.EqualsLiteral("html") ||
>+        publicId.EqualsLiteral("-//W3C//DTD HTML 4.01//EN") ||
>         publicId.EqualsLiteral("-//W3C//DTD HTML 4.01 Frameset//EN") ||
>         publicId.EqualsLiteral("-//W3C//DTD HTML 4.01 Transitional//EN") ||
>         publicId.EqualsLiteral("-//W3C//DTD HTML 4.0//EN") ||
>         publicId.EqualsLiteral("-//W3C//DTD HTML 4.0 Frameset//EN") ||
>         publicId.EqualsLiteral("-//W3C//DTD HTML 4.0 Transitional//EN")) {
>       rv = NS_NewHTMLDocument(getter_AddRefs(d));
>       isHTML = PR_TRUE;
>     } else if (publicId.EqualsLiteral("-//W3C//DTD XHTML 1.0 Strict//EN") ||

I don't understand these changes. Are they really related to this bug? Can you point to spec?

Looks good otherwise though.

Comment 19

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

HTML5 requires a specific kind of doctype for createHTMLDocument.
That is a part I don't change in ::CreateHTMLDocument, but I just make
NS_NewDOMDocument to recognize that doctype.

Comment 20

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

http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#dom-domhtmlimplementation-createhtmldocument

Comment 21

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

http://hg.mozilla.org/mozilla-central/rev/db645dab024f

Comment 22

[Eric Shepherd [:sheppy]]

This is now fully documented:

https://developer.mozilla.org/En/DOM/DOMImplementation.createHTMLDocument

With a working sample and linked to from the appropriate places.