Bug 201643 - Partition processes running service workers by session ID
Summary: Partition processes running service workers by session ID
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:
Blocks:
 
Reported: 2019-09-10 07:16 PDT by youenn fablet
Modified: 2019-09-13 13:37 PDT (History)
11 users (show)

See Also:


Attachments
WIP (50.24 KB, patch)
2019-09-10 07:40 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (57.48 KB, patch)
2019-09-11 05:41 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fix GTK (59.16 KB, patch)
2019-09-12 02:37 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (49.26 KB, patch)
2019-09-13 05:06 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-10 07:16:22 PDT
Partition processes running service workers by session ID
Comment 1 youenn fablet 2019-09-10 07:40:38 PDT
Created attachment 378461 [details]
WIP
Comment 2 youenn fablet 2019-09-11 05:41:30 PDT
Created attachment 378557 [details]
Patch
Comment 3 youenn fablet 2019-09-12 02:37:55 PDT
Created attachment 378636 [details]
Fix GTK
Comment 4 youenn fablet 2019-09-12 04:34:20 PDT
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.
Comment 5 Chris Dumez 2019-09-12 08:26:25 PDT
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.
Comment 6 Chris Dumez 2019-09-12 08:40:12 PDT
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.
Comment 7 youenn fablet 2019-09-13 05:06:45 PDT
Created attachment 378724 [details]
Patch
Comment 8 youenn fablet 2019-09-13 05:09:39 PDT
(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.
Comment 9 Chris Dumez 2019-09-13 08:23:37 PDT
Comment on attachment 378724 [details]
Patch

r=me
Comment 10 WebKit Commit Bot 2019-09-13 13:36:14 PDT
Comment on attachment 378724 [details]
Patch

Clearing flags on attachment: 378724

Committed r249853: <https://trac.webkit.org/changeset/249853>
Comment 11 WebKit Commit Bot 2019-09-13 13:36:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-09-13 13:37:20 PDT
<rdar://problem/55350650>