467123 - document.cloneNode() failed in a JS component

Status: RESOLVED FIXED

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

Description

[Sylvain Spinelli]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081128 Minefield/3.1b3pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081128 Minefield/3.1b3pre

// In a JS component, I load a DOM (from chrome://)
var vXhrComp = Components.classes["@mozilla.org/xmlextras/
xmlhttprequest;1"];
var vReq = vXhrComp.createInstance
(Components.interfaces.nsIXMLHttpRequest);
vReq.open("GET", "chrome://mychrome/content/myFile.xml", false);
vReq.send(null);
var vXml = vReq.responseXML;

//I want to "backup" my dom
var vXmlBackup = vXml.cloneNode(true);

The cloneNode failed : Security Error: Content at moz-nullprincipal:
{c0db3ee1-d7c3-47e4-b6d9-0a08830f15c9} may not load or link to //
chrome://mychrome/content/myFile.xml.

I think it's a problem with transfering nsIPrincipal to the new
Document.

In a xul page in chrome, this code works fine. 

Reproducible: Always

Steps to Reproduce:
1. Install the xpi (see attached testcase)
2. In JSconsole write : window.openDialog("chrome://bugclonenode/content/", "bugclonenode", "chrome, resizable");
3. Click on the button : XHR load document : it's ok: 
4. Click a second time : call the cloneNode method on the already loaded Document : nok



It works in firefox 3.0.4, but the security warning is published (in the JsConsole).

In firefox 3.1 (and xulrunner 1.9.1), the cloneNode() throw an exception.

Comment 1

[Sylvain Spinelli]

Attachment: testcase

xpi test case (firefox or xulrunner).

Comment 2

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

ok, so this is because bug 42976 added NS_ENSURE_SUCCESS(rv, rv) after
rv = clone->SetBaseURI(...);
If that check is removed, 1.9.1 behaves like 1.9.0 - security error
is printed to error console but the actual cloning does work.

Not sure what would be the most reasonable behavior here.

Comment 3

[Eli Friedman]

IMO, we should just change the "rv = clone->SetBaseURI(nsIDocument::GetBaseURI());" to "clone->mDocumentBaseURI = mDocumentBaseURI;"; someone more familiar with the security properties of principals and base URIs should really take a look at this, though.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  Yeah, that should work.  If the original document was allowed to have this base URI, this one should too.  Note that there are places where we SetBaseURI and _then_ set to a powerless principal for various reasons, and XHR is one of those places.

Also note that this consumer is using XHR in "backwards compat" mode (without an explicit init() call), so some things will just be broken.  This is one of them.  I do still think we should make this work, though.

Smaug, want to patch?

Comment 5

[Sylvain Spinelli]

I make a mistake :
> In a xul page in chrome, this code works fine. 

In fact, it only works in a chrome xul page if the document is loaded from :

var vXml = document.implementation.createDocument("", "", null);
vXml.async = false;
vXml.load(pUrl);

But in a JS component there is no "document" object.

I don't understand how I can use XHR.init() in a JS component since this method is [noscript].

Comment 6

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

(In reply to comment #4)
> Also note that this consumer is using XHR in "backwards compat" mode (without
> an explicit init() call), so some things will just be broken.  This is one of
> them.
This bug has nothing to do with Init(), IMO. If chrome principal is used with
XHR, XHR explicitly sets the document principal to be null principal (bug 421228).
Calling Init() wouldn't change that situation.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Gah.  I keep forgetting that the DOMParser init() stuff was ported to XHR in a broken way.  We should fix that.  :( And you shouldn't be initing with the system principal, imo.

But yes, we should just do what comment 3 says.

Comment 8

[Boris Zbarsky [:bzbarsky]]

This is a regression from 1.9.0, and would adversely affect extensions...  I really think we should fix this.

I can promise speedy reviews.

Comment 9

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

And the error in the error console is regression from 1.8, I think.
I'll make the patch (comment 3) and I guess I should try to write a test too.

Comment 10

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

Attachment: proposed patch + test

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 351367 [details] [diff] [review]
proposed patch + test

r+sr=bzbarsky

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 351367 [details] [diff] [review]
proposed patch + test

a191=beltzner, should also be sure to consider this for blocking as it's a regression that can mess up add-ons

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

(cc'ing team add-ons so that they can add this to the known incompatibilities with beta 2 for add-on developers ...)

Comment 14

[Benjamin Smedberg]

mozilla-central commit was http://hg.mozilla.org/mozilla-central/rev/10b0ac3fde93

This never made it to 1.9.1? P1 because this can affect addon compat.

Comment 15

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/39823d237084