Bug 185274 - Allow WebResourceLoader to cancel a load served from a service worker
Summary: Allow WebResourceLoader to cancel a load served from a service worker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-03 16:06 PDT by youenn fablet
Modified: 2018-05-09 12:59 PDT (History)
8 users (show)

See Also:


Attachments
Patch (62.34 KB, patch)
2018-05-04 08:36 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.72 MB, application/zip)
2018-05-04 09:52 PDT, EWS Watchlist
no flags Details
Patch (62.33 KB, patch)
2018-05-04 10:30 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (63.00 KB, patch)
2018-05-07 15:10 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (62.94 KB, patch)
2018-05-07 16:40 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (62.95 KB, patch)
2018-05-07 20:26 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.89 MB, application/zip)
2018-05-08 00:54 PDT, EWS Watchlist
no flags Details
Patch (63.09 KB, patch)
2018-05-09 04:45 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (63.08 KB, patch)
2018-05-09 10:33 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing crash (63.01 KB, patch)
2018-05-09 12:14 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-05-03 16:06:38 PDT
Allow WebResourceLoader to cancel a load served from a service worker
Comment 1 youenn fablet 2018-05-04 08:36:56 PDT
Created attachment 339546 [details]
Patch
Comment 2 EWS Watchlist 2018-05-04 09:52:21 PDT
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
Comment 3 EWS Watchlist 2018-05-04 09:52:23 PDT
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
Comment 4 youenn fablet 2018-05-04 10:30:14 PDT
Created attachment 339563 [details]
Patch
Comment 5 Chris Dumez 2018-05-07 13:42:34 PDT
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 6 youenn fablet 2018-05-07 14:51:11 PDT
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.
Comment 7 Chris Dumez 2018-05-07 15:02:46 PDT
(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.
Comment 8 youenn fablet 2018-05-07 15:10:24 PDT
Created attachment 339757 [details]
Patch
Comment 9 youenn fablet 2018-05-07 16:39:43 PDT
> > 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.
Comment 10 youenn fablet 2018-05-07 16:40:10 PDT
Created attachment 339767 [details]
Patch
Comment 11 youenn fablet 2018-05-07 20:26:26 PDT
Created attachment 339793 [details]
Patch
Comment 12 EWS Watchlist 2018-05-08 00:54:16 PDT
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
Comment 13 EWS Watchlist 2018-05-08 00:54:27 PDT
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 14 Chris Dumez 2018-05-08 08:45:42 PDT
Comment on attachment 339793 [details]
Patch

r=me
Comment 15 WebKit Commit Bot 2018-05-08 09:26:59 PDT
Comment on attachment 339793 [details]
Patch

Clearing flags on attachment: 339793

Committed r231486: <https://trac.webkit.org/changeset/231486>
Comment 16 WebKit Commit Bot 2018-05-08 09:27:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2018-05-08 09:28:34 PDT
<rdar://problem/40060291>
Comment 18 Ryan Haddad 2018-05-08 17:36:07 PDT
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()
Comment 19 Ryan Haddad 2018-05-08 17:40:35 PDT
Reverted r231486 for reason:

Caused service worker LayoutTest failures on macOS Debug WK2.

Committed r231532: <https://trac.webkit.org/changeset/231532>
Comment 20 youenn fablet 2018-05-09 04:45:27 PDT
Created attachment 339954 [details]
Patch
Comment 21 youenn fablet 2018-05-09 04:46:20 PDT
(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.
Comment 22 youenn fablet 2018-05-09 10:33:26 PDT
Created attachment 339981 [details]
Patch
Comment 23 youenn fablet 2018-05-09 12:14:54 PDT
Created attachment 339999 [details]
Fixing crash
Comment 24 WebKit Commit Bot 2018-05-09 12:59:52 PDT
Comment on attachment 339999 [details]
Fixing crash

Clearing flags on attachment: 339999

Committed r231588: <https://trac.webkit.org/changeset/231588>
Comment 25 WebKit Commit Bot 2018-05-09 12:59:54 PDT
All reviewed patches have been landed.  Closing bug.