Summary: | Create a Fetch event when ServiceWorker has to handle a fetch | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, beidson, buildbot, cdumez, commit-queue, esprehn+autocc, jlewis3, kondapallykalyan, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=178685 | ||||||||||||||||||||||
Bug Depends on: | 178475 | ||||||||||||||||||||||
Bug Blocks: | 178534 | ||||||||||||||||||||||
Attachments: |
|
Description
youenn fablet
2017-10-18 17:25:32 PDT
Created attachment 324184 [details]
Patch
Created attachment 324240 [details]
Patch
Created attachment 324281 [details]
Patch
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. 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. Created attachment 324308 [details]
Patch
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. 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? (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... Created attachment 324313 [details]
Patch
Patch does not apply. Created attachment 324317 [details]
Patch
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 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
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? (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. (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. > 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.
Created attachment 324347 [details]
Patch for landing
Created attachment 324561 [details]
Rebasing
Comment on attachment 324561 [details] Rebasing Clearing flags on attachment: 324561 Committed r223839: <https://trac.webkit.org/changeset/223839> All reviewed patches have been landed. Closing bug. 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 |