Allow WebResourceLoader to cancel a load served from a service worker
Created attachment 339546 [details] Patch
Comment on attachment 339546 [details] Patch Attachment 339546 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7565022 New failing tests: fast/mediastream/delayed-permission-allowed.html
Created attachment 339556 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 339563 [details] Patch
Comment on attachment 339563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339563&action=review > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:203 > + m_fetches.remove(iterator); This is probably me being a bit paranoid but this looks a bit fragile. In theory, the call to cancel() above could invalidate the iterator. My suggestion would be to the: if (auto client = m_fetches.take(id)) client->cancel(); > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.h:104 > + HashMap<std::pair<SWServerConnectionIdentifier, FetchIdentifier>, Ref<ServiceWorkerFetch::Client>> m_fetches; I would suggest m_ongoingFetches. > Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp:151 > + SWSERVERCONNECTION_RELEASE_LOG_ERROR_IF_ALLOWED("startFetch: fetchIdentifier: %" PRIu64 " -> DidNotHandle because no active worker", fetchIdentifier.toUInt64()); FYI, we added ObjectIdentifier::loggingString() for these purposes. > Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp:152 > + m_contentConnection->send(Messages::ServiceWorkerClientFetch::DidNotHandle { }, fetchIdentifier.toUInt64()); This seems wrong. We should be passing FetchIdentifier types over IPC, not uint64_t types. > Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:265 > + m_connection->cancelFetch(m_identifier, m_serviceWorkerRegistrationIdentifier); Is it possible that the registration's active worker has changed by now? In such case, we would have started the fetch with one service worker and asked another one to cancel it, which probably would not work. > Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:160 > + if (!isMainThread()) { In which case are we not on the main thread? StorageProcess' service worker code is single threaded, no? > Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp:105 > + m_ongoingFetchTasks.take(fetchIdentifier.toUInt64()); Why isn't m_ongoingFetchTasks using FetchIdentifier as key?
Comment on attachment 339563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339563&action=review >> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp:151 >> + SWSERVERCONNECTION_RELEASE_LOG_ERROR_IF_ALLOWED("startFetch: fetchIdentifier: %" PRIu64 " -> DidNotHandle because no active worker", fetchIdentifier.toUInt64()); > > FYI, we added ObjectIdentifier::loggingString() for these purposes. OK >> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp:152 >> + m_contentConnection->send(Messages::ServiceWorkerClientFetch::DidNotHandle { }, fetchIdentifier.toUInt64()); > > This seems wrong. We should be passing FetchIdentifier types over IPC, not uint64_t types. This is the destination identifier, not a parameter of ServiceWorkerClientFetch::DidNotHandle. I can change the pattern in a follow-up to remove using destination identifier. I feel though that, ideally, destination identifier could be typed as well so that it takes directly the identifier. >> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:265 >> + m_connection->cancelFetch(m_identifier, m_serviceWorkerRegistrationIdentifier); > > Is it possible that the registration's active worker has changed by now? In such case, we would have started the fetch with one service worker and asked another one to cancel it, which probably would not work. This is precisely the reason why we switched a while ago from using worker identifier to using registration identifier. Registration identifier is stable for the whole fetch. >> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:160 >> + if (!isMainThread()) { > > In which case are we not on the main thread? StorageProcess' service worker code is single threaded, no? We are in the service worker process here and these methods can be called from the worker thread. >> Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp:105 >> + m_ongoingFetchTasks.take(fetchIdentifier.toUInt64()); > > Why isn't m_ongoingFetchTasks using FetchIdentifier as key? Sure, will do this.
(In reply to youenn fablet from comment #6) > Comment on attachment 339563 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339563&action=review > > >> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp:151 > >> + SWSERVERCONNECTION_RELEASE_LOG_ERROR_IF_ALLOWED("startFetch: fetchIdentifier: %" PRIu64 " -> DidNotHandle because no active worker", fetchIdentifier.toUInt64()); > > > > FYI, we added ObjectIdentifier::loggingString() for these purposes. > > OK > > >> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp:152 > >> + m_contentConnection->send(Messages::ServiceWorkerClientFetch::DidNotHandle { }, fetchIdentifier.toUInt64()); > > > > This seems wrong. We should be passing FetchIdentifier types over IPC, not uint64_t types. > > This is the destination identifier, not a parameter of > ServiceWorkerClientFetch::DidNotHandle. > I can change the pattern in a follow-up to remove using destination > identifier. > I feel though that, ideally, destination identifier could be typed as well > so that it takes directly the identifier. Oh, I missed that this was the destination. Seems fine then. > > >> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:265 > >> + m_connection->cancelFetch(m_identifier, m_serviceWorkerRegistrationIdentifier); > > > > Is it possible that the registration's active worker has changed by now? In such case, we would have started the fetch with one service worker and asked another one to cancel it, which probably would not work. > > This is precisely the reason why we switched a while ago from using worker > identifier to using registration identifier. > Registration identifier is stable for the whole fetch. I know, so this means we may start a fetch with a service worker, then ask another service worker to cancel that load. This seems wrong, no? > > >> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:160 > >> + if (!isMainThread()) { > > > > In which case are we not on the main thread? StorageProcess' service worker code is single threaded, no? > > We are in the service worker process here and these methods can be called > from the worker thread. Oh, this is in WebKit/WebProcess/Storage no WebKit/Storage :) > > >> Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp:105 > >> + m_ongoingFetchTasks.take(fetchIdentifier.toUInt64()); > > > > Why isn't m_ongoingFetchTasks using FetchIdentifier as key? > > Sure, will do this.
Created attachment 339757 [details] Patch
> > This is precisely the reason why we switched a while ago from using worker > > identifier to using registration identifier. > > Registration identifier is stable for the whole fetch. > > I know, so this means we may start a fetch with a service worker, then ask > another service worker to cancel that load. This seems wrong, no? Did discuss this directly with Chris. Here is some additional information for the record. It might happen that we will ask a service worker to cancel a load that it did not do (case is service worker being updated). This is fine as: - it will not break behavior of the new service worker - when shutting down the old service worker, all ActiveDOMObjects are canceled including fetches Also, as per spec, the task queue containing fetch events is on the registration.
Created attachment 339767 [details] Patch
Created attachment 339793 [details] Patch
Comment on attachment 339793 [details] Patch Attachment 339793 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7604274 New failing tests: http/tests/preload/onload_event.html
Created attachment 339804 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 339793 [details] Patch r=me
Comment on attachment 339793 [details] Patch Clearing flags on attachment: 339793 Committed r231486: <https://trac.webkit.org/changeset/231486>
All reviewed patches have been landed. Closing bug.
<rdar://problem/40060291>
This change caused service worker LayoutTest failures and timeouts on macOS Debug WK2. https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r231493%20(3185)/results.html There are "Other Crashes" in the linked test run that seem to be failing ASSERT(m_connection) in WebKit::WebServiceWorkerFetchTaskClient::cleanup()
Reverted r231486 for reason: Caused service worker LayoutTest failures on macOS Debug WK2. Committed r231532: <https://trac.webkit.org/changeset/231532>
Created attachment 339954 [details] Patch
(In reply to Ryan Haddad from comment #19) > Reverted r231486 for reason: > > Caused service worker LayoutTest failures on macOS Debug WK2. > > Committed r231532: <https://trac.webkit.org/changeset/231532> Issue is probably the buggy Debug assertion on m_connection. This assertion is not true after hopping to the main thread.
Created attachment 339981 [details] Patch
Created attachment 339999 [details] Fixing crash
Comment on attachment 339999 [details] Fixing crash Clearing flags on attachment: 339999 Committed r231588: <https://trac.webkit.org/changeset/231588>