460460 - importNode() and adoptNode() fail when node comes from XHR created in a JS component

Status: NEW

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

Description

[Fabrice Desré]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1) Gecko/20081007 Firefox/3.1b1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081015

It is not possible anymore to import or adopt nodes from an XML document retrieved using XHR into a doc created by implementation.createDocument().

The error returned is NS_UNEXPECTED_ERROR

Reproducible: Always

Steps to Reproduce:
1. Create a document using doc = CC["@mozilla.org/xml/xml-document;1"].createInstance(CI.nsIDOMDocument).implementation.createDocument("", "root", null);
2. Retrieve an XML doc using XHR
3. try node = doc.importNode(xhr.responseXML.documentElement)
Actual Results:  
Fails with NS_UNEXPECTED_ERROR

Expected Results:  
This should work, and was working before

Comment 1

[Fabrice Desré]

Note that this also fails in any chrome script in an extension, not only in a JS XPCOM component.

Comment 2

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

Bug 456271 prevented adopting/importing if (1) the new doc doesn't have
script handling object (which is the case here) and (2) the original document 
isn't a chrome document and (3) it does have a script handling object.

On IRC you mentioned that you create XHR using .createInstance().
That tries to get the |window| to which it is bound to from stack.
Perhaps that is the problem here. Maybe it finds non-chrome window.

What causes XHR to be created?

Comment 3

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

Does it fail if you create XHR using "new XMLHttpRequest()"?

Comment 4

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

The best thing you could do is to create document using
document.implementation.createDocument("", "", null);
and XHR using new XMLHttpRequest();

Perhaps you could pass chrome |window| to your JS component
and call |new chromeWindow.XMLHttpRequest()| and |chromeWindow.document.implementation.createDocument("", "", null)| ?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Er... why does the XHR document have a script handling object?  Or alternately, why does the createDocument()-created object not have one?  I would think they would behave identically in this respect...

I should note that this code's use of a contract to create XML documents is going to become unsupported in the very near future, but that's not really important to this bug report.

Comment 6

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

CC["@mozilla.org/xml/xml-document;1"].createInstance(CI.nsIDOMDocument).implementation.createDocument("",
"root", null);
Doesn't set any script handling object because the original created document doesn't have one.
(I don't know why to create 2 documents, first a temporary with .createInstance and then with .createDocument)

Comment 7

[Boris Zbarsky [:bzbarsky]]

I guess the reason the createDocument() doc doesn't have a script handling object in this case is in fact due to the use of the contract id thing, huh?

Getting the window from the stack sounds like a bad-and-frightening thing to me.... :(

Also, does DOMParser have similar issues if created via contract?

This whole script handling object thing needs to go.  It's basically incompatible with sane use in JS components, if it requires everything to have a DOMWindow... :(

Comment 8

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

(In reply to comment #5)
> Er... why does the XHR document have a script handling object? 
We could perhaps remove the Init thing, which tries to get the script object.
C++ should always call Init explicitly and JS should use new XMLHttpRequest
syntax.

Comment 9

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

(In reply to comment #7)
> This whole script handling object thing needs to go.  It's basically
> incompatible with sane use in JS components, if it requires everything to have
> a DOMWindow... :(
I know. The problem is in XPConnect.

Comment 10

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

The problem to fix is this http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/xpccallcontext.cpp&rev=1.25&mark=95-103#88

Comment 11

[Boris Zbarsky [:bzbarsky]]

I should note that for DOMParser we expose a JS-callable init() to be used from JS components, but the hack job of adding init() to XMLHttpRequest didn't copy that over.  I should also note that the JS-callable DOMParser init() doesn't take a script global object.  Should it?  Again, the goal is to be able to use all this stuff in a sane way in a JS component.

As for the XPConnect thing...  What that really means is that XHR and so forth need a JSContext, right?  They don't actually need a script global, as far as I can tell.

Comment 12

[Boris Zbarsky [:bzbarsky]]

In particular, I would think that a JS component has a sane JSContext, but I'm not sure it would outlive the initial execution of the component's script.  :(

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, and comment 8 is a non-starter precisely because JS components can't use |new XMLHttpRequest()|.

Comment 14

[[e-mail]]

FWIW this is a regression between firefox 3.0.x and 3.5.  TBH from the comments I can't gleam exactly what is supposed to be the correct behavior.  This is what used to work in older version of mozilla:

var xmlNode = someXHRXMLNode; // recv from a XMLHTTPRequest
var doc = Cc["@mozilla.org/xml/xml-document;1"].createInstance(Ci.nsIDOMDocument);
// more code to create document
// this line blows up with NS_UNEXPECTED_ERROR
someNodeInNewDoc.appendChild(doc.importNode(xmlNode,true));


The solution (as hinted at above) is to make sure both documents are created using the same implementation.  Luckily from any DOM node you can can get back to the implementation.  What worked for me:


var doc = xmlNode.ownerDocument.implementation.createDocument(null,"yourroottag",null);
doc.documentElement.appendChild(doc.importNode(xmlNode,true));

Comment 15

[Boris Zbarsky [:bzbarsky]]

> FWIW this is a regression between firefox 3.0.x and 3.5.

Yes, we know.  See comment 2.

Comment 16

[Eric Shepherd [:sheppy]]

This is an issue if you use document.implementation.createHTMLDocument() to create an HTML document on the fly, then attempt to insert it into an iframe as well.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Er... what does comment 16 have to do with this bug?  You can't insert dynamically created documents into iframes.  Or do you mean that you're inserting nodes from that document?

Comment 18

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

Nothing. Bug 594767 was filed for comment 16.

Comment 19

[Thomas Wisniewski]

This code works fine for me in the browser console, so I take it this has since been fixed?

>doc = Cc["@mozilla.org/xml/xml-document;1"].createInstance(Ci.nsIDOMDocument).implementation.createDocument("",
"root", null);
>xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
>xhr.responseType = "document";
>xhr.open("get", "http://google.com/", true);
>xhr.send();
>node = doc.importNode(xhr.response.documentElement);
>doc.childNodes[0].appendChild(node);

Comment 20

[Boris Zbarsky [:bzbarsky]]

The link in comment 10 is dead because we kill things without creating redirects.  The code to throw NS_ERROR_UNEXPECTED if adopting from a doc with a script handling object into one without is still there in nsNodeUtils::CloneAndAdopt.

I think the big difference is that in the meantime we fixed bug 756277, which makes the failure mode described in comment 2 not happen anymore...