Last Comment Bug 460460 - importNode() and adoptNode() fail when node comes from XHR created in a JS component
: importNode() and adoptNode() fail when node comes from XHR created in a JS co...
Status: NEW
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on:
Blocks: 456271
  Show dependency treegraph
 
Reported: 2008-10-17 08:36 PDT by Fabrice Desré
Modified: 2010-09-09 09:37 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Fabrice Desré 2008-10-17 08:36:56 PDT
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é 2008-10-17 08:44:01 PDT
Note that this also fails in any chrome script in an extension, not only in a JS XPCOM component.
Comment 2 Olli Pettay [:smaug] (reviewing overload) 2008-10-17 09:03:19 PDT
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] (reviewing overload) 2008-10-17 09:04:13 PDT
Does it fail if you create XHR using "new XMLHttpRequest()"?
Comment 4 Olli Pettay [:smaug] (reviewing overload) 2008-10-17 09:10:19 PDT
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 [:bz] (still a bit busy) 2008-10-17 09:10:28 PDT
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] (reviewing overload) 2008-10-17 09:15:43 PDT
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 [:bz] (still a bit busy) 2008-10-17 09:16:56 PDT
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] (reviewing overload) 2008-10-17 09:17:17 PDT
(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] (reviewing overload) 2008-10-17 09:18:11 PDT
(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] (reviewing overload) 2008-10-17 09:23:35 PDT
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 [:bz] (still a bit busy) 2008-10-17 09:29:58 PDT
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 [:bz] (still a bit busy) 2008-10-17 09:30:32 PDT
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 [:bz] (still a bit busy) 2008-10-17 09:31:04 PDT
Oh, and comment 8 is a non-starter precisely because JS components can't use |new XMLHttpRequest()|.
Comment 14 carl@zeevee.com 2009-06-09 06:55:00 PDT
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 [:bz] (still a bit busy) 2009-06-09 07:50:59 PDT
> FWIW this is a regression between firefox 3.0.x and 3.5.

Yes, we know.  See comment 2.
Comment 16 Eric Shepherd [:sheppy] 2010-09-09 07:03:11 PDT
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 [:bz] (still a bit busy) 2010-09-09 09:08:01 PDT
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] (reviewing overload) 2010-09-09 09:37:25 PDT
Nothing. Bug 594767 was filed for comment 16.

Note You need to log in before you can comment on or make changes to this bug.