Bug 203055 – Remove mayHaveServiceWorkerRegisteredForOrigin

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Remove mayHaveServiceWorkerRegisteredForOrigin

[Alex Christensen]

Created attachment 381112 [details]
Patch

[Alex Christensen]

Note: I think this was originally introduced to prevent a storage process from being started on startup.  That reason for existence is gone since the storage process was merged into the network process.

[Chris Dumez]

Comment on attachment 381112 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        It used to be significant when more service worker logic was in the UIProcess, but now that it's in the network process,

It was in the StorageProcess.

This was an optimization by Youenn (I related the bugs). His claim was:
"This may help speeding up the load of the first page in the web process."

[Alex Christensen]

I'll update the ChangeLog before landing.

[Alex Christensen]

I have no idea why this affects only iOS

[youenn fablet]

Comment on attachment 381112 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Remove mayHaveServiceWorkerRegisteredForOrigin

I do not think we are ready yet to remove it.
Without it, we would go to network process to check for registration and wait for the registration result to start the load.
This is not great when we know there is no service worker.
The fact that StorageProcess is no longer there reduces the optimization benefit but there is still a benefit.

There are in fact two optimisations:
1. First optimization is to check for swOriginTable to see whether a service worker is registered for that origin.
2. Second optimization is, when swOriginTable is not yet initialised (which happens at the beginning of the web process), to check whether any service worker is registered.

I am working on moving the registration matching from DocumentLoader to NetworkResourceLoader.
At that point, we will be able to remove this optimization since everything will be done in NetworkProcess.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:1115
> +    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"sw://host/regularPageWithConnection.html"]];

This test is good to have.
Currently, we are checking registration before custom schemes.
When moving registration matching to network process, I will need to go back to web process in case registration is not matching and there is a custom scheme.
It would be simpler if custom schemes were loaded from NetworkProcess or if we were removing support for service workers interception of custom schemes.

[Alex Christensen]

(In reply to youenn fablet from comment #6)
> I am working on moving the registration matching from DocumentLoader to
> NetworkResourceLoader.
> At that point, we will be able to remove this optimization since everything
> will be done in NetworkProcess.
I look forward to it.  I guess I'm blocked on it.

> It would be simpler if custom schemes were loaded from NetworkProcess or if
> we were removing support for service workers interception of custom schemes.
I don't think there's anyone using service workers to intercept custom schemes.

[youenn fablet]

(In reply to Alex Christensen from comment #7)
> (In reply to youenn fablet from comment #6)
> > I am working on moving the registration matching from DocumentLoader to
> > NetworkResourceLoader.
> > At that point, we will be able to remove this optimization since everything
> > will be done in NetworkProcess.
> I look forward to it.  I guess I'm blocked on it.
> 
> > It would be simpler if custom schemes were loaded from NetworkProcess or if
> > we were removing support for service workers interception of custom schemes.
> I don't think there's anyone using service workers to intercept custom
> schemes.

Agree it is a bit of an edge case.
API tests are doing so right now to work around the lack of a HTTP server.

[Alex Christensen]

(In reply to youenn fablet from comment #8)
> (In reply to Alex Christensen from comment #7)
> > (In reply to youenn fablet from comment #6)
> > > I am working on moving the registration matching from DocumentLoader to
> > > NetworkResourceLoader.
> > > At that point, we will be able to remove this optimization since everything
> > > will be done in NetworkProcess.
> > I look forward to it.  I guess I'm blocked on it.
> > 
> > > It would be simpler if custom schemes were loaded from NetworkProcess or if
> > > we were removing support for service workers interception of custom schemes.
> > I don't think there's anyone using service workers to intercept custom
> > schemes.
> 
> Agree it is a bit of an edge case.
> API tests are doing so right now to work around the lack of a HTTP server.

Luckily we now have TCPServer.

[Alex Christensen]

(In reply to Alex Christensen from comment #9)
> Luckily we now have TCPServer.
I'll prepare a patch.

[Alex Christensen]

https://bugs.webkit.org/show_bug.cgi?id=203141

[youenn fablet]

> I am working on moving the r egistration matching from DocumentLoader to
> NetworkResourceLoader.

https://bugs.webkit.org/show_bug.cgi?id=203144

> At that point, we will be able to remove this optimization since everything
> will be done in NetworkProcess.

Removing the sworiginstore optimization might have a slight cost for some cases (appcache loads and memory cache loads). Should investigate to see whether this is fine.
We will be able to remove WebProcessPool::mayHaveRegisteredServiceWorkers.

[Alex Christensen]

Created attachment 381496 [details]
Patch

[youenn fablet]

Comment on attachment 381496 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:-509
> -    if (!provider.mayHaveServiceWorkerRegisteredForOrigin(origin)) {

Let's replace this one for now by:
if (!ServiceWorkerProvider::singleton().serviceWorkerConnection().mayHaveServiceWorkerRegisteredForOrigin(origin))
We should also remove existingServiceWorkerConnection() since we no longer care about the cost of creating a connection.

> Source/WebCore/workers/service/SWClientConnection.h:-80
> -    virtual bool mayHaveServiceWorkerRegisteredForOrigin(const SecurityOriginData&) const = 0;

Let's keep this one.

> Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:-116
> -bool WebSWClientConnection::mayHaveServiceWorkerRegisteredForOrigin(const SecurityOriginData& origin) const

Let's not remove this one for now.
It seems best to remove this mechanism in its own revision ,for regression analysis.
Also, when removing this one, we should remove more code when doing so in SWServer et al.

[youenn fablet]

Created attachment 381525 [details]
Patch

[youenn fablet]

Created attachment 381544 [details]
Patch

[Alex Christensen]

Created attachment 381550 [details]
retry ews

[Aakash Jain]

(In reply to Alex Christensen from comment #17)
> Created attachment 381550 [details]
> retry ews
EWS now has the ability to retry patches, just click the 'Retry failed builds' button. See https://lists.webkit.org/pipermail/webkit-dev/2019-October/030888.html

[Alex Christensen]

But does it have the ability to download builds from itself?

[Aakash Jain]

(In reply to Alex Christensen from comment #19)
> But does it have the ability to download builds from itself?
Currently some of the bots are facing network issues while trying to reach Amazon S3 https://lists.webkit.org/pipermail/webkit-dev/2019-October/030887.html

We are actively looking in the issue.

[Alex Christensen]

http://trac.webkit.org/r251439

[Radar WebKit Bug Importer]

<rdar://problem/56504295>

[Fujii Hironori]

Committed r251470: <https://trac.webkit.org/changeset/251470>