Bug 180894 - Add support for response blob given to fetch events
Summary: Add support for response blob given to fetch events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-15 16:55 PST by youenn fablet
Modified: 2017-12-18 11:54 PST (History)
7 users (show)

See Also:


Attachments
Patch (24.72 KB, patch)
2017-12-15 17:04 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (26.93 KB, patch)
2017-12-18 11:08 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 youenn fablet 2017-12-15 16:55:16 PST
Add support for response blob given to fetch events
Comment 1 youenn fablet 2017-12-15 17:04:44 PST
Created attachment 329544 [details]
Patch
Comment 2 EWS Watchlist 2017-12-15 17:05:46 PST
Attachment 329544 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:57:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:61:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2017-12-17 12:03:07 PST
Comment on attachment 329544 [details]
Patch

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

> Source/WebCore/platform/network/FormData.cpp:451
> +    URL blobURL;
> +    for (auto& element : m_elements) {
> +        if (element.m_type != FormDataElement::Type::EncodedBlob || !blobURL.isNull())
> +            return { };
> +        blobURL = element.m_url;
> +    }
> +    return blobURL;

I think this is a strange way to write this. I think the algorithm it follows is: Return the URL only if there is only a single element of type EncodedBlob. But it also can handle an encoded blob if it has a null URL and skip over it. But is that feature important? Can we write it in without such complex loop logic? Like a more straightforward "check that there is nothing else except for a single encoded blob", then "get that first element since we know it's the right kind of element now", and then "return the URL"?

Also, I don’t see test coverage for this; like trying it with two encoded blob elements.

> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:78
> +    // FIXME: We should send this form data to the other process and consume it there.
> +    // For now and for the case of blobs, we read it there and send the data through IPC.

Is there some good reason we can’t do this right now?

> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:118
> +    m_isLoadingBlob = false;
> +    didFinish();

Instead of adding m_isLoadingBlob can we just do this?

1) Check m_blobLoader.has_value in "didFinish".
2) Factor out the body of didFinish without the m_blobLoader check and call it from here.

It’s really annoying to have this additional boolean.

> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:69
> +        Ref<WebServiceWorkerFetchTaskClient> client;

This creates a reference cycle. I know that the cycle will be broken when we set m_blobLoader to std::nullopt, but do we really need this object to keep itself alive like that? I guess maybe the answer is yes.
Comment 4 youenn fablet 2017-12-18 10:16:51 PST
(In reply to Darin Adler from comment #3)
> Comment on attachment 329544 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329544&action=review
> 
> > Source/WebCore/platform/network/FormData.cpp:451
> > +    URL blobURL;
> > +    for (auto& element : m_elements) {
> > +        if (element.m_type != FormDataElement::Type::EncodedBlob || !blobURL.isNull())
> > +            return { };
> > +        blobURL = element.m_url;
> > +    }
> > +    return blobURL;
> 
> I think this is a strange way to write this. I think the algorithm it
> follows is: Return the URL only if there is only a single element of type
> EncodedBlob. But it also can handle an encoded blob if it has a null URL and
> skip over it. But is that feature important? Can we write it in without such
> complex loop logic? Like a more straightforward "check that there is nothing
> else except for a single encoded blob", then "get that first element since
> we know it's the right kind of element now", and then "return the URL"?

I can change it to std::optional<URL> then.

> Also, I don’t see test coverage for this; like trying it with two encoded
> blob elements.

I will try adding a unit test and if not easy a layout test.

> 
> > Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:78
> > +    // FIXME: We should send this form data to the other process and consume it there.
> > +    // For now and for the case of blobs, we read it there and send the data through IPC.
> 
> Is there some good reason we can’t do this right now?

Blob data right now can only be gathered from the network process by the web process that creates it.
For full support we would need the service worker process to instruct the network process to clone every blob in the form data, clones being made accessible to the targeted web process.
This is the final plan when we will get full form data support but does require more work.

> > Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:118
> > +    m_isLoadingBlob = false;
> > +    didFinish();
> 
> Instead of adding m_isLoadingBlob can we just do this?
> 
> 1) Check m_blobLoader.has_value in "didFinish".
> 2) Factor out the body of didFinish without the m_blobLoader check and call
> it from here.
> 
> It’s really annoying to have this additional boolean.

m_isLoadingBlob is set in the worker thread while m_blobLoader is set asynchronously on the main thread.
To remove m_isLoadingBlob, we would need to change WebServiceWorkerFetchTaskClient::didReceiveFormData into something like WebServiceWorkerFetchTaskClient:: didReceiveFormDataAndFinish.

> > Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:69
> > +        Ref<WebServiceWorkerFetchTaskClient> client;
> 
> This creates a reference cycle. I know that the cycle will be broken when we
> set m_blobLoader to std::nullopt, but do we really need this object to keep
> itself alive like that? I guess maybe the answer is yes.

We need to keep alive WebServiceWorkerFetchTaskClient otherwise it will get deleted at the end of processing the fetch event response.
Comment 5 youenn fablet 2017-12-18 10:17:51 PST
(In reply to youenn fablet from comment #4)
> (In reply to Darin Adler from comment #3)
> > Comment on attachment 329544 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=329544&action=review
> > 
> > > Source/WebCore/platform/network/FormData.cpp:451
> > > +    URL blobURL;
> > > +    for (auto& element : m_elements) {
> > > +        if (element.m_type != FormDataElement::Type::EncodedBlob || !blobURL.isNull())
> > > +            return { };
> > > +        blobURL = element.m_url;
> > > +    }
> > > +    return blobURL;
> > 
> > I think this is a strange way to write this. I think the algorithm it
> > follows is: Return the URL only if there is only a single element of type
> > EncodedBlob. But it also can handle an encoded blob if it has a null URL and
> > skip over it. But is that feature important? Can we write it in without such
> > complex loop logic? Like a more straightforward "check that there is nothing
> > else except for a single encoded blob", then "get that first element since
> > we know it's the right kind of element now", and then "return the URL"?

Right, I'll change the implementation of this routine.
Comment 6 youenn fablet 2017-12-18 11:08:01 PST
Created attachment 329656 [details]
Patch for landing
Comment 7 youenn fablet 2017-12-18 11:09:30 PST
Addin a test with two blobs is not easy since we do not have support for DOMFormData in workers right now and we are not supporting well blob form data based uploads in service worker either.
Comment 8 EWS Watchlist 2017-12-18 11:10:27 PST
Attachment 329656 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:57:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:61:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 WebKit Commit Bot 2017-12-18 11:53:26 PST
Comment on attachment 329656 [details]
Patch for landing

Clearing flags on attachment: 329656

Committed r226066: <https://trac.webkit.org/changeset/226066>
Comment 10 WebKit Commit Bot 2017-12-18 11:53:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2017-12-18 11:54:31 PST
<rdar://problem/36112206>