URL property of document returned by XMLHttpRequest does not follow the spec

RESOLVED FIXED in mozilla31

Status

()

RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: xKhorasan, Assigned: xKhorasan)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla31
x86
Linux
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [engineering-friend])

Attachments

(1 attachment, 26 obsolete attachments)

41.84 KB, patch
xKhorasan
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 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
Note that the spec we implement is <http://xhr.spec.whatwg.org/>.
(Assignee)

Comment 2

6 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.
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...
Flags: needinfo?(annevk)

Comment 4

6 years ago
Can you explain the bogus security checks in more detail?

The concern I have is not breaking relative URLs.
Flags: needinfo?(annevk)
> 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...
Flags: needinfo?(annevk)

Comment 6

6 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.
Flags: needinfo?(annevk)
> 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...
(Assignee)

Comment 8

5 years ago
Created attachment 754120 [details] [diff] [review]
patch v1

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 )
Attachment #754120 - Flags: review?(bzbarsky)
I'd really like us to decide whether we're OK with this behavior change first of all: see comment 3 and comment 5.
Flags: needinfo?(jonas)
Comment on attachment 754120 [details] [diff] [review]
patch v1

Pending said decision....
Attachment #754120 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Blocks: 726433
(Assignee)

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 23 Branch → Trunk
(Assignee)

Comment 11

5 years ago
Created attachment 760236 [details]
XHR document's URL behavior test script

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.
(Assignee)

Updated

5 years ago
Attachment #760236 - Attachment mime type: application/octet-stream → application/javascript
It will affect chrome-privileged script that is looking at the result of an XHR call from content....
(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?
Flags: needinfo?(jonas)
> 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...
(Assignee)

Comment 15

5 years ago
Created attachment 769315 [details] [diff] [review]
patch v2

Thanks Boris and Jonas for the discussion.
Attachment #754120 - Attachment is obsolete: true
Attachment #769315 - Flags: review?(bzbarsky)
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?
Attachment #769315 - Flags: review?(bzbarsky) → review?(bugs)
Not really pretty but haven't figured out anything better...
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.
Attachment #769315 - Flags: review?(bugs) → review-
(Assignee)

Comment 19

5 years ago
Created attachment 775606 [details] [diff] [review]
patch v2.9 WIP
Attachment #769315 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 775997 [details] [diff] [review]
patch v3

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.
Attachment #775606 - Attachment is obsolete: true
Attachment #775997 - Flags: review?(bugs)
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...();
Attachment #775997 - Flags: review?(bugs) → review-
(Assignee)

Comment 22

5 years ago
Created attachment 779448 [details] [diff] [review]
bug-859095-fix-v4.patch

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.
Attachment #775997 - Attachment is obsolete: true
Attachment #779448 - Flags: review?(bugs)
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.
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)
Attachment #779448 - Flags: review?(bugs)
And don't hesitate to ping me on irc.
Assignee: nobody → xKhorasan+mozilla
(Assignee)

Comment 26

5 years ago
Created attachment 782778 [details] [diff] [review]
patch v5

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.
Attachment #779448 - Attachment is obsolete: true
Attachment #782778 - Flags: review?(bugs)
XHRs loaded by content don't have null principal. They have the principal of the page.
See Constructor in nsXMLHttpRequest.h
I still don't understand CheckMayLoad. Why can't we always set the chrome uris?
(Assignee)

Comment 29

5 years ago
Created attachment 788546 [details] [diff] [review]
sample patch that overwrite Chrome URIs in Reset() as document URIs does

(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.)

Updated

5 years ago
Whiteboard: [engineering-friend]
(Assignee)

Comment 31

5 years ago
Created attachment 790335 [details] [diff] [review]
patch v6, overwrite Chrome URIs in Reset call

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].
Attachment #782778 - Attachment is obsolete: true
Attachment #788546 - Attachment is obsolete: true
Attachment #790335 - Flags: review?(bugs)

Updated

5 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
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.
Attachment #790335 - Flags: review?(bugs) → review-
(Assignee)

Comment 33

5 years ago
Created attachment 795122 [details] [diff] [review]
WIP patch v6.1, add xml:base test

I overlooked the xml:base case.  Thanks for the catch.
Attachment #760236 - Attachment is obsolete: true
(Assignee)

Comment 34

5 years ago
Created attachment 795127 [details] [diff] [review]
patch v7, fix nsIContent::GetChromeXHRDocBaseURI for xml:base

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.
Attachment #790335 - Attachment is obsolete: true
Attachment #795122 - Attachment is obsolete: true
Attachment #795127 - Flags: review?(bugs)
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.
Attachment #795127 - Flags: review?(bugs) → review-
(Assignee)

Comment 36

5 years ago
Created attachment 798159 [details] [diff] [review]
WIP patch, add test for <base> element
Attachment #795127 - Attachment is obsolete: true
(Assignee)

Comment 37

5 years ago
Created attachment 798172 [details] [diff] [review]
WIP patch, fix chrome URLs for <base> element and clone
(Assignee)

Comment 38

5 years ago
Created attachment 798238 [details] [diff] [review]
patch v8

(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.
Attachment #798159 - Attachment is obsolete: true
Attachment #798172 - Attachment is obsolete: true
Attachment #798238 - Flags: review?(bugs)
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.
Attachment #798238 - Flags: review?(bugs) → review-
(Assignee)

Comment 40

5 years ago
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?

> 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)?
Attachment #798238 - Attachment is obsolete: true
Attachment #799816 - Flags: review?(bugs)
(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)?
> 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.
(Assignee)

Comment 43

5 years ago
Created attachment 801206 [details] [diff] [review]
patch v10

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)
Attachment #799816 - Attachment is obsolete: true
Attachment #801206 - Flags: review?(bugs)
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);
Attachment #801206 - Flags: review?(bugs) → review-
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.
(Assignee)

Comment 47

5 years ago
Created attachment 804768 [details] [diff] [review]
WIP for the review comment #44
Attachment #801206 - Attachment is obsolete: true
(Assignee)

Comment 48

5 years ago
Created attachment 804936 [details] [diff] [review]
bug-859095-fix-v11.patch

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
Attachment #804768 - Attachment is obsolete: true
Attachment #804936 - Flags: review?(bugs)
(Assignee)

Comment 49

5 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..
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
Attachment #804936 - Flags: review?(bugs) → review-
(Assignee)

Comment 51

5 years ago
Created attachment 808243 [details] [diff] [review]
WIP patch

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.
Attachment #804936 - Attachment is obsolete: true
(Assignee)

Comment 52

5 years ago
Created attachment 808360 [details] [diff] [review]
patch v12

Changes:
- fix .documentURIObject and .baseURIObject behavior, and add tests for these properties
Attachment #808243 - Attachment is obsolete: true
Attachment #808360 - Flags: review?(bugs)
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
Attachment #808360 - Flags: review?(bugs) → review-
(Assignee)

Comment 54

5 years ago
Created attachment 808608 [details] [diff] [review]
patch v13

(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.
Attachment #808360 - Attachment is obsolete: true
Attachment #808608 - Flags: review?(bugs)
(Assignee)

Comment 58

5 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.
(Assignee)

Comment 59

5 years ago
Created attachment 809471 [details] [diff] [review]
patch v14

patch for comment 58
Attachment #808608 - Attachment is obsolete: true
Attachment #809471 - Flags: review?(bugs)
Any chance for an interdiff. Bugzilla's interdiff seems to fail here.
(Assignee)

Comment 61

5 years ago
Created attachment 809474 [details] [diff] [review]
interdiff between v13 and v14

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

Comment 63

5 years ago
Created attachment 811755 [details]
try_ubuntu64_vm-b2g-emulator_test-mochitest-1-bm67-tests1-linux-build380.txt.gz

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.
(Assignee)

Comment 64

5 years ago
Created attachment 8397109 [details] [diff] [review]
just rebased patch v14 to current tip
Attachment #809471 - Attachment is obsolete: true
Attachment #809474 - Attachment is obsolete: true
Attachment #811755 - Attachment is obsolete: true
(Assignee)

Comment 65

5 years ago
Created attachment 8399035 [details] [diff] [review]
patch v15, fix test failure on B2G

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.
Attachment #8399035 - Flags: review?(bugs)
(Assignee)

Comment 66

5 years ago
Created attachment 8401932 [details] [diff] [review]
patch v15.1, add proper author name and title to patch v15

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.
Attachment #8397109 - Attachment is obsolete: true
Attachment #8399035 - Attachment is obsolete: true
Attachment #8401932 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(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!
(Assignee)

Comment 69

5 years ago
Thanks Carsten!  And thanks again Olli for reviewing patches that I attached so many times!
https://hg.mozilla.org/mozilla-central/rev/beef4eb44854
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.