Bug 201643 – Partition processes running service workers by session ID

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

[youenn fablet]

Partition processes running service workers by session ID

[youenn fablet]

Created attachment 378461 [details]
WIP

[youenn fablet]

Created attachment 378557 [details]
Patch

[youenn fablet]

Created attachment 378636 [details]
Fix GTK

[youenn fablet]

Comment on attachment 378636 [details]
Fix GTK

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2391
> +                return;

I'll remove this weakThis, given the server is owned by the network process.

[Chris Dumez]

Comment on attachment 378636 [details]
Fix GTK

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

> Source/WebCore/platform/RegistrableDomainWithSessionID.h:33
> +class RegistrableDomainWithSessionID {

Not a single time are we accessing the data members of this struct, this is over-engineered IMO and should be a simple std::pair.

[Chris Dumez]

Comment on attachment 378636 [details]
Fix GTK

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2389
> +        auto value = makeUnique<SWServer>(makeUniqueRef<WebSWOriginStore>(), WTFMove(path), sessionID, [weakThis = makeWeakPtr(this), this](auto& domain, auto sessionID) {

Feels weird for the parameter to take in a sessionID given that it is called by the SWServer and the SWServer is associated to a single sessionID. I'd rather we capture the sessionID in the lambda here and not pass the sessionID as parameter to the lambda.

> Source/WebKit/NetworkProcess/NetworkProcess.h:300
> +    void createWorkerContextConnection(const WebCore::RegistrableDomain&, PAL::SessionID);

I know this is existing code but could we rename to establishWorkerContextConnection() since it is not returning anything?

> Source/WebKit/NetworkProcess/NetworkProcess.h:302
> +    WebCore::SWServer* existingSWServerForSession(PAL::SessionID sessionID) { return m_swServers.get(sessionID); }

swServerForSessionIfExists(), see https://trac.webkit.org/changeset/242273/webkit.

[youenn fablet]

Created attachment 378724 [details]
Patch

[youenn fablet]

(In reply to youenn fablet from comment #7)
> Created attachment 378724 [details]
> Patch

Updated according suggestions.
Some further refactoring can be done to stop passing sessionIDs where no longer strictly needed. I left that for follow-ups.

[Chris Dumez]

Comment on attachment 378724 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 378724 [details]
Patch

Clearing flags on attachment: 378724

Committed r249853: <https://trac.webkit.org/changeset/249853>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55350650>