Summary: | POST request on a blob resource should return a "network error" instead of HTTP 500 response | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nael Ouedraogo <nael.ouedp> | ||||||||||||||||
Component: | WebCore Misc. | Assignee: | Nael Ouedraogo <nael.ouedp> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | achristensen, ap, buildbot, commit-queue, rniwa, youennf | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Nael Ouedraogo
2016-05-24 05:18:40 PDT
Created attachment 279887 [details]
Patch
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. 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
Failure observed on mac-yosemite seems not related to the patch. Failed tests on mac-elcapitan in debug successfully passed locally. 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 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. Created attachment 279964 [details]
Patch
(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 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 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 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 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 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
Created attachment 280105 [details]
Patch
> 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 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 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. (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. Created attachment 280236 [details]
Patch
(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 on attachment 280236 [details] Patch Clearing flags on attachment: 280236 Committed r201557: <http://trac.webkit.org/changeset/201557> All reviewed patches have been landed. Closing bug. |