Bug 195649 – Use a ServiceWorker process per registrable domain

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Use a ServiceWorker process per registrable domain instead of one per security origin. This is more in line with PSON and avoids launching too many processes.

[Chris Dumez]

Created attachment 364471 [details]
Patch

[EWS Watchlist]

Attachment 364471 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ServiceWorkerProcessProxy.cpp:55:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 364471 [details]
Patch

ping review?

[John Wilander]

Comment on attachment 364471 [details]
Patch

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

LGTM.

> Source/WebCore/workers/service/server/SWServer.cpp:513
> +    RegistrableDomain registrableDomain(data.scriptURL);

Maybe uniform initialization here?

[youenn fablet]

Comment on attachment 364471 [details]
Patch

LGTM.

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

> Source/WebCore/workers/service/server/SWServerToContextConnection.cpp:44
> +    static NeverDestroyed<HashMap<RegistrableDomain, SWServerToContextConnection*>> connectionsByOrigin;

Not an issue for this patch, but I wonder how this singleton interacts with multiple NetworkProcess instances.
Ideally, we would not rely on this singleton and instead rely on NetworkProcess::m_serverToContextConnections.

> Source/WebCore/workers/service/server/SWServerToContextConnection.h:31
>  #include "SecurityOriginData.h"

Can we remove this include?

> Source/WebCore/workers/service/server/SWServerWorker.cpp:55
> +    , m_registrableDomain(scriptURL)

Can we use m_data.scriptURL here instead of scriptURL?
We might want to make scriptURL a URL&& and move it as part of m_data construction.

[Chris Dumez]

Comment on attachment 364471 [details]
Patch

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

>> Source/WebCore/workers/service/server/SWServerToContextConnection.cpp:44
>> +    static NeverDestroyed<HashMap<RegistrableDomain, SWServerToContextConnection*>> connectionsByOrigin;
> 
> Not an issue for this patch, but I wonder how this singleton interacts with multiple NetworkProcess instances.
> Ideally, we would not rely on this singleton and instead rely on NetworkProcess::m_serverToContextConnections.

I think you are right but this should probably be dealt with separately.

[Chris Dumez]

Created attachment 364566 [details]
Patch

[EWS Watchlist]

Attachment 364566 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ServiceWorkerProcessProxy.cpp:55:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

I filed https://bugs.webkit.org/show_bug.cgi?id=195697 for the singleton issue to not forget about it.

[WebKit Commit Bot]

Comment on attachment 364566 [details]
Patch

Clearing flags on attachment: 364566

Committed r242905: <https://trac.webkit.org/changeset/242905>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48861502>