Bug 185274 – Allow WebResourceLoader to cancel a load served from a service worker

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

[youenn fablet]

Allow WebResourceLoader to cancel a load served from a service worker

[youenn fablet]

Created attachment 339546 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

Created attachment 339563 [details]
Patch

[Chris Dumez]

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?

[youenn fablet]

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.

[Chris Dumez]

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

[youenn fablet]

Created attachment 339757 [details]
Patch

[youenn fablet]

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

[youenn fablet]

Created attachment 339767 [details]
Patch

[youenn fablet]

Created attachment 339793 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

Comment on attachment 339793 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 339793 [details]
Patch

Clearing flags on attachment: 339793

Committed r231486: <https://trac.webkit.org/changeset/231486>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40060291>

[Ryan Haddad]

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

[Ryan Haddad]

Reverted r231486 for reason:

Caused service worker LayoutTest failures on macOS Debug WK2.

Committed r231532: <https://trac.webkit.org/changeset/231532>

[youenn fablet]

Created attachment 339954 [details]
Patch

[youenn fablet]

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

[youenn fablet]

Created attachment 339981 [details]
Patch

[youenn fablet]

Created attachment 339999 [details]
Fixing crash

[WebKit Commit Bot]

Comment on attachment 339999 [details]
Fixing crash

Clearing flags on attachment: 339999

Committed r231588: <https://trac.webkit.org/changeset/231588>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.