:xKhorasan Assignee
	Description • 11 years ago

User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:23.0) Gecko/20130406 Firefox/23.0
Build ID: 20130406070945

According to XMLHttpRequest Working Draft, document response entity body has URL property that is set to request URL [1].
And request URL is set when:
1. the open() method is called [2]
2. the source origin and request URL are same origin, and the send() method faces redirect [3]
3. the send() method causes cross-origin request steps and faces redirect [4,5]

So, URL property of the document response entity body should works as "finalUrl" in GM_xmlhttpRequest [6].
However, URL property of the document response entity body is currently equal to location.href and not works as finalUrl.

[1] http://www.w3.org/TR/XMLHttpRequest/#document-response-entity-body (step 9)
[2] http://www.w3.org/TR/XMLHttpRequest/#the-open%28%29-method (step 20)
[3] http://www.w3.org/TR/XMLHttpRequest/#infrastructure-for-the-send%28%29-method (step 3 in "Otherwise")
[4] http://www.w3.org/TR/XMLHttpRequest/#the-send%28%29-method (step 10, "Otherwise")
[5] http://www.w3.org/TR/cors/#generic-cross-origin-request-algorithms (redirect step 2)
[6] https://github.com/scriptish/scriptish/wiki/GM_xmlhttpRequest#response-object


Steps to Reproduce:

1. Launch Firefox and open http://request-xkhorasan.rhcloud.com/finalurl


Actual Results:

w/o redirect:http://request-xkhorasan.rhcloud.com/finalurl
w/ redirect:http://request-xkhorasan.rhcloud.com/finalurl


Expected Results:

w/o redirect: http://response-xkhorasan.rhcloud.com/cors
w/ redirect: http://response-xkhorasan.rhcloud.com/cors

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 1 • 11 years ago

Note that the spec we implement is <http://xhr.spec.whatwg.org/>.

	:xKhorasan Assignee
	Comment 2 • 11 years ago

In <http://xhr.spec.whatwg.org/>,

* the document's URL is described in http://xhr.spec.whatwg.org/#document-response-entity-body (step 8, "Set document's URL to request URL").
* request URL in open() method is described in http://xhr.spec.whatwg.org/#the-open%28%29-method (step 14, "Set request URL to parsed URL").
* request URL in redirect step is described in http://xhr.spec.whatwg.org/#infrastructure-for-the-send%28%29-method (step 1, "Set the request URL to the URL conveyed by the Location header").
  This description covers same origin and cross origin request.

So following the spec <http://xhr.spec.whatwg.org/>, URL property of the document response entity body still should work as "finalUrl" in GM_xmlhttpRequest.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 11 years ago

Hmm.  The problem is things using document URIs for security checks (yes, they're broken, but they exist nevertheless).  Right now we're making sure that the loaded document has the same permissions as the document that loaded it even for such consumers but that wouldn't be the case with the spec's approach.

Note that we only do the URL overriding for cross-site loads, because those are the only ones where people might screw up by extracting an origin from the document URI...

	Anne (:annevk)
	Comment 4 • 11 years ago

Can you explain the bogus security checks in more detail?

The concern I have is not breaking relative URLs.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 11 years ago

> Can you explain the bogus security checks in more detail?

It's common for extensions to base security checks on document URIs, just because most extension authors don't think in terms of origins.  We had that problem in our UI code too, though I think we've mostly won that battle.

This is not least because there is no way in the web platform to get the origin of most objects, as it happens.  We do have Mozilla-specific properties for it on things like nodes, but extension developers may not be familiar with them.

> The concern I have is not breaking relative URLs.

When are those really an issue in data documents like XHR result documents?

But also, that can be solved via a base URI that does not match the document URI as needed...

	Anne (:annevk)
	Comment 6 • 11 years ago

The data can is perfectly capable of containing relative URLs, so yes. (In general we need something like xhr.responseURL I think for non-Document objects to use.)

Is it not a problem for those extensions if a document is purported to have a different URL than it actually has? E.g. if they want to change the markup of resources of a certain URL that would fail (not sure if that's possible with extensions though and whether that's a good idea to begin with).

But yes, we can hook into the document base URL algorithm if you think that is better. We can also introduce Document.origin.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 11 years ago

> than it actually has

Define "actually has"?

> E.g. if they want to change the markup of resources of a certain URL that would fail

Extensions that do this need to work on the network level, not the DOM level anyway...

A more likely scenario where extensions check document urls is an extension trying to avoid exposing data cross-site...

	:xKhorasan Assignee
	Comment 8 • 11 years ago

This patch set document's URL to request URL following the spec <http://xhr.spec.whatwg.org/>.
And also set document's baseURI to request URL for cross-site loads.

(In reply to Boris Zbarsky (:bz) from comment #3)
> Note that we only do the URL overriding for cross-site loads, because those are the only ones where people might screw up by extracting an origin from the document URI...
This behaviour was introduced in Bug 459470, so I fixed and added test case in content/base/test/test_XHRDocURI.html.

(In reply to Boris Zbarsky (:bz) from comment #3)
> Right now we're making sure that the loaded document has the same permissions as the document that loaded it even for such consumers

if the document loaded via non-system XHR, it has the same principal as the XHR has.
if the document loaded via system XHR, it has null principal.
( http://hg.mozilla.org/mozilla-central/file/512f5aab943e/content/base/src/nsXMLHttpRequest.cpp#l2024 )

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 11 years ago

I'd really like us to decide whether we're OK with this behavior change first of all: see comment 3 and comment 5.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 11 years ago

Comment on attachment 754120 [details] [diff] [review]
patch v1

Pending said decision....

	:xKhorasan Assignee
	Comment 11 • 11 years ago

I investigated XHR document's URL behavior for both chrome privilege script and web page script.

1. Open about:config and set "devtools.chrome.enabled" to true
2. Open http://request-xkhorasan.rhcloud.com/finalurl
3. Press Shift+F4 to launch Scratchpad
4. Paste attachment script to scratchpad, and select "Content" option in Environment menu
5. Press Ctrl+R to execute script
6. Select "Browser" option in Environment menu
7. Press Ctrl+R to execute script

User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:24.0) Gecko/20130605 Firefox/24.0
Build ID: 20130605121235


result:
# with "Content" Environment option
original URL = http://request-xkhorasan.rhcloud.com/test, document's URL = http://request-xkhorasan.rhcloud.com/test
original URL = http://request-xkhorasan.rhcloud.com/redirect?url=http://request-xkhorasan.rhcloud.com/test, document's URL = http://request-xkhorasan.rhcloud.com/test
original URL = http://response-xkhorasan.rhcloud.com/cors, document's URL = http://request-xkhorasan.rhcloud.com/finalurl
original URL = http://request-xkhorasan.rhcloud.com/redirect?url=http://response-xkhorasan.rhcloud.com/cors, document's URL = http://request-xkhorasan.rhcloud.com/finalurl

# with "Browser" Environment option
original URL = http://request-xkhorasan.rhcloud.com/test, document's URL = http://request-xkhorasan.rhcloud.com/test
original URL = http://request-xkhorasan.rhcloud.com/redirect?url=http://request-xkhorasan.rhcloud.com/test, document's URL = http://request-xkhorasan.rhcloud.com/test
original URL = http://response-xkhorasan.rhcloud.com/cors, document's URL = http://response-xkhorasan.rhcloud.com/cors
original URL = http://request-xkhorasan.rhcloud.com/redirect?url=http://response-xkhorasan.rhcloud.com/cors, document's URL = http://response-xkhorasan.rhcloud.com/cors


With Browser environment option, XHR document's URL behave as I described in comment 0 even if the request is cross-site request.
So it seems that the behavior change caused by this bug would only affect web page script, and not affect chrome privilege script.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 11 years ago

It will affect chrome-privileged script that is looking at the result of an XHR call from content....

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 13 • 11 years ago

(In reply to Anne (:annevk) from comment #6)
> The data can is perfectly capable of containing relative URLs, so yes. (In
> general we need something like xhr.responseURL I think for non-Document
> objects to use.)

Like Boris said, we can solve this problem by setting the .baseURI of the loaded document.

> Is it not a problem for those extensions if a document is purported to have
> a different URL than it actually has?

It definitely could be. But it's less likely that those problems are security problems.

> We can also introduce Document.origin.

I think we should do this. As well as Location.origin.

Though, what should Document.origin be in this case?


In general, I'm definitely worried about making the proposed change here. Too much Gecko code is still making security decisions based on URLs rather than nsIPrincipals or origins.

It sucks a lot that we can't follow the spec, since I do agree that the spec behavior is the more sane one.


Maybe we could use some magic that made web-page code see the correct URL, while chrome code would see the modified URL?

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 11 years ago

> Maybe we could use some magic that made web-page code see the correct URL, while chrome
> code would see the modified URL?

That's actually fairly doable...  It does mean a security check on every .URL, and storing an additional URI member on document, but that might be better than the alternatives...

	:xKhorasan Assignee
	Comment 15 • 11 years ago

Thanks Boris and Jonas for the discussion.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 11 years ago

Comment on attachment 769315 [details] [diff] [review]
patch v2

I'd like someone else to check over the idea here....

Olli, let me know if you'd rather not deal with this?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 11 years ago

Not really pretty but haven't figured out anything better...

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 18 • 11 years ago

Comment on attachment 769315 [details] [diff] [review]
patch v2

I think I'd prefer exposing the new URLs only to content JS.
At least for now.  Otherwise GetDocumentURI() call in C++
(and there are a lot) would depend on whatever JS happens to be running.

So the relevant URL getters would need a new method and there you could do the
IsCallerChrome() check. You'd need to add the new C++ methods to
http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Bindings.conf
as binarynames so that the right JS->C++ mapping happens.

	:xKhorasan Assignee
	Comment 20 • 11 years ago

Thanks Olli for the review and Bindings.conf information.

In this attached patch, I created new methods GetDocumentURIFromJS() on nsIDocument and GetBaseURIFromJS() on nsINode, and make new URIs are checked in those method.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 21 • 11 years ago

Comment on attachment 775997 [details] [diff] [review]
patch v3

Please add some comments to the new methods in nsIDocument.
nsIDocument's and nsINode's IID must be updated.

Why GetChromeXHRDocBaseURI is not with other XHR specific methods?

In nsIContent::GetChromeXHRDocBaseURI() 
just do OwnerDoc()->Get...();

	:xKhorasan Assignee
	Comment 22 • 11 years ago

Attached patch addressed the review commment #21.
GetChromeXHRDocBaseURI() declaration is now placed together with [GS]ChromeXHRDoc*URI methods in nsIDocument.

Also fixed GetChromeXHRDocURI() and GetChromeXHRDocBaseURI() so that the check in GetDocumentURIFromJS() and GetBaseURIFromJS() to be simple.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 23 • 11 years ago

Why do we need the null principal check in ChromeXHRDocURIIsNeeded()?
And why mPrincipal->CheckMayLoad? I think I'm missing something here.

I would like to have all the GetChrome* methods hidden from non-nsINode callers.
So, would it work if those were private or protected.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 24 • 11 years ago

Comment on attachment 779448 [details] [diff] [review]
bug-859095-fix-v4.patch

(clearing the r? for now. But please ask a review again once you've answered to the questions and/or uploaded a new patch)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 25 • 11 years ago

And don't hesitate to ping me on irc.

	:xKhorasan Assignee
	Comment 26 • 11 years ago

Thanks for your concern.

The attached patch changed all the GetChrome* methods to be protected.

(In reply to Olli Pettay [:smaug] from comment #23)
> Why do we need the null principal check in ChromeXHRDocURIIsNeeded()?
> And why mPrincipal->CheckMayLoad? I think I'm missing something here.

The mPrincipal->CheckMayLoad check restricts to set ChromeXHRDoc*URIs only when the XHR request got cross-origin redirect. And without mPrincipal->CheckMayLoad check, the test case in the attached patch fails, and this breaks compatibility.  So this check is needed.
>  // use content XHR and access URI properties from chrome privileged script
>  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>
>  xhr = new XMLHttpRequest;
>  xhr.open("GET", "http://mochi.test:8888/tests/content/base/test/file_XHRDocURI.xml");
>  xhr.onreadystatechange = function(e) {
>    if (!xhr.responseXML)
>      return;
>    is(xhr.responseXML.documentURI, "http://mochi.test:8888/tests/content/base/test/file_XHRDocURI.xml",
>       "wrong url");
>    is(xhr.responseXML.baseURI, "http://mochi.test:8888/tests/content/base/test/file_XHRDocURI.xml",
>       "wrong base");
>    if (xhr.readyState == 4) {
>      gen.next();
>    }
>  };
>  xhr.send();
>  yield;

result:
failed | wrong url - got http://mochi.test:8888/tests/content/base/test/test_XHRDocURI.html?autorun=1&closeWhenDone=1&consoleLevel=INFO&failureFile=/home/xkhorasan/program/mozilla-central/obj-i686-pc-linux-gnu/.mozbuild/mochitest_failures.json, expected http://mochi.test:8888/tests/content/base/test/file_XHRDocURI.xml
failed | wrong base - got http://example.org/, expected http://mochi.test:8888/tests/content/base/test/file_XHRDocURI.xml

I thought isNullPrincipal check is needed to check if the document is loaded via content script XHR, since following the comment #12, we need to return ChromeXHRDoc*URIs only when the document is loaded via content script XHR and document URIs are accessed from chrome privileged code.  However, without this check, all the test case in the attached patch passed.  So here removed ChromeXHRDocURIIsNeeded() and simply using nsContentUtils::IsCallerChrome() instead.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 27 • 11 years ago

XHRs loaded by content don't have null principal. They have the principal of the page.
See Constructor in nsXMLHttpRequest.h

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 28 • 11 years ago

I still don't understand CheckMayLoad. Why can't we always set the chrome uris?

	:xKhorasan Assignee
	Comment 29 • 11 years ago

(In reply to Olli Pettay [:smaug] from comment #28)
> Why can't we always set the chrome uris?

When the XHR request is same origin request, document URI and base URI passed to NS_NewDOMDocument (http://hg.mozilla.org/mozilla-central/file/6cd6960ea7f6/content/base/src/nsXMLHttpRequest.cpp#l2031) will be overwritten in Reset (http://hg.mozilla.org/mozilla-central/file/6cd6960ea7f6/content/base/src/nsDocument.cpp#l2052) which is called from StartDocumentLoad (http://hg.mozilla.org/mozilla-central/file/6cd6960ea7f6/content/base/src/nsXMLHttpRequest.cpp#l2055).
However, Chrome URIs are not overwritten in Reset, as document URIs does.  So if we set Chrome URIs for same origin request, Chrome URIs and document URIs would not be same, and this result is not what we need for same origin request.
Therefore, we should check if the XHR request is cross origin request, and should set Chrome URIs if it is true.

... Or is it better creating a new patch that overwrite Chrome URIs in Reset as document URIs does?  (attached sample patch.)

	:xKhorasan Assignee
	Comment 30 • 11 years ago

interdiff between patch v5 and sample patch:
https://bugzilla.mozilla.org/attachment.cgi?oldid=782778&action=interdiff&newid=788546&headers=1

	:xKhorasan Assignee
	Comment 31 • 11 years ago

I come to think that overwriting Chrome URIs in Reset is better than mChannel->checkMayLoad.

Attached patch is just rebased attachment 788546 [details] [diff] [review].

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 32 • 11 years ago

Comment on attachment 790335 [details] [diff] [review]
patch v6, overwrite Chrome URIs in Reset call

What if xml:base is used? baseURI for elements and such should use then the
right base uri, not the chrome uri from document.
So nsIContent::GetChromeXHRDocBaseURI() should somehow check whether the
base uri of the document has been overridden.

Sorry that base uri is such a messy thing.

	:xKhorasan Assignee
	Comment 33 • 11 years ago

I overlooked the xml:base case.  Thanks for the catch.

	:xKhorasan Assignee
	Comment 34 • 11 years ago

Attached patch fix nsIContent::GetChromeXHRDocBaseURI() to check if the document's baseURI and content's baseURI is equal, and if those baseURIs are not same, return content's baseURI since it is overridden by xml:base.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 35 • 11 years ago

Comment on attachment 795127 [details] [diff] [review]
patch v7, fix nsIContent::GetChromeXHRDocBaseURI for xml:base

So what if content base is the same as doc base, but chrome doc base is different? I mean the case when content has explicitly set base uri.

	:xKhorasan Assignee
	Comment 38 • 11 years ago

(In reply to Olli Pettay [:smaug] from comment #35)
> So what if content base is the same as doc base, but chrome doc base is
> different? I mean the case when content has explicitly set base uri.

In this case, .baseURI returned wrong value when using content XHR and accessing .baseURI from chrome priviledged script.  So the attached patch fixes to override mChromeXHRDocBaseURI when SetBaseURIUsingFirstBaseWithHref is called.

And also, .documentURI and .baseURI returned wrong value for cloned XHR document.  So the patch also fixes this problem, and adds test.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 39 • 11 years ago

Comment on attachment 798238 [details] [diff] [review]
patch v8

Almost there :)

Could you remove void GetChromeXHRDocBaseURI(nsAString& aURI)
and just inline its code to nsINode::GetBaseURIFromJS.

GetChromeXHRDocBaseURI(const nsINode* aNode) is also a bit odd.
The only case the null check it adds is needed is in Attr (OwnerDoc() never returns null), so you could just null check in Attr and remove that method.

	:xKhorasan Assignee
	Comment 40 • 11 years ago

(In reply to Olli Pettay [:smaug] from comment #39)
> Could you remove void GetChromeXHRDocBaseURI(nsAString& aURI)
> and just inline its code to nsINode::GetBaseURIFromJS.
Fixed.
Btw, we don't have to do the same thing on GetChromeXHRDocURI(nsString& aURI) and GetDocumentURIFromJS?

> GetChromeXHRDocBaseURI(const nsINode* aNode) is also a bit odd.
> The only case the null check it adds is needed is in Attr (OwnerDoc() never
> returns null), so you could just null check in Attr and remove that method.
Hmm, I think nsINode::GetChromeXHRDocBaseURI(const nsINode* aNode) is needed to call the protected GetChromeXHRDocBaseURI method out of the class.
For example, currently Attr::GetChromeXHRDocBaseURI need to call nsIContent::GetChromeXHRDocBaseURI out of the nsIContent class, and we cannot call nsIContent::GetChromeXHRDocBaseURI directly from nsIContent instance in the method in Attr class, since that method is protected.
Would it better removing GetChromeXHRDocBaseURI(const nsINode* aNode) and declaring Attr as friend class in nsIContent (and nsIContent in nsIDocument)?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 41 • 11 years ago

(In reply to :xKhorasan from comment #40)
> Created attachment 799816 [details] [diff] [review]
> patch v9, remove GetChromeXHRDocBaseURI(nsAString &aURI)
> 
> (In reply to Olli Pettay [:smaug] from comment #39)
> > Could you remove void GetChromeXHRDocBaseURI(nsAString& aURI)
> > and just inline its code to nsINode::GetBaseURIFromJS.
> Fixed.
> Btw, we don't have to do the same thing on GetChromeXHRDocURI(nsString&
> aURI) and GetDocumentURIFromJS?
Well, the latter is called from Webidl code, and the first one...indeed, it is just called
one so it should be inlined.


> 
> > GetChromeXHRDocBaseURI(const nsINode* aNode) is also a bit odd.
> > The only case the null check it adds is needed is in Attr (OwnerDoc() never
> > returns null), so you could just null check in Attr and remove that method.
> Hmm, I think nsINode::GetChromeXHRDocBaseURI(const nsINode* aNode) is needed
> to call the protected GetChromeXHRDocBaseURI method out of the class.
> For example, currently Attr::GetChromeXHRDocBaseURI need to call
> nsIContent::GetChromeXHRDocBaseURI out of the nsIContent class, and we
> cannot call nsIContent::GetChromeXHRDocBaseURI directly from nsIContent
> instance in the method in Attr class, since that method is protected.
> Would it better removing GetChromeXHRDocBaseURI(const nsINode* aNode) and
> declaring Attr as friend class in nsIContent (and nsIContent in nsIDocument)?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 42 • 11 years ago

> Would it better removing GetChromeXHRDocBaseURI(const nsINode* aNode) and
> declaring Attr as friend class in nsIContent (and nsIContent in nsIDocument)?
yes, if that friend class is needed.

	:xKhorasan Assignee
	Comment 43 • 11 years ago

Thanks Olli for the comment.

Changes are:
- removed GetChromeXHRDocURI(nsString& aURI) and inline its code to GetDocumentURIFromJS
- declared Attr as friend class of nsIContent, and nsIContent as friend class of nsIDocument (comment #40)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 44 • 11 years ago

Comment on attachment 801206 [details] [diff] [review]
patch v10

>+  // Return the URI for the document.
>+  // The returned value may differ if the document is loaded via XHR, and
>+  // when accessed from chrome privileged script and
>+  // from content privileged script for compatibility.
>+  void GetDocumentURIFromJS(nsString& retval) const;
Nit, nsString& retval (for some odd reason other methods around this use wrong coding style, but don't change them)


>+nsIContent::GetChromeXHRDocBaseURI() const
>+{
>+  nsIDocument* doc = OwnerDoc();
>+  nsCOMPtr<nsIURI> docBase = doc->GetDocBaseURI();
>+  nsCOMPtr<nsIURI> contentBase = GetBaseURI();
>+  if (!docBase) {
>+    return doc->GetChromeXHRDocBaseURI();
>+  }
>+  bool same;
>+  nsresult rv = docBase->Equals(contentBase, &same);
>+  if (NS_SUCCEEDED(rv) && !same) {
>+    return contentBase.forget();
>+  }
But what if the URIs are the same and there is explicit base uri set for some element.
Then we want to return contentBase

Could we add some optional bool parameter to GetBaseURI
to check XHRDoc base uri at the end.

something like
nsIContent::GetBaseURI(bool aTryUseXHRDocBaseURI = false)
then in nsIContent::GetBaseURI use either GetDocBaseURI or GetChromeXHRDocBaseURI as default

>+nsIDocument::GetDocumentURIFromJS(nsString& aDocumentURI) const
>+{
>+  if (!nsContentUtils::IsCallerChrome()) {
>+    return GetDocumentURI(aDocumentURI);
>+  }
if (!mChromeXHRDocURI || !nsContentUtils::IsCallerChrome()) {
  return GetDocumentURI(aDocumentURI);
}
nsAutoCString uri;
mChromeXHRDocURI->GetSpec(uri);
CopyUTF8toUTF16(uri, aDocumentURI);
>+nsINode::GetBaseURIFromJS(nsAString& aURI) const
>+{
>+  if (!nsContentUtils::IsCallerChrome()) {
>+    return GetBaseURI(aURI);
>+  }

And if you'd made the change to GetBaseURI to have bool parameter
this method could be just 
nsCOMPtr<nsIURI> baseURI = GetBaseURI(nsContentUtils::IsCallerChrome());
nsAutoCString spec;
if (baseURI) {
  baseURI->GetSpec(spec);
}
CopyUTF8toUTF16(spec, aURI);

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 45 • 11 years ago

And now, fun parts of the web platform - it is evolving all the time.
See bug 903372. If that happens, this bug doesn't need to deal with baseuri.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 46 • 11 years ago

...maybe. Depends on what all is removed.

	:xKhorasan Assignee
	Comment 48 • 11 years ago

Thanks for the review!

Changes are:
- fix retval to aDocumentURI in GetDocumentURIFromJS declaration
- add test for the xhr document whose document baseURI and element baseURI are same.
- add optionnal parameter to GetBaseURI.
- also fix GetDocumentURIFromJS and GetBaseURIFromJS

	:xKhorasan Assignee
	Comment 49 • 11 years ago

Bug 903372 looks good news for us, but I'm not sure whether we should set bug 903372 in the "Depends on:" field of this bug..

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 50 • 11 years ago

Comment on attachment 804936 [details] [diff] [review]
bug-859095-fix-v11.patch

update IID of nsIContent


>+function testChromeHTMLDocURI(aDoc, aNonChromeBaseURI) {
>+  aDoc.body.setAttributeNS("http://www.w3.org/XML/1998/namespace", "base", aNonChromeBaseURI);
>+  is(aDoc.body.baseURI, aNonChromeBaseURI,
>+     "wrong base (doc base and xml:base are same)");
>+  var attr = aDoc.getElementById("data").getAttributeNode("id");
>+  is(attr.baseURI, aNonChromeBaseURI,
>+     "wrong attr base (doc base and xml:base are same)")
>+}
Could you check also doc uri here.

>+  // use content XHR and access URI properties from chrome privileged script
>+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
Please don't add more enablePrivilege usage. Use SpecialPowers (it has IIRC 'wrap' method to access stuff from chrome js.)


Almost r+, but I'd like to see tests fixed to not use enablePrivilege

	:xKhorasan Assignee
	Comment 51 • 11 years ago

Changes:
- update IID in nsIContent.h
- add check for document URIs in testCHromeHTMLDocURI()
- remove enablePrivilege and use SpecialPowers.wrap

But I noticed that there is .documentURIObject and baseURIObject,
and the current patch does not change their behavior as we did for .documentURI and .baseURI.
So I'll make a new patch for .documentURIObject and baseURIObject.

	:xKhorasan Assignee
	Comment 52 • 11 years ago

Changes:
- fix .documentURIObject and .baseURIObject behavior, and add tests for these properties

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 53 • 11 years ago

Comment on attachment 808360 [details] [diff] [review]
patch v12

>+nsIURI*
>+nsIDocument::GetDocumentURIObject() const
>+{
>+  if (!mChromeXHRDocURI || !nsContentUtils::IsCallerChrome()) {
>+    return GetDocumentURI();
>+  }
documentURIObject in .webidl is ChromeOnly so drop IsCallerChrome here.
Same with baseURIObject

	:xKhorasan Assignee
	Comment 54 • 11 years ago

(In reply to Olli Pettay [:smaug] from comment #53)
> Comment on attachment 808360 [details] [diff] [review]
> patch v12
> 
> >+nsIURI*
> >+nsIDocument::GetDocumentURIObject() const
> >+{
> >+  if (!mChromeXHRDocURI || !nsContentUtils::IsCallerChrome()) {
> >+    return GetDocumentURI();
> >+  }
> documentURIObject in .webidl is ChromeOnly so drop IsCallerChrome here.
> Same with baseURIObject

Thanks, fixed.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 55 • 11 years ago

Comment on attachment 808608 [details] [diff] [review]
patch v13

Pushed to try to see whether tests pass everywhere
https://tbpl.mozilla.org/?tree=Try&rev=2d208926b321

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 56 • 11 years ago

Bah, pushed from wrong tree.
https://tbpl.mozilla.org/?tree=Try&rev=d4f76e5564a3 should be better.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 57 • 11 years ago

But doesn't look good https://tbpl.mozilla.org/php/getParsedLog.php?id=28234491&tree=Try&full=1#error0

	:xKhorasan Assignee
	Comment 58 • 11 years ago

Thanks for pushing to Try server, and sorry for the build failure.

nsIDocument::GetChromeXHRDocBaseURI declaration did not cause build failure on gcc 4.6.3,  but on Try server that declaration does causes build failure.
nsIDocument::GetChromeXHRDocBaseURI is only called from nsIDocument::GetBaseURI, so I will create a new patch to get rid of nsIDocument::GetChromeXHRDocBaseURI and inline its code into nsIDocument::GetBaseURI.

	:xKhorasan Assignee
	Comment 59 • 11 years ago

patch for comment 58

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 60 • 11 years ago

Any chance for an interdiff. Bugzilla's interdiff seems to fail here.

	:xKhorasan Assignee
	Comment 61 • 11 years ago

Sure, here is interdiff for attachment 808608 [details] [diff] [review] and attachment 809471 [details] [diff] [review] .

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 62 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=359f27ce6e00

	:xKhorasan Assignee
	Comment 63 • 11 years ago

Almost all the test failures seem not related to attachment 809471 [details] [diff] [review], but on B2G the test in attachment 809471 [details] [diff] [review] failed:

> 17:46:16     INFO -  2415 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_XHRDocURI.html | uncaught exception - Error: Permission denied to access property 'toString' at :0
> 17:46:16     INFO -  JavaScript error: , line 0: Permission denied to access property 'toString'
> 17:46:16     INFO -  2416 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_XHRDocURI.html | uncaught exception - Error: Permission denied to access property 'message' at :0
> 17:46:22     INFO -  JavaScript error: , line 0: Permission denied to access property 'message'
> 17:46:22     INFO -  2417 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_XHRDocURI.html | uncaught exception - uncaught exception: unknown (can't convert to string) at :0
> 17:46:22     INFO -  JavaScript error: , line 0: uncaught exception: unknown (can't convert to string)
> 17:46:22     INFO -  2418 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_XHRDocURI.html | [SimpleTest.finish()] this test already called finish!
> 17:46:22     INFO -  2419 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_XHRDocURI.html | called finish() multiple times
> 17:46:22     INFO -  2420 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_XHRDocURI.html | [SimpleTest.finish()] this test already called finish!
> 17:46:22     INFO -  2421 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_XHRDocURI.html | called finish() multiple times
> 17:46:22     INFO -  2422 INFO TEST-END | /tests/content/base/test/test_XHRDocURI.html | finished in 4965ms

From the build log, test failed at:

>  // use chrome XHR and access URI properties from chrome privileged script
>  xhr = SpecialPowers.createSystemXHR();
>  xhr.open("GET", "http://mochi.test:8888/tests/content/base/test/file_XHRDocURI.xml");
>  xhr.onreadystatechange = function(e) {
>    if (!xhr.responseXML) {
>      return;
>    }
>    var expects = {
>      documentURI: "http://mochi.test:8888/tests/content/base/test/file_XHRDocURI.xml",
>      baseURI: "http://mochi.test:8888/tests/content/base/test/file_XHRDocURI.xml",
>      elementBaseURI: "http://www.example.com/"
>    };
>    var xml = SpecialPowers.wrap(xhr.responseXML);
>    testChromeXMLDocURI(xml, expects);
>    if (xhr.readyState == 4) {
>      gen.next();
>    }
>  };

I'm not sure what caused this test failure, so I should investigate this.

	:xKhorasan Assignee
	Comment 65 • 10 years ago

Sorry for taking too long time to set up 64bit environment to build B2G, and to fix broken test for this bug on B2G.

After I rebased attachment 809471 [details] [diff] [review] (patch v14) to current tip, I could not reproduce the test failure described in comment 63, but the test failed since SpecialPowers.createSystemXHR() has removed for bug 901343.
So I fixed the patch only for its test as following:
- call SpecialPowers.addPermission() before starting tests that use System XHR, and call SpecialPowers.removePermission() after all of these tests finished.
- replace SpecialPowers.createSystemXHR() to new XMLHttpRequest({mozAnon: false, mozSystem: true})

With this patch (v15), I can run its test with no failure for both mochitest-plain and mochitest-remote.

	:xKhorasan Assignee
	Comment 66 • 10 years ago

Thanks for Olli, the reviewed patch attachment 8399035 [details] [diff] [review] (v15) was pushed to the try server.
https://tbpl.mozilla.org/?tree=Try&rev=8e1a8cd9de1d

I noticed that the patch v15 does not contains author information and proper title.
So I created patch v15.1, only add author name and title to the patch v15.

	Carsten Book [:Tomcat]
	Comment 67 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/beef4eb44854

	Carsten Book [:Tomcat]
	Comment 68 • 10 years ago

(In reply to Carsten Book [:Tomcat] from comment #67)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/beef4eb44854

btw thanks for the contribution to mozilla and congrats to your first patch!

	:xKhorasan Assignee
	Comment 69 • 10 years ago

Thanks Carsten!  And thanks again Olli for reviewing patches that I attached so many times!

	Ryan VanderMeulen [:RyanVM]
	Comment 70 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/beef4eb44854