Bug 202309 – Handle service worker loads through NetworkResourceLoader

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

[youenn fablet]

This is a first step towards simplifying service worker loads and optimise the ping pong we are doing for service worker matching.

[youenn fablet]

Created attachment 379721 [details]
Patch

[youenn fablet]

Created attachment 380647 [details]
Patch

[youenn fablet]

Created attachment 380734 [details]
Patch

[youenn fablet]

Created attachment 380741 [details]
Patch

[youenn fablet]

Created attachment 380747 [details]
Patch

[youenn fablet]

Created attachment 380748 [details]
Patch

[youenn fablet]

Created attachment 380749 [details]
Patch

[youenn fablet]

Created attachment 380751 [details]
Patch

[Alex Christensen]

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.

[youenn fablet]

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

[youenn fablet]

Created attachment 380860 [details]
Patch

[youenn fablet]

Created attachment 380863 [details]
Patch

[Alex Christensen]

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?

[youenn fablet]

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

[youenn fablet]

Comment on attachment 380863 [details]
Patch

Cq+ing, I can update the patch for the fetch vector in a follow-up.

[WebKit Commit Bot]

Comment on attachment 380863 [details]
Patch

Clearing flags on attachment: 380863

Committed r251124: <https://trac.webkit.org/changeset/251124>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Seems to break again GTK bot, probably ENABLE(SERVICE_WORKER) issues.
Will look at it.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 380968 [details]
GTK build fix

[Radar WebKit Bug Importer]

<rdar://problem/56283603>

[WebKit Commit Bot]

Comment on attachment 380968 [details]
GTK build fix

Clearing flags on attachment: 380968

Committed r251126: <https://trac.webkit.org/changeset/251126>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

(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

[youenn fablet]

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