Closed
Bug 998076
Opened 10 years ago
Closed 10 years ago
implement responseURL attribute for XMLHttpRequest
Categories
(Core :: DOM: Core & HTML, defect)
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)
28.43 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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-
Assignee | ||
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
>-[scriptable, uuid(2e91e088-e9fa-4ba4-9887-2a0b7cf27a3e)]
>+[scriptable, uuid(b1b43854-62a5-4a40-841c-341d942bff69)]
You don't have to bump the iid anymore.
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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-
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8411073 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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 11•10 years ago
|
||
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-
Assignee | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
(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 14•10 years ago
|
||
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-
Assignee | ||
Comment 15•10 years ago
|
||
(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)
Comment 16•10 years ago
|
||
(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
Updated•10 years ago
|
Attachment #8420499 -
Flags: review?(bugs) → review+
Comment 17•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=c65c9e5cc9b5
Assignee | ||
Comment 18•10 years ago
|
||
(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.
Assignee | ||
Comment 19•10 years ago
|
||
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
Assignee | ||
Comment 20•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8424249 -
Flags: review?(bugs) → review+
Comment 21•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=502b05f198b9
Assignee | ||
Comment 22•10 years ago
|
||
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
Comment 23•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce943352e8e
Keywords: checkin-needed
Comment 24•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7ce943352e8e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•10 years ago
|
Keywords: dev-doc-needed
Dev docs added. Feature documentation: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/responseURL Rel note: https://developer.mozilla.org/en-US/Firefox/Releases/32
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•