Bug 189422 – Move Service Worker Management from Storage Process to Network Process

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

[Sihui Liu]

Continue to shrink storage process.

[Saam Barati]

Does this mean we're going to run JavaScript in the Network Process?

[Sihui Liu]

Created attachment 350498 [details]
Patch

[Sihui Liu]

Created attachment 350529 [details]
Patch

[Alexey Proskuryakov]

Will service worker JavaScript code also be running in the Networking process?

Separate to that, adding more and more responsibilities to the Networking process is a dangerous approach. More code is more crashes, and a crash in Networking has particularly unpleasant user symptoms.

[youenn fablet]

(In reply to Alexey Proskuryakov from comment #4)
> Will service worker JavaScript code also be running in the Networking
> process?

No, this is just about moving code running in storage process to network process.

[Saam Barati]

(In reply to youenn fablet from comment #5)
> (In reply to Alexey Proskuryakov from comment #4)
> > Will service worker JavaScript code also be running in the Networking
> > process?
> 
> No, this is just about moving code running in storage process to network
> process.

Gotcha. What code already runs in the storage process?

[youenn fablet]

(In reply to Saam Barati from comment #6)
> (In reply to youenn fablet from comment #5)
> > (In reply to Alexey Proskuryakov from comment #4)
> > > Will service worker JavaScript code also be running in the Networking
> > > process?
> > 
> > No, this is just about moving code running in storage process to network
> > process.
> 
> Gotcha. What code already runs in the storage process?

Storage of service worker registrations (including scripts), handling of service worker instance lifetime and passing events between service worker instances and service worker clients.

[Saam Barati]

(In reply to youenn fablet from comment #7)
> (In reply to Saam Barati from comment #6)
> > (In reply to youenn fablet from comment #5)
> > > (In reply to Alexey Proskuryakov from comment #4)
> > > > Will service worker JavaScript code also be running in the Networking
> > > > process?
> > > 
> > > No, this is just about moving code running in storage process to network
> > > process.
> > 
> > Gotcha. What code already runs in the storage process?
> 
> Storage of service worker registrations (including scripts), handling of
> service worker instance lifetime and passing events between service worker
> instances and service worker clients.

Makes sense. It feels like if this is what’s happening this bug is slightly misnamed. It should probably specify something about the storage process

[Sihui Liu]

Created attachment 350605 [details]
Patch

[Sihui Liu]

Created attachment 350609 [details]
Patch

[Sihui Liu]

Created attachment 350636 [details]
Patch

[Chris Dumez]

Some build failures.

[youenn fablet]

Comment on attachment 350636 [details]
Patch

Some nits.

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:65
> +#if ENABLE(SERVICE_WORKER)

Do we need this #if?

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:758
> +void NetworkConnectionToWebProcess::establishSWServerConnection(PAL::SessionID sessionID, SWServerConnectionIdentifier& serverConnectionIdentifier)

I think we now go with SWServerConnectionIdentifier and not SWServerConnectionIdentifier&.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:193
> +    HashMap<WebCore::SWServerConnectionIdentifier, WeakPtr<WebSWServerConnection>> m_swConnections;

We usually do not mix members and methods.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1235
> +    auto result = m_swServers.add(sessionID, nullptr);

Can we try to use HashMap::ensure?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1372
> +void NetworkProcess::addServiceWorkerSession(PAL::SessionID sessionID, String& serviceWorkerRegistrationDirectory, SandboxExtension::Handle& handle)

String&& serviceWorkerRegistrationDirectory
const SandboxExtension::Handle&

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:41
> +    : SWServerToContextConnection(securityOrigin)

SWServerToContextConnection should probably take a SecurityOriginData&&

> Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:65
> +            (allow file-issue-extension (require-all (extension-class "com.apple.app-sandbox.read-write") (subpath path))))))

Can you explain why we need these sandbox changes?

[Sihui Liu]

Created attachment 350650 [details]
Patch

[Sihui Liu]

Comment on attachment 350636 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:65
>> +#if ENABLE(SERVICE_WORKER)
> 
> Do we need this #if?

It seems that we only need these two header files & these two files only have content when we enable service worker. So this #if can make this more clear.

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:758
>> +void NetworkConnectionToWebProcess::establishSWServerConnection(PAL::SessionID sessionID, SWServerConnectionIdentifier& serverConnectionIdentifier)
> 
> I think we now go with SWServerConnectionIdentifier and not SWServerConnectionIdentifier&.

Okay.

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:193
>> +    HashMap<WebCore::SWServerConnectionIdentifier, WeakPtr<WebSWServerConnection>> m_swConnections;
> 
> We usually do not mix members and methods.

Okay.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1235
>> +    auto result = m_swServers.add(sessionID, nullptr);
> 
> Can we try to use HashMap::ensure?

Okay.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1372
>> +void NetworkProcess::addServiceWorkerSession(PAL::SessionID sessionID, String& serviceWorkerRegistrationDirectory, SandboxExtension::Handle& handle)
> 
> String&& serviceWorkerRegistrationDirectory
> const SandboxExtension::Handle&

Okay.

>> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:41
>> +    : SWServerToContextConnection(securityOrigin)
> 
> SWServerToContextConnection should probably take a SecurityOriginData&&

In NetworkProcess::createNetworkConnectionToWebProcess, m_serverToContextConnections also takes SecurityOriginData&&. Do you think we need to make a change there?

>> Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:65
>> +            (allow file-issue-extension (require-all (extension-class "com.apple.app-sandbox.read-write") (subpath path))))))
> 
> Can you explain why we need these sandbox changes?

I compared the entitlements of storage process and network process, and found storage process had more permissions on "DARWIN_USER_CACHE_DIR", so I moved the code here.

[Sihui Liu]

Created attachment 350661 [details]
Patch

[EWS Watchlist]

Comment on attachment 350661 [details]
Patch

Attachment 350661 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9334358

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 350675 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 350661 [details]
Patch

Attachment 350661 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9334421

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 350679 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[youenn fablet]

> >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:65
> >> +#if ENABLE(SERVICE_WORKER)
> > 
> > Do we need this #if?
> 
> It seems that we only need these two header files & these two files only
> have content when we enable service worker. So this #if can make this more
> clear.

If it is not needed, it is best to remove these, the preprocessed file will otherwise have a #if embedded in a #if.
We probably include other service worker header files in the same cpp without the #if.

> >> Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:65
> >> +            (allow file-issue-extension (require-all (extension-class "com.apple.app-sandbox.read-write") (subpath path))))))
> > 
> > Can you explain why we need these sandbox changes?
> 
> I compared the entitlements of storage process and network process, and
> found storage process had more permissions on "DARWIN_USER_CACHE_DIR", so I
> moved the code here.

Maybe Brent would know the reason of these rules.
If these rules are not service worker related, I would tend to not add them.

[Sihui Liu]

(In reply to youenn fablet from comment #21)
> > >> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:65
> > >> +#if ENABLE(SERVICE_WORKER)
> > > 
> > > Do we need this #if?
> > 
> > It seems that we only need these two header files & these two files only
> > have content when we enable service worker. So this #if can make this more
> > clear.
> 
> If it is not needed, it is best to remove these, the preprocessed file will
> otherwise have a #if embedded in a #if.
> We probably include other service worker header files in the same cpp
> without the #if.
> 
Okay.

> > >> Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:65
> > >> +            (allow file-issue-extension (require-all (extension-class "com.apple.app-sandbox.read-write") (subpath path))))))
> > > 
> > > Can you explain why we need these sandbox changes?
> > 
> > I compared the entitlements of storage process and network process, and
> > found storage process had more permissions on "DARWIN_USER_CACHE_DIR", so I
> > moved the code here.
> 
> Maybe Brent would know the reason of these rules.
> If these rules are not service worker related, I would tend to not add them.
It seems to be unrelated to service worker. I will revert the change.

[Sihui Liu]

Created attachment 350742 [details]
Patch

[Sihui Liu]

Created attachment 350753 [details]
Patch

[youenn fablet]

Comment on attachment 350753 [details]
Patch

LGTM once bots are happy.
A few nits.

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

> Source/WebKit/NetworkProcess/NetworkProcess.messages.in:28
> +    CreateNetworkConnectionToWebProcess(bool isServiceWorkerProcess, struct WebCore::SecurityOriginData origin)

In the future, we might want to go with service worker instances run in a web process that also loads web pages, in which case this will break.
This is fine for now.

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWOriginStore.h:44
> +    void importComplete() final;

Can it be made private?

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1637
> +#if ENABLE(SERVICE_WORKER)

This is for non cocoa ports that do not support yet service workers.
Probably fine to have this change although untested.
Looking at the cocoa version, there is some redundant code for IDB and resolve directories.
The main difference seems cookie handling. A follow-up patch could try to share more code.

> Source/WebKit/WebProcess/Network/NetworkProcessConnection.h:32
> +#include "WebSWClientConnection.h"

Maybe a forward declaration of WebSWClientConnection will be all that is needed.

[Sihui Liu]

Comment on attachment 350753 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkProcess.messages.in:28
>> +    CreateNetworkConnectionToWebProcess(bool isServiceWorkerProcess, struct WebCore::SecurityOriginData origin)
> 
> In the future, we might want to go with service worker instances run in a web process that also loads web pages, in which case this will break.
> This is fine for now.

In that case, there will be only be one type of web process? Then we could make the change when we need to remove service worker process.

>> Source/WebKit/NetworkProcess/ServiceWorker/WebSWOriginStore.h:44
>> +    void importComplete() final;
> 
> Can it be made private?

It's used in SWServer::registrationStoreImportComplete().

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1637
>> +#if ENABLE(SERVICE_WORKER)
> 
> This is for non cocoa ports that do not support yet service workers.
> Probably fine to have this change although untested.
> Looking at the cocoa version, there is some redundant code for IDB and resolve directories.
> The main difference seems cookie handling. A follow-up patch could try to share more code.

I agree. Let's make optimizations in follow-up patch.

>> Source/WebKit/WebProcess/Network/NetworkProcessConnection.h:32
>> +#include "WebSWClientConnection.h"
> 
> Maybe a forward declaration of WebSWClientConnection will be all that is needed.

Okay.

[youenn fablet]

> >> Source/WebKit/NetworkProcess/NetworkProcess.messages.in:28
> >> +    CreateNetworkConnectionToWebProcess(bool isServiceWorkerProcess, struct WebCore::SecurityOriginData origin)
> > 
> > In the future, we might want to go with service worker instances run in a web process that also loads web pages, in which case this will break.
> > This is fine for now.
> 
> In that case, there will be only be one type of web process? Then we could
> make the change when we need to remove service worker process.

Yep.

> >> Source/WebKit/NetworkProcess/ServiceWorker/WebSWOriginStore.h:44
> >> +    void importComplete() final;
> > 
> > Can it be made private?
> 
> It's used in SWServer::registrationStoreImportComplete().

SWServer owns a SWOriginStore which declares importComplete as public.
WebSWOriginStore probably does not need to expose its overriding importComplete as public.

[Sihui Liu]

Created attachment 350775 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 350775 [details]
Patch for landing

Rejecting attachment 350775 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 350775, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
/ServiceWorker/WebSWServerToContextConnection.cpp
patching file Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h
patching file Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.messages.in
patching file Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm
patching file Source/WebKit/Shared/Storage/StorageProcessCreationParameters.cpp
patching file Source/WebKit/Shared/Storage/StorageProcessCreationParameters.h
patching file Source/WebKit/Shared/WebsiteDataStoreParameters.cpp
patching file Source/WebKit/Shared/WebsiteDataStoreParameters.h
patching file Source/WebKit/Sources.txt
patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp
rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp'
patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.h
rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.h'
patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp
rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp'
patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h
rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h'
patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.messages.in
rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.messages.in'
patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWServerToContextConnection.cpp
rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWServerToContextConnection.cpp'
patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWServerToContextConnection.h
rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWServerToContextConnection.h'
patching file Source/WebKit/StorageProcess/ServiceWorker/WebSWServerToContextConnection.messages.in
rm 'Source/WebKit/StorageProcess/ServiceWorker/WebSWServerToContextConnection.messages.in'
patching file Source/WebKit/StorageProcess/StorageProcess.cpp
patching file Source/WebKit/StorageProcess/StorageProcess.h
patching file Source/WebKit/StorageProcess/StorageProcess.messages.in
patching file Source/WebKit/StorageProcess/StorageToWebProcessConnection.cpp
patching file Source/WebKit/StorageProcess/StorageToWebProcessConnection.h
patching file Source/WebKit/StorageProcess/StorageToWebProcessConnection.messages.in
patching file Source/WebKit/StorageProcess/ios/StorageProcessIOS.mm
patching file Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp
Hunk #2 succeeded at 116 (offset -5 lines).
Hunk #3 succeeded at 203 (offset -5 lines).
Hunk #4 succeeded at 289 (offset -5 lines).
Hunk #5 succeeded at 346 (offset -5 lines).
Hunk #6 succeeded at 687 (offset -5 lines).
patching file Source/WebKit/UIProcess/Network/NetworkProcessProxy.h
Hunk #1 succeeded at 69 with fuzz 1.
Hunk #2 succeeded at 160 (offset -2 lines).
patching file Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in
patching file Source/WebKit/UIProcess/ServiceWorkerProcessProxy.cpp
patching file Source/WebKit/UIProcess/Storage/StorageProcessProxy.cpp
Hunk #1 succeeded at 135 (offset -5 lines).
Hunk #2 succeeded at 225 (offset -5 lines).
patching file Source/WebKit/UIProcess/Storage/StorageProcessProxy.h
Hunk #1 succeeded at 77 (offset -2 lines).
patching file Source/WebKit/UIProcess/Storage/StorageProcessProxy.messages.in
patching file Source/WebKit/UIProcess/WebProcessPool.cpp
Hunk #4 FAILED at 613.
Hunk #5 FAILED at 647.
2 out of 9 hunks FAILED -- saving rejects to file Source/WebKit/UIProcess/WebProcessPool.cpp.rej
patching file Source/WebKit/UIProcess/WebProcessPool.h
patching file Source/WebKit/UIProcess/WebProcessProxy.cpp
patching file Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm
patching file Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
patching file Source/WebKit/WebKit.xcodeproj/project.pbxproj
patching file Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp
patching file Source/WebKit/WebProcess/Network/NetworkProcessConnection.h
patching file Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp
patching file Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp
patching file Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h
patching file Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp
patching file Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp
patching file Source/WebKit/WebProcess/Storage/WebToStorageProcessConnection.cpp
patching file Source/WebKit/WebProcess/Storage/WebToStorageProcessConnection.h
patching file Source/WebKit/WebProcess/WebProcess.cpp
patching file Source/WebKit/WebProcess/WebProcess.h
patching file Source/WebKit/WebProcess/WebProcess.messages.in
patching file Tools/ChangeLog
patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/9346202

[Sihui Liu]

Created attachment 350790 [details]
Patch for landing

[Sihui Liu]

Created attachment 350791 [details]
Patch for landing

[EWS Watchlist]

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


ERROR: Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 46 files


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

[WebKit Commit Bot]

Comment on attachment 350791 [details]
Patch for landing

Clearing flags on attachment: 350791

Committed r236477: <https://trac.webkit.org/changeset/236477>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44775415>

[Sihui Liu]

Created attachment 351328 [details]
LoadingTimeTest

[Sihui Liu]

With this patch, it could take ~250ms less to load first web page with Service Worker.

[Ryosuke Niwa]

(In reply to Sihui Liu from comment #37)
> With this patch, it could take ~250ms less to load first web page with
> Service Worker.

Nice!

[Geoffrey Garen]

A Membuster A/B test reported a net memory savings of 0MB - 1.5MB. Unfortunately, the test is too noisy to be more specific.

[Alexey Proskuryakov]

Typically, some zones in breakdown are noisy, and others aren't. For a change like this, I expect a prominent change in a non-noisy zone.