Bug 187461 - Fetch using FormData with file doesn't go through Service Worker
Summary: Fetch using FormData with file doesn't go through Service Worker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: Safari Technology Preview
Hardware: All Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
: 183695 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-07-09 02:15 PDT by nyro
Modified: 2022-01-05 02:59 PST (History)
7 users (show)

See Also:


Attachments
Zip containing all 4 files described (4.91 KB, application/zip)
2018-07-09 02:15 PDT, nyro
no flags Details
Patch (42.87 KB, patch)
2022-01-03 08:09 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (37.10 KB, patch)
2022-01-03 08:14 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (41.90 KB, patch)
2022-01-03 11:08 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (46.13 KB, patch)
2022-01-04 04:20 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (46.79 KB, patch)
2022-01-05 01:30 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description nyro 2018-07-09 02:15:03 PDT
Created attachment 344569 [details]
Zip containing all 4 files described

When using a FormData with file to send content of a form using the fetch API, the request doesn't go through the associated Service Worker.

To show it, I setup the attached file :  

# index.html
Contains 3 forms with a simple debug div to show what's happening on devices

# test.js
Handle the submit of the form in order to:
1. prevent default it
2. Create a formData
3. If the form has the id "removeFile", delete the file from the formData
4. Send data to service worker/server using the fetch API
5. Get a JSON response
6. Show the json response on the debug

# sw.js
A minimal service worker that only add a "X-FROM-SW" header to the request containing the timestamp

# up.php
A simple PHP script to return a JSON with the received "X-FROM-SW" or false if not found.


# Observed results
- Simple Form : everything ok as expected and the result contains the X-FROM-SW
- File form : doesn't pass through the service worker (no log in the fetch event), so there is no header added
- Remove file form : treated like the simple form, so the result contains the X-FROM-SW

I setup these file on the following URL:
https://bugs.nyro.com/safariFileUpload/
Comment 1 Radar WebKit Bug Importer 2018-07-09 09:14:18 PDT
<rdar://problem/41975544>
Comment 2 youenn fablet 2018-07-15 07:13:04 PDT
Thanks for the report.
This is a known limitation of the service worker implementation at the moment in FetchBody::fromFormData.
Comment 3 youenn fablet 2018-07-15 07:14:41 PDT
A potential workaround is to use blobs if possible although there might be limitations there as well.
Comment 4 nyro 2018-07-24 08:31:51 PDT
Youenn, following your recommandation, I ended up with the following:

#####################
formData.delete('file');

// get it as blob adn readd it as regular post field
// NB: File has to be recreated on server side using _files[file][name] and _files[file][data] to work as it was a regular file upload
var file = form.querySelector('input[type="file"]').files[0];

formData.append('_files[file][name]', file.name);

var reader = new FileReader();
reader.onload = function (e) {
    formData.append('_files[file][data]', e.target.result);

    doFetch(id, form, formData);
};
reader.readAsDataURL(file);
#####################

Which works, the request goes through the service Worker.

On server side, it becomes a little tricky regarding the language choosen, but I mentionned all the necessearry regarding PHP, the one I'm using.

I didn't implemented it yet on my production PWA, but I'll do it soon and come back here if I found out more gotcha.

In the mean time, I hope this will be fixed soon on Safari in order to remove all this code ;)

All the updated code and exemple have been updated on my demo page:
https://bugs.nyro.com/safariFileUpload/
Comment 5 nyro 2020-07-25 23:11:03 PDT
Hello,
Is there any news about that?
It's been 2 years now...
Comment 6 nyro 2021-07-28 14:55:30 PDT
Hello,
Is there any news about that?
It's been 3 years now...
Comment 7 youenn fablet 2022-01-03 08:09:24 PST
Created attachment 448239 [details]
Patch
Comment 8 youenn fablet 2022-01-03 08:14:03 PST
Created attachment 448241 [details]
Patch
Comment 9 youenn fablet 2022-01-03 11:08:55 PST
Created attachment 448249 [details]
Patch
Comment 10 youenn fablet 2022-01-04 04:20:49 PST
Created attachment 448282 [details]
Patch
Comment 11 Chris Dumez 2022-01-04 09:14:21 PST
Comment on attachment 448282 [details]
Patch

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

r=me with nits.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:278
> +        if (!value.size()) {

could probably use `value.empty()`

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:284
> +        builder.append(value.data(), value.size());

A bit disappointing that FragmentedSharedBuffer doesn't have an append() overload that takes a Span<>. We should probably add one.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:306
> +        if (!value.size()) {

could be `value.empty()`

> Source/WebCore/Modules/fetch/FormDataConsumer.cpp:59
> +    WTF::switchOn(m_formData->elements()[m_currentElementIndex++].data, [this](const Vector<uint8_t>& content) {

May not need WTF::

> Source/WebCore/Modules/fetch/FormDataConsumer.cpp:70
> +    consume(Span<const uint8_t> { content.data(), content.size() });

Can't you simply call `content.span()` ?

> Source/WebCore/Modules/fetch/FormDataConsumer.cpp:121
> +    m_callback(Span<const uint8_t> { content.data(), content.size() });

Source and destination are both a Span<const uint8_t>, why aren't we passing content directly instead of reconstructing a span? Am I missing something?
Comment 12 youenn fablet 2022-01-05 01:07:49 PST
*** Bug 183695 has been marked as a duplicate of this bug. ***
Comment 13 youenn fablet 2022-01-05 01:30:09 PST
Created attachment 448367 [details]
Patch for landing
Comment 14 EWS 2022-01-05 02:47:38 PST
Committed r287612 (245739@main): <https://commits.webkit.org/245739@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448367 [details].
Comment 15 youenn fablet 2022-01-05 02:59:28 PST
> > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:278
> > +        if (!value.size()) {
> 
> could probably use `value.empty()`

OK

> > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:284
> > +        builder.append(value.data(), value.size());
> 
> A bit disappointing that FragmentedSharedBuffer doesn't have an append()
> overload that takes a Span<>. We should probably add one.

Added it.

> > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:306
> > +        if (!value.size()) {
> 
> could be `value.empty()`

OK

> > Source/WebCore/Modules/fetch/FormDataConsumer.cpp:59
> > +    WTF::switchOn(m_formData->elements()[m_currentElementIndex++].data, [this](const Vector<uint8_t>& content) {
> 
> May not need WTF::

Removed.

> > Source/WebCore/Modules/fetch/FormDataConsumer.cpp:70
> > +    consume(Span<const uint8_t> { content.data(), content.size() });
> 
> Can't you simply call `content.span()` ?

Didn't know about this one, used it.

> > Source/WebCore/Modules/fetch/FormDataConsumer.cpp:121
> > +    m_callback(Span<const uint8_t> { content.data(), content.size() });
> 
> Source and destination are both a Span<const uint8_t>, why aren't we passing
> content directly instead of reconstructing a span? Am I missing something?

I was missing a WTFMove.