Bug 180894 – Add support for response blob given to fetch events

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

Add support for response blob given to fetch events

[youenn fablet]

Created attachment 329544 [details]
Patch

[EWS Watchlist]

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.

[Darin Adler]

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.

[youenn fablet]

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

[youenn fablet]

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

[youenn fablet]

Created attachment 329656 [details]
Patch for landing

[youenn fablet]

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.

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 329656 [details]
Patch for landing

Clearing flags on attachment: 329656

Committed r226066: <https://trac.webkit.org/changeset/226066>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36112206>