Bug 158022 – POST request on a blob resource should return a "network error" instead of HTTP 500 response

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nael Ouedraogo]

POST request on a blob resource should return a "network error" instead of HTTP 500 response.

[Nael Ouedraogo]

Created attachment 279887 [details]
Patch

[Build Bot]

Comment on attachment 279887 [details]
Patch

Attachment 279887 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1386400

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 279888 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Nael Ouedraogo]

Failure observed on mac-yosemite seems not related to the patch.
Failed tests on mac-elcapitan in debug successfully passed locally.

[Alex Christensen]

Your new test passes in Chrome but not in Firefox, but http://w3c-test.org/fetch/api/basic/scheme-blob.html also passes in Chrome but not Firefox, which leads me to believe that Firefox is wrong in this case.

Please include all the HTTP methods in your test and an invalid method.

[Alex Christensen]

Comment on attachment 279887 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=279887&action=review

> Source/WebCore/xml/XMLHttpRequest.cpp:-670
> -    // Only GET request is supported for blob URL.

I'm not sure why this is here.  It comes from http://trac.webkit.org/changeset/66462/trunk/WebCore/xml/XMLHttpRequest.cpp but that was not part of the reviewed patch.

> LayoutTests/fast/files/xhr-blob-post-request.html:52
> +        log('error callback called for synchronous XHR');

Please put PASS in this message to indicate that this is expected.

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/basic/scheme-blob-worker-expected.txt:-4
>  PASS Fetching [GET] blob:http://www.localhost:8800/ is KO 
> -FAIL Fetching [POST] URL.createObjectURL(blob) is KO assert_unreached: Should have rejected. Reached unreachable code

It would probably be worth adding more than GET and POST to the w3c test suite, too.

[Nael Ouedraogo]

Created attachment 279964 [details]
Patch

[Nael Ouedraogo]

(In reply to comment #6)
> > LayoutTests/fast/files/xhr-blob-post-request.html:52
> > +        log('error callback called for synchronous XHR');
> 
> Please put PASS in this message to indicate that this is expected.

Thanks for the review. I fixed this in the uploaded patch.

> > LayoutTests/imported/w3c/web-platform-tests/fetch/api/basic/scheme-blob-worker-expected.txt:-4
> >  PASS Fetching [GET] blob:http://www.localhost:8800/ is KO 
> > -FAIL Fetching [POST] URL.createObjectURL(blob) is KO assert_unreached: Should have rejected. Reached unreachable code
> 
> It would probably be worth adding more than GET and POST to the w3c test
> suite, too.

I have also added XHR tests for all HTTP methods and also for an invalid method. 
I'll add additional WPT tests for FETCH in a new patch.

[Build Bot]

Comment on attachment 279964 [details]
Patch

Attachment 279964 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1392234

New failing tests:
fast/files/xhr-blob-request.html

[Build Bot]

Created attachment 279967 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Alex Christensen]

Comment on attachment 279964 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=279964&action=review

> LayoutTests/fast/files/xhr-blob-request.html:145
> +test_notAllowed_req_blob_async('POST', true);

Because these are asynchronous, I don't think there's a guarantee that this will be the last test to receive a response, which could lead to flakiness.  A better design would be to either end or start the next test in the onloadend function so the order is guaranteed.

[Build Bot]

Comment on attachment 279964 [details]
Patch

Attachment 279964 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1392247

New failing tests:
fast/files/xhr-blob-request.html

[Build Bot]

Created attachment 279968 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Nael Ouedraogo]

Created attachment 280105 [details]
Patch

[Nael Ouedraogo]

> Because these are asynchronous, I don't think there's a guarantee that this
> will be the last test to receive a response, which could lead to flakiness. 
> A better design would be to either end or start the next test in the
> onloadend function so the order is guaranteed.

I fixed this issue in the uploaded patch with use of promise_test function of testharness.js script.
Results are correct for asynchronous XHR request with all HTTP methods.

Synchronous XHR requests with not allowed HTTP methods fail since not only an exception is thrown but also onError callback is called. XHR specification specifies that only exception should be thrown. The tests pass with Chrome. I'll investigate it further.

New tests have been added for Fetch in WPT tests. All tests passes. A pull request to WPT repository is submitted for these new tests (https://github.com/w3c/web-platform-tests/pull/3094).

[WebKit Commit Bot]

Comment on attachment 280105 [details]
Patch

Rejecting attachment 280105 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Commit Bot]

Comment on attachment 280105 [details]
Patch

Rejecting attachment 280105 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Alex Christensen]

(In reply to comment #15)
> Synchronous XHR requests with not allowed HTTP methods fail since not only
> an exception is thrown but also onError callback is called. XHR
> specification specifies that only exception should be thrown. The tests pass
> with Chrome. I'll investigate it further.
I think this should be investigated before landing this.

[Nael Ouedraogo]

Created attachment 280236 [details]
Patch

[Nael Ouedraogo]

(In reply to comment #18)
> I think this should be investigated before landing this.

I modified networkError() function to avoid dispatching an error event for synchronous XHR request as per specification (https://xhr.spec.whatwg.org/#request-error-steps). Unfortunately, several tests in http/tests/xmlhttprequest fail due to missing call of onError callback. For this reason, it seems difficult to remove the test in XMLHttpRequest::createRequest() for synchronous XHR request.

In the uploaded patch, XHR behavior is modified only for asynchronous requests on Blob with not allowed HTTP methods. OnError callback is called instead of throwing an exception which is conform to specification.

[WebKit Commit Bot]

Comment on attachment 280236 [details]
Patch

Clearing flags on attachment: 280236

Committed r201557: <http://trac.webkit.org/changeset/201557>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.