Closed Bug 467123 Opened 13 years ago Closed 13 years ago

document.cloneNode() failed in a JS component

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: sylvain.spinelli, Assigned: smaug)

References

Details

(Keywords: fixed1.9.1, regression)

Attachments

(2 files)

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.
Attached file testcase
xpi test case (firefox or xulrunner).
Component: General → DOM
Product: Firefox → Core
QA Contact: general → general
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.
Blocks: 42976
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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?
Keywords: regression
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].
(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.
Blocks: 421228
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.
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.
Flags: blocking1.9.1?
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.
Assignee: nobody → Olli.Pettay
Attachment #351367 - Flags: superreview?(bzbarsky)
Attachment #351367 - Flags: review?(bzbarsky)
Attachment #351367 - Flags: superreview?(bzbarsky)
Attachment #351367 - Flags: superreview+
Attachment #351367 - Flags: review?(bzbarsky)
Attachment #351367 - Flags: review+
Comment on attachment 351367 [details] [diff] [review]
proposed patch + test

r+sr=bzbarsky
Attachment #351367 - Flags: approval1.9.1?
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: wanted1.9.0.x?
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
Attachment #351367 - Flags: approval1.9.1? → approval1.9.1+
(cc'ing team add-ons so that they can add this to the known incompatibilities with beta 2 for add-on developers ...)
Flags: wanted1.9.0.x?
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.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
Summary: Document.cloneNode() failed in a JS component → document.cloneNode() failed in a JS component
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.