[XMLHttpRequest] Stop throwing for network error in async mode

[XMLHttpRequest] Stop throwing for network error in async mode

See the history and decision at http://crbug.com/479484

This CL updates fast/files/apply-blob-url-to-xhr.html layout test to
stop expecting an exception but expect an error event being
dispatched. Using this opportunity, merged the worker version of the
test into one test script.

security/cannot-read-self-from-file and
http/tests/xmlhttprequest/cross-origin-unsupported-url (and its worker
version) are simply updated to expect error event dispatching.

BUG=479484
R=sof,mkwst

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199098

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199282

[tyoshino (SeeGerritForStatus), 2015-06-24 13:31:21]

tyoshino@chromium.org changed reviewers:
+ mkwst@chromium.org, sigbjorn@opera.com

[tyoshino (SeeGerritForStatus), 2015-06-24 13:31:59]

tyoshino@chromium.org changed reviewers:
+ sigbjornf@opera.com
- sigbjorn@opera.com

[tyoshino (SeeGerritForStatus), 2015-06-24 13:32:10]

On 2015/06/24 13:31:31, tyoshino wrote:

-sigbjorn
+sof

[Mike West, 2015-06-24 13:45:17]

Do we have tests which verify that the `error` event is triggered? And that
`readystatechange` events behave as expected?

[tyoshino (SeeGerritForStatus), 2015-06-24 13:52:52]

On 2015/06/24 13:45:17, Mike West wrote:
> Do we have tests which verify that the `error` event is triggered? And that
> `readystatechange` events behave as expected?

Oh. I need to fix the throwDOMException code to dispatch an error. Wait a
moment.

[tyoshino (SeeGerritForStatus), 2015-06-24 14:01:01]

On 2015/06/24 13:52:52, tyoshino wrote:
> On 2015/06/24 13:45:17, Mike West wrote:
> > Do we have tests which verify that the `error` event is triggered? And that
> > `readystatechange` events behave as expected?
> 
> Oh. I need to fix the throwDOMException code to dispatch an error. Wait a
> moment.

I meant this call

exceptionState.throwDOMException(NetworkError, "'GET' is the only method allowed
for 'blob:' URLs.");

didFail() calls from ThreadableLoader should correctly dispatch error events.
I'll add tests where needed to cover all the paths.

[tyoshino (SeeGerritForStatus), 2015-07-03 12:03:47]

Re: error event dispatching:
- shouldTreatURLSchemeAsCORSEnabled() in makeCrossOriginAccessRequest()
  - Tested by cross-origin-unsupported-url.html
- redirectReceived()
  - May be called sync when the Blink cache remembers the redirect? ->
Investigating.
- handlePreflightResponse(), handleResponse(), notifyFinished()
  - same as above
- blob + non-GET checking in XMLHttpRequest::createRequest()
  - Just need to fix and add tests

[Mike West, 2015-07-08 07:43:46]

On 2015/07/03 at 12:03:47, tyoshino wrote:
> Re: error event dispatching:
> - shouldTreatURLSchemeAsCORSEnabled() in makeCrossOriginAccessRequest()
>   - Tested by cross-origin-unsupported-url.html
> - redirectReceived()
>   - May be called sync when the Blink cache remembers the redirect? ->
Investigating.
> - handlePreflightResponse(), handleResponse(), notifyFinished()
>   - same as above
> - blob + non-GET checking in XMLHttpRequest::createRequest()
>   - Just need to fix and add tests

Ping. Is this still in progress? :)

[tyoshino (SeeGerritForStatus), 2015-07-08 10:10:50]

On 2015/07/08 07:43:46, Mike West wrote:
> On 2015/07/03 at 12:03:47, tyoshino wrote:
> > Re: error event dispatching:
> > - shouldTreatURLSchemeAsCORSEnabled() in makeCrossOriginAccessRequest()
> >   - Tested by cross-origin-unsupported-url.html
> > - redirectReceived()
> >   - May be called sync when the Blink cache remembers the redirect? ->
> Investigating.
> > - handlePreflightResponse(), handleResponse(), notifyFinished()
> >   - same as above
> > - blob + non-GET checking in XMLHttpRequest::createRequest()
> >   - Just need to fix and add tests
> 
> Ping. Is this still in progress? :)

Yes. I'll go through them again and add tests if necessary, maybe next week.
I'll be OOO on Friday and busy for giving a talk on Thursday.

I'll ping you again when ready. Sorry for delay.

[Mike West, 2015-07-09 12:48:19]

On 2015/07/08 at 10:10:50, tyoshino wrote:
> On 2015/07/08 07:43:46, Mike West wrote:
> > On 2015/07/03 at 12:03:47, tyoshino wrote:
> > > Re: error event dispatching:
> > > - shouldTreatURLSchemeAsCORSEnabled() in makeCrossOriginAccessRequest()
> > >   - Tested by cross-origin-unsupported-url.html
> > > - redirectReceived()
> > >   - May be called sync when the Blink cache remembers the redirect? ->
> > Investigating.
> > > - handlePreflightResponse(), handleResponse(), notifyFinished()
> > >   - same as above
> > > - blob + non-GET checking in XMLHttpRequest::createRequest()
> > >   - Just need to fix and add tests
> > 
> > Ping. Is this still in progress? :)
> 
> Yes. I'll go through them again and add tests if necessary, maybe next week.
I'll be OOO on Friday and busy for giving a talk on Thursday.
> 
> I'll ping you again when ready. Sorry for delay.

No worries! I just noticed a similar bug with mixed content, and I'm hopeful
this will fix it. :)

[sof, 2015-07-12 13:34:48]

https://codereview.chromium.org/1060113004/diff/20001/Source/core/xmlhttprequ...
File Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1060113004/diff/20001/Source/core/xmlhttprequ...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:1070: m_loader =
ThreadableLoader::create(executionContext, this, request, options,
resourceLoaderOptions);
Given that async will now no longer throw, I think the code would read better if
you simply "return;" here.

[tyoshino (SeeGerritForStatus), 2015-07-14 06:54:43]

On 2015/07/09 12:48:19, Mike West (OOO through 16th) wrote:
> On 2015/07/08 at 10:10:50, tyoshino wrote:
> > On 2015/07/08 07:43:46, Mike West wrote:
> > > On 2015/07/03 at 12:03:47, tyoshino wrote:
> > > > Re: error event dispatching:
> > > > - shouldTreatURLSchemeAsCORSEnabled() in makeCrossOriginAccessRequest()
> > > >   - Tested by cross-origin-unsupported-url.html
> > > > - redirectReceived()
> > > >   - May be called sync when the Blink cache remembers the redirect? ->
> > > Investigating.
> > > > - handlePreflightResponse(), handleResponse(), notifyFinished()
> > > >   - same as above

Resource::addClient() may really invoke them synchronously. But they go through
the same path as the async XHR. I think they should just work (dispatch an error
event).

> > > > - blob + non-GET checking in XMLHttpRequest::createRequest()
> > > >   - Just need to fix and add tests

Now this has been addressed by ps4.

> > > 
> > > Ping. Is this still in progress? :)
> > 
> > Yes. I'll go through them again and add tests if necessary, maybe next week.
> I'll be OOO on Friday and busy for giving a talk on Thursday.
> > 
> > I'll ping you again when ready. Sorry for delay.
> 
> No worries! I just noticed a similar bug with mixed content, and I'm hopeful
> this will fix it. :)

Oh, which one?

[tyoshino (SeeGerritForStatus), 2015-07-14 07:00:05]

https://codereview.chromium.org/1060113004/diff/20001/Source/core/xmlhttprequ...
File Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1060113004/diff/20001/Source/core/xmlhttprequ...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:1070: m_loader =
ThreadableLoader::create(executionContext, this, request, options,
resourceLoaderOptions);
On 2015/07/12 13:34:48, sof wrote:
> Given that async will now no longer throw, I think the code would read better
if
> you simply "return;" here.

Done.

[tyoshino (SeeGerritForStatus), 2015-07-14 07:00:18]

Now checking layout test results...

[sof, 2015-07-14 07:07:01]

https://codereview.chromium.org/1060113004/diff/100001/Source/core/xmlhttpreq...
File Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1060113004/diff/100001/Source/core/xmlhttpreq...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:838: m_exceptionCode =
NetworkError;
Add an early return if !m_exceptionCode ?

[tyoshino (SeeGerritForStatus), 2015-07-14 08:48:55]

Also added change on security/cannot-read-self-from-file.html

Now all the layout tests pass.

https://codereview.chromium.org/1060113004/diff/100001/Source/core/xmlhttpreq...
File Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1060113004/diff/100001/Source/core/xmlhttpreq...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:838: m_exceptionCode =
NetworkError;
On 2015/07/14 07:07:01, sof wrote:
> Add an early return if !m_exceptionCode ?

Done.

[sof, 2015-07-14 09:30:58]

lgtm, i think this now takes care of mkwst's initial concerns wrt coverage also.

https://codereview.chromium.org/1060113004/diff/120001/LayoutTests/http/tests...
File
LayoutTests/http/tests/xmlhttprequest/cross-origin-unsupported-url-expected.txt
(right):

https://codereview.chromium.org/1060113004/diff/120001/LayoutTests/http/tests...
LayoutTests/http/tests/xmlhttprequest/cross-origin-unsupported-url-expected.txt:15:
PASS xhr.send() did not throw exception.
The information loss here is slightly unfortunate, but i've said that already..
:)

[tyoshino (SeeGerritForStatus), 2015-07-17 10:14:13]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-07-17 10:14:13]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1060113004/#ps140001
(title: "Rebease")

[commit-bot: I haz the power, 2015-07-17 10:14:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1060113004/140001

[commit-bot: I haz the power, 2015-07-17 11:46:32]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199098

[Julien - ping for review, 2015-07-17 17:34:17]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1229013004/ by jchaffraix@chromium.org.

The reason for reverting is: The test is causing browser_tests
ExtensionApiTest.CrossOriginXHRNoFileAccess to fail on the Linux Chromium OS and
the gn bots. Manually confirmed that it's the culprit with a gn component build
on Linux.

Relevant log:
[FAIL] fileAccessNotAllowed: expected an error to be thrown
Error
    at fileAccessNotAllowed
(chrome-extension://dgmgifjgehkiibkkonbjcmcnagdegeco/test.js:24:21)
    at Object.\u003Canonymous> (extensions::test:95:19)
    at Object.handleRequest (extensions::binding:57:27)
    at Object.\u003Canonymous> (extensions::binding:385:32)
    at Object.\u003Canonymous> (extensions::test:326:16)
    at Object.handleRequest (extensions::binding:57:27)
    at Object.\u003Canonymous> (extensions::binding:385:32)
    at Object.callback
(chrome-extension://dgmgifjgehkiibkkonbjcmcnagdegeco/test.js:6:15)
    at safeCallbackApply (extensions::sendRequest:21:15)
    at handleResponse (extensions::sendRequest:72:7)", source:
chrome-extension://dgmgifjgehkiibkkonbjcmcnagdegeco/test.html (0)
../../chrome/browser/extensions/cross_origin_xhr_apitest.cc:37: Failure
Value of: RunExtensionTestNoFileAccess( "cross_origin_xhr/no_file_access")
  Actual: false
Expected: true

Full log of the failure:
https://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Te....

[Julien - ping for review, 2015-07-17 17:34:18]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1240503004/ by jchaffraix@chromium.org.

The reason for reverting is: The test is causing browser_tests
ExtensionApiTest.CrossOriginXHRNoFileAccess to fail on the Linux Chromium OS and
the gn bots. Manually confirmed that it's the culprit with a gn component build
on Linux.

Relevant log:
[FAIL] fileAccessNotAllowed: expected an error to be thrown
Error
    at fileAccessNotAllowed
(chrome-extension://dgmgifjgehkiibkkonbjcmcnagdegeco/test.js:24:21)
    at Object.\u003Canonymous> (extensions::test:95:19)
    at Object.handleRequest (extensions::binding:57:27)
    at Object.\u003Canonymous> (extensions::binding:385:32)
    at Object.\u003Canonymous> (extensions::test:326:16)
    at Object.handleRequest (extensions::binding:57:27)
    at Object.\u003Canonymous> (extensions::binding:385:32)
    at Object.callback
(chrome-extension://dgmgifjgehkiibkkonbjcmcnagdegeco/test.js:6:15)
    at safeCallbackApply (extensions::sendRequest:21:15)
    at handleResponse (extensions::sendRequest:72:7)", source:
chrome-extension://dgmgifjgehkiibkkonbjcmcnagdegeco/test.html (0)
../../chrome/browser/extensions/cross_origin_xhr_apitest.cc:37: Failure
Value of: RunExtensionTestNoFileAccess( "cross_origin_xhr/no_file_access")
  Actual: false
Expected: true

Full log of the failure:
https://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Te....

[tyoshino (SeeGerritForStatus), 2015-07-21 04:15:36]

Sorry for the breakage and thanks for reverting.

[tyoshino (SeeGerritForStatus), 2015-07-22 04:54:52]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2015-07-22 04:55:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1060113004/140001

[tyoshino (SeeGerritForStatus), 2015-07-22 04:55:35]

The browser test has been fixed, and checked that linux_chromium_rel_ng passes.
Relanding.

[commit-bot: I haz the power, 2015-07-22 06:15:46]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199282