Bug 202309 - Handle service worker loads through NetworkResourceLoader
Summary: Handle service worker loads through NetworkResourceLoader
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: 202195
Blocks:
  Show dependency treegraph
 
Reported: 2019-09-27 07:10 PDT by youenn fablet
Modified: 2019-12-13 15:22 PST (History)
16 users (show)

See Also:


Attachments
Patch (100.03 KB, patch)
2019-09-27 07:31 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (100.76 KB, patch)
2019-10-10 08:40 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (104.41 KB, patch)
2019-10-11 01:43 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (104.91 KB, patch)
2019-10-11 03:51 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (105.00 KB, patch)
2019-10-11 06:12 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (105.03 KB, patch)
2019-10-11 06:57 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (105.05 KB, patch)
2019-10-11 07:04 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (105.12 KB, patch)
2019-10-11 07:11 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (106.33 KB, patch)
2019-10-14 01:08 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (106.17 KB, patch)
2019-10-14 02:35 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
GTK build fix (2.31 KB, patch)
2019-10-15 01:16 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 2019-09-27 07:10:06 PDT
This is a first step towards simplifying service worker loads and optimise the ping pong we are doing for service worker matching.
Comment 1 youenn fablet 2019-09-27 07:31:39 PDT
Created attachment 379721 [details]
Patch
Comment 2 youenn fablet 2019-10-10 08:40:48 PDT
Created attachment 380647 [details]
Patch
Comment 3 youenn fablet 2019-10-11 01:43:40 PDT
Created attachment 380734 [details]
Patch
Comment 4 youenn fablet 2019-10-11 03:51:12 PDT
Created attachment 380741 [details]
Patch
Comment 5 youenn fablet 2019-10-11 06:12:14 PDT
Created attachment 380747 [details]
Patch
Comment 6 youenn fablet 2019-10-11 06:57:42 PDT
Created attachment 380748 [details]
Patch
Comment 7 youenn fablet 2019-10-11 07:04:10 PDT
Created attachment 380749 [details]
Patch
Comment 8 youenn fablet 2019-10-11 07:11:29 PDT
Created attachment 380751 [details]
Patch
Comment 9 Alex Christensen 2019-10-11 13:35:06 PDT
Comment on attachment 380751 [details]
Patch

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

> Source/WebCore/workers/service/server/SWServer.cpp:977
> +bool SWServer::canHandleScheme(const String& scheme) const

I would make this take a StringView, then have a fast path if m_registeredSchemes.isEmpty() that doesn't make a String or hash it at all.  That will reduce a string construction and hash in the common non-testing case.

> Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:265
> +    Optional<HashSet<HTTPHeaderName, WTF::IntHash<HTTPHeaderName>, WTF::StrongEnumHashTraits<HTTPHeaderName>>> httpHeadersToKeep;

I think you should typedef this HashSet to avoid repeating so many template parameters.
Also, would it be worth just sending a Vector, or is hashing it worth it?

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:111
> -void ServiceWorkerFetchTask::didReceiveResponse(const ResourceResponse& response, bool needsContinueDidReceiveResponseMessage)
> +void ServiceWorkerFetchTask::didReceiveResponse(ResourceResponse&& response, bool needsContinueDidReceiveResponseMessage)

I don't see why you're moving a response into this function, which only sends it over IPC.  It seems unnecessary.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:130
> -    if (m_connection)
> -        m_connection->send(Messages::ServiceWorkerClientFetch::DidReceiveFormData { formData }, m_identifier.fetchIdentifier);
> +    // FIXME: Allow WebResourceLoader to receive form data.

This seems like a regression as it stands now.
Comment 10 youenn fablet 2019-10-13 12:16:55 PDT
(In reply to Alex Christensen from comment #9)
> Comment on attachment 380751 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380751&action=review
> 
> > Source/WebCore/workers/service/server/SWServer.cpp:977
> > +bool SWServer::canHandleScheme(const String& scheme) const
> 
> I would make this take a StringView, then have a fast path if
> m_registeredSchemes.isEmpty() that doesn't make a String or hash it at all. 
> That will reduce a string construction and hash in the common non-testing
> case.

I hesitated and went with const String& since this was the way it was done before.
Will change to StringView.

> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:265
> > +    Optional<HashSet<HTTPHeaderName, WTF::IntHash<HTTPHeaderName>, WTF::StrongEnumHashTraits<HTTPHeaderName>>> httpHeadersToKeep;
> 
> I think you should typedef this HashSet to avoid repeating so many template
> parameters.

Should use HTTPHeaderNameSet.

> Also, would it be worth just sending a Vector, or is hashing it worth it?

Right, we could probably use a Vector, this should be done independently though since this should be changed in DocumntThreadableLoader et al.

> > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:111
> > -void ServiceWorkerFetchTask::didReceiveResponse(const ResourceResponse& response, bool needsContinueDidReceiveResponseMessage)
> > +void ServiceWorkerFetchTask::didReceiveResponse(ResourceResponse&& response, bool needsContinueDidReceiveResponseMessage)
> 
> I don't see why you're moving a response into this function, which only
> sends it over IPC.  It seems unnecessary.

We cannot use a const since we are modifying the response.
a ResourceResponse& is sufficient though.

> 
> > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:130
> > -    if (m_connection)
> > -        m_connection->send(Messages::ServiceWorkerClientFetch::DidReceiveFormData { formData }, m_identifier.fetchIdentifier);
> > +    // FIXME: Allow WebResourceLoader to receive form data.
> 
> This seems like a regression as it stands now.

It is not since current code base has a FIXME for that in ServiceWorkerClientFetch::didReceiveFormData.
Comment 11 youenn fablet 2019-10-14 01:08:44 PDT
Created attachment 380860 [details]
Patch
Comment 12 youenn fablet 2019-10-14 02:35:12 PDT
Created attachment 380863 [details]
Patch
Comment 13 Alex Christensen 2019-10-14 15:39:50 PDT
Comment on attachment 380863 [details]
Patch

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

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:178
> +    Vector<ServiceWorkerFetchTask*> fetches;

Since we're no longer mutating m_ongoingFetches while iterating, could we remove the use of the Vector?  If not, could we make this a safer pointer type?  If not, could we at least use reserveInitialCapacity/uncheckedAppend?
Comment 14 youenn fablet 2019-10-14 23:11:53 PDT
(In reply to Alex Christensen from comment #13)
> Comment on attachment 380863 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380863&action=review
> 
> > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:178
> > +    Vector<ServiceWorkerFetchTask*> fetches;
> 
> Since we're no longer mutating m_ongoingFetches while iterating, could we
> remove the use of the Vector?  If not, could we make this a safer pointer
> type?  If not, could we at least use reserveInitialCapacity/uncheckedAppend?

Creating a Vector is safer as calling fail/didNotHandle will mutate m_ongoingFetches.

We could use a Vector<WeakPtr<>> but we would need to increment the weak ptr ref or move it from the map to the vector. Let me know if you prefer that.

We could use reserveInitialCapacity but we might over-allocate since only the fetches of one worker only will be put in the vector. We usually do not do that.

We could also simply copyToVector the map and iterate over it.
Probably not a big deal for perf since the time out should happen rarely.
Comment 15 youenn fablet 2019-10-14 23:15:57 PDT
Comment on attachment 380863 [details]
Patch

Cq+ing, I can update the patch for the fetch vector in a follow-up.
Comment 16 WebKit Commit Bot 2019-10-14 23:59:53 PDT
Comment on attachment 380863 [details]
Patch

Clearing flags on attachment: 380863

Committed r251124: <https://trac.webkit.org/changeset/251124>
Comment 17 WebKit Commit Bot 2019-10-14 23:59:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 youenn fablet 2019-10-15 01:13:04 PDT
Seems to break again GTK bot, probably ENABLE(SERVICE_WORKER) issues.
Will look at it.
Comment 19 youenn fablet 2019-10-15 01:16:18 PDT
Reopening to attach new patch.
Comment 20 youenn fablet 2019-10-15 01:16:21 PDT
Created attachment 380968 [details]
GTK build fix
Comment 21 Radar WebKit Bug Importer 2019-10-15 01:16:26 PDT
<rdar://problem/56283603>
Comment 22 WebKit Commit Bot 2019-10-15 01:59:18 PDT
Comment on attachment 380968 [details]
GTK build fix

Clearing flags on attachment: 380968

Committed r251126: <https://trac.webkit.org/changeset/251126>
Comment 23 WebKit Commit Bot 2019-10-15 01:59:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Carlos Alberto Lopez Perez 2019-10-16 08:57:56 PDT
(In reply to WebKit Commit Bot from comment #16)
> Comment on attachment 380863 [details]
> Patch
> 
> Clearing flags on attachment: 380863
> 
> Committed r251124: <https://trac.webkit.org/changeset/251124>

Seems this has caused lot of crashes in service-workers tests on GTK.

The bot is aborting early.

I will try to get a backtrack.
Comment 25 Carlos Alberto Lopez Perez 2019-10-16 09:15:11 PDT
(In reply to Carlos Alberto Lopez Perez from comment #24)
> (In reply to WebKit Commit Bot from comment #16)
> > Comment on attachment 380863 [details]
> > Patch
> > 
> > Clearing flags on attachment: 380863
> > 
> > Committed r251124: <https://trac.webkit.org/changeset/251124>
> 
> Seems this has caused lot of crashes in service-workers tests on GTK.
> 
> The bot is aborting early.
> 
> I will try to get a backtrack.

Reportad that on bug 203036
Comment 26 youenn fablet 2019-10-16 09:16:33 PDT
(In reply to Carlos Alberto Lopez Perez from comment #25)
> (In reply to Carlos Alberto Lopez Perez from comment #24)
> > (In reply to WebKit Commit Bot from comment #16)
> > > Comment on attachment 380863 [details]
> > > Patch
> > > 
> > > Clearing flags on attachment: 380863
> > > 
> > > Committed r251124: <https://trac.webkit.org/changeset/251124>
> > 
> > Seems this has caused lot of crashes in service-workers tests on GTK.
> > 
> > The bot is aborting early.
> > 
> > I will try to get a backtrack.
> 
> Reportad that on bug 203036

I'll get a look tomorrow.