Bug 158022 - POST request on a blob resource should return a "network error" instead of HTTP 500 response
Summary: POST request on a blob resource should return a "network error" instead of HT...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nael Ouedraogo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-24 05:18 PDT by Nael Ouedraogo
Modified: 2016-06-01 10:12 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.65 KB, patch)
2016-05-26 08:38 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (888.38 KB, application/zip)
2016-05-26 08:56 PDT, Build Bot
no flags Details
Patch (12.17 KB, patch)
2016-05-27 09:17 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (803.49 KB, application/zip)
2016-05-27 10:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (1.42 MB, application/zip)
2016-05-27 10:15 PDT, Build Bot
no flags Details
Patch (13.75 KB, patch)
2016-05-30 11:18 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Patch (13.10 KB, patch)
2016-06-01 07:33 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nael Ouedraogo 2016-05-24 05:18:40 PDT
POST request on a blob resource should return a "network error" instead of HTTP 500 response.
Comment 1 Nael Ouedraogo 2016-05-26 08:38:47 PDT
Created attachment 279887 [details]
Patch
Comment 2 Build Bot 2016-05-26 08:56:05 PDT
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.
Comment 3 Build Bot 2016-05-26 08:56:08 PDT
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
Comment 4 Nael Ouedraogo 2016-05-26 10:28:24 PDT
Failure observed on mac-yosemite seems not related to the patch.
Failed tests on mac-elcapitan in debug successfully passed locally.
Comment 5 Alex Christensen 2016-05-26 19:58:16 PDT
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.
Comment 6 Alex Christensen 2016-05-26 23:06:34 PDT
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.
Comment 7 Nael Ouedraogo 2016-05-27 09:17:52 PDT
Created attachment 279964 [details]
Patch
Comment 8 Nael Ouedraogo 2016-05-27 09:20:21 PDT
(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.
Comment 9 Build Bot 2016-05-27 10:03:59 PDT
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
Comment 10 Build Bot 2016-05-27 10:04:02 PDT
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
Comment 11 Alex Christensen 2016-05-27 10:06:27 PDT
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.
Comment 12 Build Bot 2016-05-27 10:15:32 PDT
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
Comment 13 Build Bot 2016-05-27 10:15:35 PDT
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
Comment 14 Nael Ouedraogo 2016-05-30 11:18:19 PDT
Created attachment 280105 [details]
Patch
Comment 15 Nael Ouedraogo 2016-05-30 11:32:42 PDT
> 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).
Comment 16 WebKit Commit Bot 2016-05-30 11:34:39 PDT
Comment on attachment 280105 [details]
Patch

Rejecting attachment 280105 [details] from review queue.

nael.ouedp@gmail.com 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.
Comment 17 WebKit Commit Bot 2016-05-30 11:35:17 PDT
Comment on attachment 280105 [details]
Patch

Rejecting attachment 280105 [details] from commit-queue.

nael.ouedp@gmail.com 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.
Comment 18 Alex Christensen 2016-05-31 09:57:22 PDT
(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.
Comment 19 Nael Ouedraogo 2016-06-01 07:33:27 PDT
Created attachment 280236 [details]
Patch
Comment 20 Nael Ouedraogo 2016-06-01 08:14:44 PDT
(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.
Comment 21 WebKit Commit Bot 2016-06-01 10:12:50 PDT
Comment on attachment 280236 [details]
Patch

Clearing flags on attachment: 280236

Committed r201557: <http://trac.webkit.org/changeset/201557>
Comment 22 WebKit Commit Bot 2016-06-01 10:12:56 PDT
All reviewed patches have been landed.  Closing bug.