Fetch using FormData with file doesn't go through Service Worker

[nyro]

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/

[Radar WebKit Bug Importer]

<rdar://problem/41975544>

[youenn fablet]

Thanks for the report.
This is a known limitation of the service worker implementation at the moment in FetchBody::fromFormData.

[youenn fablet]

A potential workaround is to use blobs if possible although there might be limitations there as well.

[nyro]

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/

[nyro]

Hello,
Is there any news about that?
It's been 2 years now...

[nyro]

Hello,
Is there any news about that?
It's been 3 years now...

[youenn fablet]

Created attachment 448239 [details]
Patch

[youenn fablet]

Created attachment 448241 [details]
Patch

[youenn fablet]

Created attachment 448249 [details]
Patch

[youenn fablet]

Created attachment 448282 [details]
Patch

[Chris Dumez]

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?

[youenn fablet]

*** Bug 183695 has been marked as a duplicate of this bug. ***

[youenn fablet]

Created attachment 448367 [details]
Patch for landing

[EWS]

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].

[youenn fablet]

> > 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.