Closed Bug 998076 Opened 6 years ago Closed 6 years ago

implement responseURL attribute for XMLHttpRequest

Categories

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

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: xKhorasan, Assigned: xKhorasan)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 9 obsolete files)

User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0
Build ID: 20140417030203

WHATWG XMLHttpRequest (XHR) spec says that there is `responseURL` attribute that returns serialized response's url [1].
However, Firefox does not have such attribute for XHR currently.

Steps to reproduce:
Launch Firefox and open http://request-xkhorasan.rhcloud.com/responseURL

Actual Result:
'responseURL' in (new XMLHttpRequest()): false
responseURL for http://request-xkhorasan.rhcloud.com/test: undefined
responseURL for http://request-xkhorasan.rhcloud.com/redirect?url=http://request-xkhorasan.rhcloud.com/test: undefined
responseURL for http://response-xkhorasan.rhcloud.com/cors: undefined
responseURL for http://request-xkhorasan.rhcloud.com/redirect?url=http://response-xkhorasan.rhcloud.com/cors: undefined

Expected result:
'responseURL' in (new XMLHttpRequest()): true
responseURL for http://request-xkhorasan.rhcloud.com/test: http://request-xkhorasan.rhcloud.com/test
responseURL for http://request-xkhorasan.rhcloud.com/redirect?url=http://request-xkhorasan.rhcloud.com/test: http://request-xkhorasan.rhcloud.com/test
responseURL for http://response-xkhorasan.rhcloud.com/cors: http://response-xkhorasan.rhcloud.com/cors
responseURL for http://request-xkhorasan.rhcloud.com/redirect?url=http://response-xkhorasan.rhcloud.com/cors: http://response-xkhorasan.rhcloud.com/cors

[1] http://xhr.spec.whatwg.org/#dom-xmlhttprequest-responseurl
Blocks: xhr
Attached patch patch v1 (obsolete) — Splinter Review
Attached patch implements `responseURL` for both JavaScript global environment: document environment and worker environment.
The tests in the patch for document environment (in content/base/test) and for worker environment (in dom/workers/test) use same script content/base/test/test_XHRResponseURL.js.  XHR timeout tests do similar things.

In worker environment, `resposneURL` test for cross origin redirect fails, but it seems that the failure is caused by bug 886235.
So in the patch, test for cross origin redirect is skipped if JavaScript global environment is worker environment.

I ran mochitest-plain for content/base and dom/workers, and there was no test failure with the patch.
Attachment #8409428 - Attachment is obsolete: true
Attachment #8409487 - Flags: review?(bugs)
Comment on attachment 8409487 [details] [diff] [review]
patch v1

Let's not add new properties to the .idl interface, only to .webidl

And why do we need already_AddRefed<nsIURI> GetResponseURL() ?
Just have GetResponseURL(nsAString& aUrl)
Attachment #8409487 - Flags: review?(bugs) → review-
Attached patch patch v2, fix for review comment (obsolete) — Splinter Review
fixed:
- remove `responseURL` from .idl
- remove already_Addrefed<nsIURI> GetResponseURL(), and inline its code into GetResponseURL(nsAString& aUrl)
Assignee: nobody → xKhorasan+mozilla
Attachment #8409487 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8410601 - Flags: review?(bugs)
>-[scriptable, uuid(2e91e088-e9fa-4ba4-9887-2a0b7cf27a3e)]
>+[scriptable, uuid(b1b43854-62a5-4a40-841c-341d942bff69)]

You don't have to bump the iid anymore.
Thanks Kimura-san for pointing out.

fixed:
- remove iid change from .idl file
- change inconsistent variable name (from responseURL to responseUrl)
Attachment #8410601 - Attachment is obsolete: true
Attachment #8410601 - Flags: review?(bugs)
Attachment #8411073 - Flags: review?(bugs)
Comment on attachment 8411073 [details] [diff] [review]
patch v3, fix for review comments

> 
>+/* readonly attribute AString responseURL; */
>+void
>+nsXMLHttpRequest::GetResponseURL(nsAString& aUrl)
>+{
>+  aUrl.Truncate();
>+  nsCOMPtr<nsIHttpChannel> httpChannel = GetCurrentHttpChannel();
>+
>+  if (!httpChannel) {
>+    return;
>+  }
Why we support responceURL only with http(s) channels?
The spec doesn't have such limitation.


Other channels should be supported, and could we even have tests for data: or blob urls?
Attachment #8411073 - Flags: review?(bugs) → review-
Attachment #8411073 - Attachment is obsolete: true
Thanks Olli for the review.

fixed:
- fix to support non http(s) channels for `responseURL`
- add test for data: URL and blob: URL

And I also add a method to check denied cross-site request, since State(), GetStatusText(nsCString& aStatusText) and GetResponeURL(nsAString& aUrl) use same code block not to leak information for denied cross-site request.
Attachment #8412053 - Attachment is obsolete: true
Attachment #8412215 - Flags: review?(bugs)
Comment on attachment 8412215 [details] [diff] [review]
patch v4, fix to return non-http URL for responseURL, and some refactoring

>diff --git a/content/base/test/test_XHRResponseURL.js b/content/base/test/test_XHRResponseURL.js
This is not a test file itself. Could you rename it to file_


Still going through the actual tests. (Use of generators makes the code a bit hard to read.)
Comment on attachment 8412215 [details] [diff] [review]
patch v4, fix to return non-http URL for responseURL, and some refactoring

(Use of Generators really does make this hard to read)


Could you add tests for several redirects.
Attachment #8412215 - Flags: review?(bugs) → review-
Thanks for review again!

(In reply to Olli Pettay [:smaug] from comment #10)
>>diff --git a/content/base/test/test_XHRResponseURL.js b/content/base/test/test_XHRResponseURL.js
> This is not a test file itself. Could you rename it to file_
renamed.

(In reply to Olli Pettay [:smaug] from comment #11)
> (Use of Generators really does make this hard to read)
I think it is not good for us that the test is not easy to read, since it means we cannot maintain the test easily.
So I rewrote the test using Promise instead of Generators.

> Could you add tests for several redirects.
added.
Now the redirect tests we have for `responseURL` are:
- request to origin-A [1], then redirect to origin-A
- request to origin-A, then redirect to origin-A, finally got origin-A
- request to origin-A, then redirect to origin-B [2]
- request to origin-A, then redirect to origin-A, finally got origin-B
- request to origin-B, then redirect back to origin-A
- request to origin-B, then redirect to origin-B
- request to origin-B, then redirect to origin-C [3]
- request to origin-B, then redirect to origin-B, finally got origin-A
- request to origin-B, then redirect to origin-B, finally got origin-B
- request to origin-B, then redirect to origin-B, finally got origin-C
- request to origin-B, then redirect to origin-C, finally got origin-D [4]

[1] http://mochi.test:8888
[2] http://example.com
[3] http://example.org
[4] http://test1.example.com


other fix:
- rename isDeniedCrossSiteRequest() to IsDeniedCrossSiteRequest() (capitalized first letter)
Attachment #8412215 - Attachment is obsolete: true
Attachment #8414099 - Flags: review?(bugs)
Attached patch patch v5.1, fix for todo message (obsolete) — Splinter Review
(In reply to :xKhorasan from comment #2)
> In worker environment, `resposneURL` test for cross origin redirect fails,
> but it seems that the failure is caused by bug 886235.
Bug 886235 has been resolved DUPLICATE of bug 882458, so I updated test messages for todo() and todo_is().
Attachment #8414099 - Attachment is obsolete: true
Attachment #8414099 - Flags: review?(bugs)
Attachment #8415897 - Flags: review?(bugs)
Comment on attachment 8415897 [details] [diff] [review]
patch v5.1, fix for todo message

>+nsXMLHttpRequest::IsDeniedCrossSiteRequest()
>+{
>+  if (mState & XML_HTTP_REQUEST_USE_XSITE_AC) {
>+    if (mChannel) {
if ((mState & XML_HTTP_REQUEST_USE_XSITE_AC) && mChannel)
would be a tiny bit simpler


>+void
>+nsXMLHttpRequest::GetResponseURL(nsAString& aUrl)
>+{
>+  aUrl.Truncate();
>+
>+  if (!mChannel) {
>+    return;
>+  }
The spec is super unclear here but apparently we should return non-empty url only
after HEADERS_RECEIVED has been set, so that while doing redirects we don't reveal information about those.
Could you change that and add some test for it too.
Attachment #8415897 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #14)
> >+nsXMLHttpRequest::IsDeniedCrossSiteRequest()
> >+{
> >+  if (mState & XML_HTTP_REQUEST_USE_XSITE_AC) {
> >+    if (mChannel) {
> if ((mState & XML_HTTP_REQUEST_USE_XSITE_AC) && mChannel)
> would be a tiny bit simpler
Thanks, Fixed.

> The spec is super unclear here but apparently we should return non-empty url
> only
> after HEADERS_RECEIVED has been set, so that while doing redirects we don't
> reveal information about those.
Yes, the spec describe HEADERS_RECEIVED as "Several response members of the object are now available.", so we should return empty url before `HEADERS_RECEIVED` is set.
Fixed to add readyState check, and add test to ensure not to leak responseURL while doing redirects.

(BTW, since we currently do not check `readyState` for `statusText` [1], `statusText` may be revealed while doing redirects.  Would it be better filing as a separate bug?)

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1103
Attachment #8415897 - Attachment is obsolete: true
Attachment #8420499 - Flags: review?(bugs)
(In reply to :xKhorasan from comment #15)
> (BTW, since we currently do not check `readyState` for `statusText` [1],
> `statusText` may be revealed while doing redirects.  Would it be better
> filing as a separate bug?)
Separate bug please
Attachment #8420499 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #16)
> (In reply to :xKhorasan from comment #15)
> > (BTW, since we currently do not check `readyState` for `statusText` [1],
> > `statusText` may be revealed while doing redirects.  Would it be better
> > filing as a separate bug?)
> Separate bug please
filed as bug 1011748.
Thanks for pushing the patch to try.

The tests that check `responseURL` while doing redirect failed in e10s and B2G environment.

In those environment, we cannot use "http-on-examine-response" observer (see bug 945268).
But we can use "specialpowers-http-notify-request" observer for testing, which was introduced in bug 945268, and that observer has enough power to check `responseURL` is empty before HEADERS_RECEIVED is set.

I fixed only the test part of attachment 8420499 [details] [diff] [review] (patch v6) to use "specialpowers-http-notify-request" instead of "http-on-examine-response".
With attached patch, I ran mochitest-plain for content/base/test/test_XHRResponseURL.html and dom/worker/test/test_xhr_responseURL.html with (and without) --e10s options, and there was no test failure.

I will try testing on B2G environment too, and if there is no problem, then I will set review? flag.
Attachment #8420499 - Attachment is obsolete: true
Comment on attachment 8424249 [details] [diff] [review]
patch v7, fix test for e10s and B2G environment

(In reply to :xKhorasan from comment #19)
> Created attachment 8424249 [details] [diff] [review]
> patch v7, fix test for e10s and B2G environment
> 
> I will try testing on B2G environment too, and if there is no problem, then
> I will set review? flag.

I ran mochitest-remote using emulator-jb for content/base/test/test_XHRResponseURL.html and dom/workers/test/test_xhr_responseURL.html, and there was no test failure.

Olli, could you take another look?
Attachment #8424249 - Flags: review?(bugs)
Attachment #8424249 - Flags: review?(bugs) → review+
Thanks for pushing the patch to try server again!
The result looks OK.
And I confirmed that the patch can apply cleanly to the latest mozilla-central.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7ce943352e8e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.