Bug 178491 – Create a Fetch event when ServiceWorker has to handle a fetch

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Create a Fetch event when ServiceWorker has to handle a fetch

[youenn fablet]

Created attachment 324184 [details]
Patch

[youenn fablet]

Created attachment 324240 [details]
Patch

[youenn fablet]

Created attachment 324281 [details]
Patch

[Chris Dumez]

Comment on attachment 324281 [details]
Patch

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

> Source/WebCore/workers/service/FetchEvent.h:58
> +    void cleanOnResponseCallback() { m_onResponse = nullptr; }

I think clear would be better than clean.

> Source/WebCore/workers/service/context/ServiceWorkerFetchTask.cpp:53
> +    event->onResponse([client = WTFMove(client)] (FetchResponse* response) mutable {

Should the FetchResponse be passed as a RefPtr<FetchResponse>&&? What guarantees it stays alive otherwise?

> Source/WebCore/workers/service/context/ServiceWorkerFetchTask.cpp:57
> +    globalScope.dispatchEvent(event.get());

I do not believe you need the .get().

> Source/WebCore/workers/service/context/ServiceWorkerFetchTask.cpp:60
> +void ServiceWorkerFetchTask::processResponse(Ref<Client>&& client, FetchResponse* response)

RefPtr<>&& for the response?

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:101
> +        ASSERT(context.isWorkerGlobalScope());

Not needed, this is embedded in the downcast<> below.

> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:65
> +    RunLoop::main().dispatch([buffer = WTFMove(buffer), protectedThis = makeRef(*this), this] () mutable {

SharedBuffer is not ThreadSafeRefCounted.

> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:68
> +        IPC::DataReference dataReference(reinterpret_cast<const uint8_t*>(buffer->data()), buffer->size());

There is a SharedBufferDataReference class for such purposes I think.

> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:52
> +    uint64_t m_serverConnectionIdentifier { 0 };

Does not need initializer.

> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:53
> +    uint64_t m_fetchTaskIdentifier { 0 };

Does not need initializer.

[Chris Dumez]

Comment on attachment 324281 [details]
Patch

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

> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:56
> +    RunLoop::main().dispatch([responseData = response.crossThreadData(), protectedThis = makeRef(*this), this] () mutable {

We do not actually need to dispatch to the main thread to do IPC. Calling IPC::Connection::send() from a background thread is safe AFAIK.

[youenn fablet]

Created attachment 324308 [details]
Patch

[youenn fablet]

Updated patch according your comments.


> Should the FetchResponse be passed as a RefPtr<FetchResponse>&&? What
> guarantees it stays alive otherwise?

I added a protector for added safety although at the moment, this is not needed.
 
> > Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:52
> > +    uint64_t m_serverConnectionIdentifier { 0 };
> 
> Does not need initializer.

I kept the initializer.
Although it is not strictly necessary, this seems safer.

[Chris Dumez]

Comment on attachment 324308 [details]
Patch

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

> Source/WebCore/workers/service/context/ServiceWorkerFetchTask.h:41
> +class ServiceWorkerFetchTask {

This does not have any method. Maybe this should be renamed to ServiceWorkerFetch and made a namespace instead of a class?

[Chris Dumez]

(In reply to youenn fablet from comment #7)
> Updated patch according your comments.
> 
> 
> > Should the FetchResponse be passed as a RefPtr<FetchResponse>&&? What
> > guarantees it stays alive otherwise?
> 
> I added a protector for added safety although at the moment, this is not
> needed.
>  
> > > Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:52
> > > +    uint64_t m_serverConnectionIdentifier { 0 };
> > 
> > Does not need initializer.
> 
> I kept the initializer.
> Although it is not strictly necessary, this seems safer.

There is no default constructor and it is always initialized by the constructor...

[youenn fablet]

Created attachment 324313 [details]
Patch

[Chris Dumez]

Patch does not apply.

[youenn fablet]

Created attachment 324317 [details]
Patch

[Build Bot]

Comment on attachment 324317 [details]
Patch

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

New failing tests:
http/wpt/service-workers/fetchEvent.https.html

[Build Bot]

Created attachment 324329 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Comment on attachment 324317 [details]
Patch

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

r=me with comments.

> Source/WebCore/platform/network/ResourceResponseBase.h:70
> +    WEBCORE_EXPORT CrossThreadData crossThreadData() const;

No longer needed.

> Source/WebCore/platform/network/ResourceResponseBase.h:71
> +    WEBCORE_EXPORT static ResourceResponse fromCrossThreadData(CrossThreadData&&);

No longer needed.

> Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp:27
> +#include "ServiceWorkerFetch.h"

We usually put this inside the #if ENABLE().

> Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp:64
> +void processResponse(Ref<Client>&& client, FetchResponse* response)

Why the declaration at the top? I think we usually put the static functions at the top of the file to avoid needing such declarations.

> Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp:110
> +}

// namespace ServiceWorkerFetch

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:96
> +void ServiceWorkerThread::dispatchFetchTask(Ref<ServiceWorkerFetch::Client>&& client, ResourceRequest&& request, FetchOptions&& options)

Why dispatchFetchTask and not dispatchFetchEvent ?

> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:27
> +#include "WebServiceWorkerFetchTaskClient.h"

This usually goes inside the #if ENABLE()

> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:41
> +    ~WebServiceWorkerFetchTaskClient();

I think we should add a blank line before.

> LayoutTests/http/tests/workers/service/resources/basic-fetch.js:6
> +function log(msg)

We have this in 2 tests now (basic-fetch.js and basic-register.js). Could you please move this function to sw-test-pre.js to avoid code duplication?

[Chris Dumez]

(In reply to Build Bot from comment #13)
> Comment on attachment 324317 [details]
> Patch
> 
> Attachment 324317 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: http://webkit-queues.webkit.org/results/4926645
> 
> New failing tests:
> http/wpt/service-workers/fetchEvent.https.html

Please deal with this failure too.

[youenn fablet]

(In reply to Chris Dumez from comment #16)
> (In reply to Build Bot from comment #13)
> > Comment on attachment 324317 [details]
> > Patch
> > 
> > Attachment 324317 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> > Output: http://webkit-queues.webkit.org/results/4926645
> > 
> > New failing tests:
> > http/wpt/service-workers/fetchEvent.https.html
> 
> Please deal with this failure too.

Will add an Internals method to create a being dispatched fetch event.

[youenn fablet]

> Why dispatchFetchTask and not dispatchFetchEvent ?

To align with the spec that has the notion of a queue of tasks and says "queue a task" in "Handle Fetch".
I'll change it to postFetchTask as dispatch is too much related to event.

[youenn fablet]

Created attachment 324347 [details]
Patch for landing

[youenn fablet]

Created attachment 324561 [details]
Rebasing

[WebKit Commit Bot]

Comment on attachment 324561 [details]
Rebasing

Clearing flags on attachment: 324561

Committed r223839: <https://trac.webkit.org/changeset/223839>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

The most recent patch has caused several tests to start crashing on iOS and timing out on Sierra. The crashes however are coming up as invalid crashes on the results.

build:
https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r223839%20(557)/results.html
https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/builds/557

History of related tests that are crashing:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fcache-storage%2Fserviceworker%2Fcache-delete.https.html
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fcache-storage%2Fserviceworker%2Fcache-match.https.html
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fcache-storage%2Fserviceworker%2Fcache-storage-keys.https.html
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fcache-storage%2Fserviceworker%2Fcache-storage.https.html
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fxmlhttprequest%2Fworkers%2Fabort-exception-assert.html

[Radar WebKit Bug Importer]

<rdar://problem/35569021>