Bug 184490 - REGRESSION (r221839): Submitting a form with XMLHttpRequest fails when the form has an empty file input element
Summary: REGRESSION (r221839): Submitting a form with XMLHttpRequest fails when the fo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Safari 11
Hardware: All macOS 10.13
: P2 Major
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-11 00:32 PDT by Eugene
Modified: 2018-04-25 00:33 PDT (History)
14 users (show)

See Also:


Attachments
request body (715 bytes, text/plain)
2018-04-11 00:32 PDT, Eugene
no flags Details
HTML test case demonstrating bug (2.36 KB, text/html)
2018-04-11 12:35 PDT, Steve
no flags Details
HTML test case demonstrating bug (2.36 KB, text/html)
2018-04-11 12:39 PDT, Steve
no flags Details
Patch (4.43 KB, patch)
2018-04-23 16:50 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.60 MB, application/zip)
2018-04-23 17:51 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.10 MB, application/zip)
2018-04-23 18:24 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.26 MB, application/zip)
2018-04-23 18:59 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (3.19 MB, application/zip)
2018-04-23 20:05 PDT, EWS Watchlist
no flags Details
Patch (4.35 KB, patch)
2018-04-24 06:54 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eugene 2018-04-11 00:32:42 PDT
Created attachment 337684 [details]
request body

Bug is present only in Safari 11. Safari 9 and 10 work well.

1. We have a form with enctype="multipart/form-data"
2. The form has input[type=file]
3. If the file is not selected, Safari does not send the full request, stopping at the last line of the request.
4. Request body attached, header "Content-Length: 715"
5. The behavior does not depend on the version of the HTTP protocol:

HTTP/1.1
2018/04/11 00:35:35 [debug] 13989#0: *838 http read client request body
2018/04/11 00:35:35 [debug] 13989#0: *838 http client request body recv 669
2018/04/11 00:35:35 [debug] 13989#0: *838 http client request body rest 715

HTTP/2
2018/04/10 14:18:40 [info] 8478#0: *34 client prematurely closed stream: only 669 out of 715 bytes of request body received
Comment 1 Steve 2018-04-11 12:35:08 PDT
Created attachment 337720 [details]
HTML test case demonstrating bug

I believe I've also experienced this issue. A little more information to help elaborate on this issue:

* We only see this issue when submitting a form with enctype="multipart/form-data" and using FormData to send data in the XMLHttpRequest object.
* The issue only arises when the file field is left empty. A selected file allows normal submission.
* Only happened after upgrading to macOS 10.13.4 and Safari 11.1

I've attached an HTML test case that recreates the error. The file contains the following 4 examples:
* Normal form submission, non-AJAX
* AJAX form submission with no file data
* AJAX form submission using FormData to submit file data
* XMLHttpRequest submission using FormData to submit file data.

Expected behavior:
For all forms to submit a valid request and receive response data. Response data would show up beneath each form, respectively.

Actual behavior:
* The non-AJAX form submits properly with or without a file selected and always returns a valid response
* The AJAX form that sends no file data submits properly and always returns a valid response
* The AJAX form using FormData to send file data creates a valid request when file data is selected. When no file is selected and the form is submitted the browser hangs and doesn't appear to send a valid request.
* The XMLHttpRequest form using FormData to send file data creates a valid request when file data is selected. When no file is selected and the form is submitted the browser hangs and doesn't appear to send a valid request
Comment 2 Steve 2018-04-11 12:39:35 PDT
Created attachment 337723 [details]
HTML test case demonstrating bug

Added https to "HTML test case demonstrating bug" test case to remove warnings when testing.
Comment 3 Eugene 2018-04-11 13:07:43 PDT
Yep, we are experiencing this bug with XMLHttpRequest form using FormData.
Comment 4 Radar WebKit Bug Importer 2018-04-12 10:20:47 PDT
<rdar://problem/39385169>
Comment 5 Ryosuke Niwa 2018-04-16 23:42:02 PDT
Another report of this issue on Twitter: https://qiita.com/yuya_presto/items/65be91b0255af49f0396
Comment 6 Matt 2018-04-18 04:57:25 PDT
Iā€™m experiencing this as well. First noticed with 11.1 but then found in 11.2. Posted about it on StackOverkow back on 4/2 (https://stackoverflow.com/questions/49614091/safari-11-1-ajax-xhr-form-submission-fails-when-inputtype-file-is-empty) and filed a bug report with Apple.
Comment 7 Tadeu Zagallo 2018-04-23 16:50:32 PDT
Created attachment 338620 [details]
Patch
Comment 8 EWS Watchlist 2018-04-23 17:51:22 PDT
Comment on attachment 338620 [details]
Patch

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

New failing tests:
http/tests/fileapi/xhr-send-form-data-mimetype-normalization.html
http/tests/local/formdata/send-form-data-with-sliced-file.html
imported/w3c/web-platform-tests/XMLHttpRequest/formdata.htm
imported/w3c/web-platform-tests/XMLHttpRequest/formdata-blob.htm
http/tests/fileapi/xhr-send-form-data-filename-escaping.html
http/tests/misc/form-blob-challenge.html
Comment 9 EWS Watchlist 2018-04-23 17:51:23 PDT
Created attachment 338625 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 10 EWS Watchlist 2018-04-23 18:24:01 PDT
Comment on attachment 338620 [details]
Patch

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

New failing tests:
http/tests/fileapi/xhr-send-form-data-mimetype-normalization.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-xhr.https.html
http/tests/misc/form-blob-challenge.html
imported/w3c/web-platform-tests/XMLHttpRequest/formdata-blob.htm
http/tests/fileapi/xhr-send-form-data-filename-escaping.html
imported/w3c/web-platform-tests/XMLHttpRequest/formdata.htm
Comment 11 EWS Watchlist 2018-04-23 18:24:02 PDT
Created attachment 338626 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 12 EWS Watchlist 2018-04-23 18:59:20 PDT
Comment on attachment 338620 [details]
Patch

Attachment 338620 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7417764

New failing tests:
http/tests/fileapi/xhr-send-form-data-mimetype-normalization.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-xhr.https.html
imported/w3c/web-platform-tests/XMLHttpRequest/formdata.htm
imported/w3c/web-platform-tests/XMLHttpRequest/formdata-blob.htm
http/tests/fileapi/xhr-send-form-data-filename-escaping.html
http/tests/misc/form-blob-challenge.html
Comment 13 EWS Watchlist 2018-04-23 18:59:21 PDT
Created attachment 338629 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 14 EWS Watchlist 2018-04-23 20:05:35 PDT
Comment on attachment 338620 [details]
Patch

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

New failing tests:
http/tests/fileapi/xhr-send-form-data-mimetype-normalization.html
http/tests/local/formdata/send-form-data-with-sliced-file.html
imported/w3c/web-platform-tests/XMLHttpRequest/formdata.htm
imported/w3c/web-platform-tests/XMLHttpRequest/formdata-blob.htm
http/tests/fileapi/xhr-send-form-data-filename-escaping.html
http/tests/misc/form-blob-challenge.html
Comment 15 EWS Watchlist 2018-04-23 20:05:37 PDT
Created attachment 338631 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 16 Tadeu Zagallo 2018-04-24 06:54:31 PDT
Created attachment 338646 [details]
Patch
Comment 17 Geoffrey Garen 2018-04-24 10:48:36 PDT
Comment on attachment 338646 [details]
Patch

r=me
Comment 18 WebKit Commit Bot 2018-04-24 11:15:54 PDT
Comment on attachment 338646 [details]
Patch

Clearing flags on attachment: 338646

Committed r230963: <https://trac.webkit.org/changeset/230963>
Comment 19 WebKit Commit Bot 2018-04-24 11:15:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Alexey Proskuryakov 2018-04-24 12:47:18 PDT
Comment on attachment 338646 [details]
Patch

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

> Source/WebCore/platform/network/FormData.cpp:227
> +    else if (file.size())
>          appendBlob(file.url());

This doesn't look like the same check that is described in the ChangeLog ("a file but has no path"). A special case for empty files seems like it can't be right, can it?
Comment 21 Tadeu Zagallo 2018-04-24 14:12:57 PDT
(In reply to Alexey Proskuryakov from comment #20)
> Comment on attachment 338646 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338646&action=review
> 
> > Source/WebCore/platform/network/FormData.cpp:227
> > +    else if (file.size())
> >          appendBlob(file.url());
> 
> This doesn't look like the same check that is described in the ChangeLog ("a
> file but has no path"). A special case for empty files seems like it can't
> be right, can it?

The problem I found (and maybe should have update the ChangeLog to describe it) was that Blobs created from JavaScript are also instances of File at this point, so the logic here was already: if it is a file and has no path, it must be a blob. My idea was to change it to: If it is a file and has no path and is not empty, it must be a blob. This also excludes empty blobs, but that should be ok since the empty blob would not change the request in any way.
Comment 22 Alexey Proskuryakov 2018-04-24 17:56:37 PDT
> the empty blob would not change the request in any way.

I find this surprising, but don't have the time to test and prove this right or wrong. Won't there be a section with a MIME type and file name, and empty content? Or in other words, empty files do exist and can be uploaded (e.g. can be attached with form submission), so why can't empty blobs be uploaded?
Comment 23 Tadeu Zagallo 2018-04-25 00:33:52 PDT
> Won't there be a section with a MIME type and file name, and empty content?

Oh, yes. This information has already been added to the output at this point, regardless of whether it is a file or a blob. With this change, we just skip adding the contents of the blob to the output, but it has 0 bytes, which is what I meant by "the empty blob would not change the request in any way". Sorry if my first comment wasn't very clear.