Bug 202195 – Reuse existing web processes for running service workers

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Reuse existing web processes for running service workers

[youenn fablet]

Created attachment 379534 [details]
Patch

[youenn fablet]

Created attachment 379540 [details]
Fix API tests

[youenn fablet]

Created attachment 379630 [details]
Patch

[Geoffrey Garen]

Comment on attachment 379630 [details]
Patch

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

> Source/WebKit/ChangeLog:26
> +        We no longer terminate the web process when stopping service workers or when network process crash.

Is a runaway ServiceWorker still terminated when it has no clients?

[youenn fablet]

(In reply to Geoffrey Garen from comment #4)
> Comment on attachment 379630 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379630&action=review
> 
> > Source/WebKit/ChangeLog:26
> > +        We no longer terminate the web process when stopping service workers or when network process crash.
> 
> Is a runaway ServiceWorker still terminated when it has no clients?

Yes, we reuse the existing way to terminate service workers which if stop is not done after a timeout will make the process exit.

This is ok in that particular case since there is no client/page in the given process.
In other cases, like when marking a worker as redundant, we might actually exit the process that is running some pages.

This seems ok to me since we are partitioning by registrable domain and a bad service worker might anyway make the pages disfunction as well.

[youenn fablet]

Ping review?

[Alex Christensen]

Comment on attachment 379630 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessProxy.cpp:1445
> +        disableServiceWorkers();

I don't understand why this is here.

> LayoutTests/ChangeLog:3
> +        ServiceWorkerFetchTask can use the NetworkConnectionToWebProcess sessionID

Inconsistent ChangeLog title.

[youenn fablet]

Comment on attachment 379630 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessProxy.cpp:1445
>> +        disableServiceWorkers();
> 
> I don't understand why this is here.

This is not strictly necessary.

AIUI, this code means that we are about to run JavaScript from various domains in this process.
In that case, it seems better to stop all service workers running in this process.
They will be relaunched in a process with a matching registrable domain.

[Brady Eidson]

Comment on attachment 379630 [details]
Patch

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

This mostly seems good.

If we are writing code that says "an existing web process can be reused for a service worker later", then I think we should have at least one API test that verifies that precise behavior.

> Source/WebCore/workers/service/context/SWContextManager.h:98
> +    static constexpr Seconds workerTerminationTimeoutForTest { 100_ms }; // Only used by layout tests.

I don't think workerTerminationTimeoutForTest is a better name than syncWorkerTerminationTimeout

Sync termination is not only about testing.

[youenn fablet]

Created attachment 380209 [details]
Patch

[youenn fablet]

Created attachment 380216 [details]
Patch

[youenn fablet]

Created attachment 380221 [details]
Patch

[youenn fablet]

Ping review

[Chris Dumez]

Comment on attachment 380221 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:5237
> +int Internals::processId() const

I think this should either be processID or processIdentifier, I have a preference for the latter.

> Source/WebCore/workers/service/context/SWContextManager.h:72
> +        bool m_isStopped { false };

Protected data members are not great practiced. I think this should be private. If it needs to be set from a subclass, then introduce a protected setter.

> Source/WebCore/workers/service/context/SWContextManager.h:98
> +    static constexpr Seconds syncWorkerTerminationTimeout { 100_ms }; // Only used by layout tests.

Doesn't Brady's patch from a couple days ago use sync worker termination is non-testing cases, when a service worker that not handle a fetch in time?

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:895
> +    RELEASE_LOG(ServiceWorker, "WebProcess %llu no longer useful for running service workers", webProcessIdentifier().toUInt64());

We should not be logging in ephemeral sessions. NetworkConnectionToWebProcess has a sessionID for you to do the check.

> Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:114
> +- (void)_setForceSeparateServiceWorkerProcess:(BOOL)forceServiceWorkerProcess WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Why do we need this? Having to support both separate and merged models increases code complexity.

> Source/WebKit/UIProcess/WebProcessPool.cpp:729
> +            RELEASE_LOG(ServiceWorker, "WebProcessPool::establishWorkerContextConnectionToNetworkProcess reusing an existing web process %p", serviceWorkerProcessProxy);

Please log PID too.

> Source/WebKit/UIProcess/WebProcessPool.cpp:738
> +        RELEASE_LOG(ServiceWorker, "WebProcessPool::establishWorkerContextConnectionToNetworkProcess creating a new service worker process %p", serviceWorkerProcessProxy);

Please log PID too.

> Source/WebKit/UIProcess/WebProcessPool.cpp:755
> +    RegistrableDomainWithSessionID key { process.registrableDomain(), process.websiteDataStore().sessionID() };

The process' registrable domain can change throughout the lifetime of a WebProcessProxy, this is therefore unsafe. In particular, if you do a load for a given domainA in a WebContent process, then a WebContent process's registrable domain becomes domainA. If later on, you do a load for domainB in this same process, then the web process's registrable domain gets nulled out (since the WebContent process is no longer associated with a single registrable domain). There are many reasons right now causing us to load different domains in a single process, since we are sometimes unable to process swap on navigation (e.g. due to opener links).

> Source/WebKit/UIProcess/WebProcessPool.cpp:1141
> +        auto iterator = m_serviceWorkerProcesses.find(RegistrableDomainWithSessionID { process->registrableDomain(), process->websiteDataStore().sessionID() });

Using process->registrableDomain() as key is unsafe here as mentioned earlier.

> Source/WebKit/UIProcess/WebProcessPool.cpp:2538
> +void WebProcessPool::setForceSeparateServiceWorkerProcess(bool forceServiceWorkerProcess)

Missing "Separate" in the parameter name.

> Source/WebKit/UIProcess/WebProcessPool.h:530
> +    void setForceSeparateServiceWorkerProcess(bool forceServiceWorkerProcess);

Parameter name should be omitted.

> Source/WebKit/UIProcess/WebProcessPool.h:810
> +    bool m_forceServiceWorkerProcess { false };

Bad naming:
- Missing a prefix for boolean variables
- You're missing "Separate" in there

> Source/WebKit/UIProcess/WebProcessProxy.cpp:923
> +    if (isRunningServiceWorkers())

2 issues here:
- Why isn't this in canTerminateAuxiliaryProcess() with the other conditions?
- If you add a new condition for not shutting down a process, then it usually means you need to add a call to maybeShutdown() when this condition may have changed. I do not see any new calls to maybeShutdown() in this patch, which raises a red flag. You should likely be calling maybeShutdown() when a process stops running service workers (unless we are already and I merely do not see it in this patch).

> Source/WebKit/UIProcess/WebProcessProxy.cpp:1445
> +        disableServiceWorkers();

Oh, I see now that you deal with the registrable domain's getting reset here. Looks like this makes sure we remove the process from the HashMap before its registrable domain. 
Please disregard my previous comments about the registrable domain changing then. This looks fragile but acceptable.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:1548
> +    if (m_processPool)

m_processPool should never be null. Just use the processPool() getter which returns a ref.

> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:99
> +    WebProcess::singleton().disableTermination();

Please explain in the change log why this is needed. It is not obvious to me why and why it is OK, mostly because I am unfamiliar with disableTermination.

> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h:98
> +    void stop();

I am unclear what it means to "stop" a *connection*. Terminate or closing a connection makes sense, but stopping is less clear.

[youenn fablet]

> > Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:114
> > +- (void)_setForceSeparateServiceWorkerProcess:(BOOL)forceServiceWorkerProcess WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Why do we need this? Having to support both separate and merged models
> increases code complexity.

The added complexity is limited to an if check in WebProcessPool::establishWorkerContextConnectionToNetworkProcess.
We use this for instance in layout tests to check the effect of a service worker crash on a service worker client.
We also use this in API tests.

> > Source/WebKit/UIProcess/WebProcessProxy.cpp:923
> > +    if (isRunningServiceWorkers())
> 
> 2 issues here:
> - Why isn't this in canTerminateAuxiliaryProcess() with the other conditions?
> - If you add a new condition for not shutting down a process, then it
> usually means you need to add a call to maybeShutdown() when this condition
> may have changed. I do not see any new calls to maybeShutdown() in this
> patch, which raises a red flag. You should likely be calling maybeShutdown()
> when a process stops running service workers (unless we are already and I
> merely do not see it in this patch).

The principle is to use enableTermination/disableTermination which kicks in whenever network process decides that a given connection is no longer needed.
WebProcessProxy will disable service workers, which will make WebProcess close the connection to UIProcess if there is no page running in the web process.
Since we have this logic in UIProcess as well, it is indeed more consistent to also do the same in UIProcess.

> > Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:99
> > +    WebProcess::singleton().disableTermination();
> 
> Please explain in the change log why this is needed. It is not obvious to me
> why and why it is OK, mostly because I am unfamiliar with disableTermination.

disableTermination/enableTermination is used in WebProcess whenever a page is created/removed.
We reuse the same mechanism for service workers by making the service worker connection counts as a page.

[youenn fablet]

Created attachment 380637 [details]
Patch

[Chris Dumez]

If the code path is used only for testing and not a configuration we want to ship, I do not know why we are keeping it. The complexity is not just an if check, it is keeping a decent amount of extra code around and not knowing which code path is being taken.

[youenn fablet]

(In reply to Chris Dumez from comment #17)
> If the code path is used only for testing and not a configuration we want to
> ship, I do not know why we are keeping it.

This is basically a runtime flag that can be switched on and off.
This seems actually good to me to have in case we discover issues.
I guess we could try to go with WebPreferences.

> The complexity is not just an if
> check, it is keeping a decent amount of extra code around and not knowing
> which code path is being taken.

There is an if check and getters/setters.
Other than that, I am not sure to see what additional complexity this switch is adding.

In terms of complexity, I agree the approach to reuse existing processes adds some complexity. In particular, in some cases, service workers will run in the same process as a page and sometimes they will not. This is independent though of the switch to enable this approach or not.

[Chris Dumez]

Comment on attachment 380637 [details]
Patch

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

> Tools/ChangeLog:9
> +        This is useful for tests trying to crash the service worker process.

Can't those tests simply window.open() another origin to get a separate service worker process?

[youenn fablet]

> Can't those tests simply window.open() another origin to get a separate
> service worker process?

We could probably do that, until we support full process isolation, where it might break.

I honestly do not really see the added complexity of this boolean.

I am also thinking of being able to turn the feature on/off for perf comparison as part of the Debug menu.

[WebKit Commit Bot]

Comment on attachment 380637 [details]
Patch

Rejecting attachment 380637 [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', 380637, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
.
Fetching: https://bugs.webkit.org/attachment.cgi?id=380637&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=202195&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 380637 from bug 202195.
Fetching: https://bugs.webkit.org/attachment.cgi?id=380637
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Chris Dumez']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 47 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/testing/Internals.cpp
Hunk #2 succeeded at 5252 with fuzz 2 (offset 4 lines).
patching file Source/WebCore/testing/Internals.h
Hunk #1 succeeded at 892 (offset 2 lines).
patching file Source/WebCore/testing/Internals.idl
patching file Source/WebCore/testing/ServiceWorkerInternals.cpp
patching file Source/WebCore/testing/ServiceWorkerInternals.h
patching file Source/WebCore/testing/ServiceWorkerInternals.idl
patching file Source/WebCore/workers/service/ServiceWorkerProvider.cpp
patching file Source/WebCore/workers/service/context/SWContextManager.cpp
patching file Source/WebCore/workers/service/context/SWContextManager.h
patching file Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
patching file Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h
patching file Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.messages.in
patching file Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp
patching file Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp
Hunk #6 FAILED at 141.
1 out of 6 hunks FAILED -- saving rejects to file Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp.rej
patching file Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h
patching file Source/WebKit/UIProcess/API/C/WKContext.cpp
patching file Source/WebKit/UIProcess/API/C/WKContextPrivate.h
patching file Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm
Hunk #1 succeeded at 448 (offset 1 line).
Hunk #2 succeeded at 509 (offset 1 line).
patching file Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h
patching file Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp
patching file Source/WebKit/UIProcess/Network/NetworkProcessProxy.h
patching file Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in
patching file Source/WebKit/UIProcess/WebProcessPool.cpp
Hunk #1 succeeded at 671 (offset 1 line).
Hunk #2 succeeded at 702 (offset 1 line).
Hunk #3 succeeded at 1134 (offset -1 lines).
Hunk #4 succeeded at 1151 (offset -1 lines).
Hunk #5 succeeded at 1240 (offset -1 lines).
Hunk #6 succeeded at 1719 (offset -3 lines).
Hunk #7 succeeded at 2454 (offset -79 lines).
patching file Source/WebKit/UIProcess/WebProcessPool.h
Hunk #1 succeeded at 387 (offset 3 lines).
Hunk #2 succeeded at 520 (offset -8 lines).
Hunk #3 succeeded at 797 (offset -10 lines).
patching file Source/WebKit/UIProcess/WebProcessProxy.cpp
Hunk #2 succeeded at 962 with fuzz 1 (offset 3 lines).
Hunk #3 succeeded at 1469 (offset 3 lines).
Hunk #4 succeeded at 1568 (offset 5 lines).
patching file Source/WebKit/UIProcess/WebProcessProxy.h
Hunk #1 succeeded at 123 (offset -2 lines).
patching file Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp
patching file Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h
patching file Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.messages.in
patching file Source/WebKit/WebProcess/WebProcess.cpp
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm
patching file Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl
patching file Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp
patching file Tools/WebKitTestRunner/InjectedBundle/TestRunner.h
patching file Tools/WebKitTestRunner/TestController.cpp
patching file Tools/WebKitTestRunner/TestInvocation.cpp
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/http/tests/workers/service/resources/postmessage-after-sw-process-crash.js
patching file LayoutTests/http/tests/workers/service/resources/postmessage-after-terminate.js
patching file LayoutTests/http/tests/workers/service/resources/postmessage-after-terminating-hung-worker.js
patching file LayoutTests/http/wpt/service-workers/online.https.html
patching file LayoutTests/http/wpt/service-workers/service-worker-different-process.https-expected.txt
patching file LayoutTests/http/wpt/service-workers/service-worker-different-process.https.html
patching file LayoutTests/http/wpt/service-workers/service-worker-process-worker.js

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

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

[youenn fablet]

Created attachment 380857 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 380857 [details]
Patch for landing

Clearing flags on attachment: 380857

Committed r251067: <https://trac.webkit.org/changeset/251067>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

This broke some GTK bots. Any idea?
https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29/builds/26075/steps/compile-webkit/logs/stdio

[Philippe Normand]

WebKit::WebProcessProxy::createForServiceWorkers(WebKit::WebProcessPool&, WebCore::RegistrableDomain&&, WebKit::WebsiteDataStore&): error: undefined reference to 'WebKit::WebProcessProxy::enableServiceWorkers()'
collect2: error: ld returned 1 exit status

[youenn fablet]

(In reply to Philippe Normand from comment #26)
> WebKit::WebProcessProxy::createForServiceWorkers(WebKit::WebProcessPool&,
> WebCore::RegistrableDomain&&, WebKit::WebsiteDataStore&): error: undefined
> reference to 'WebKit::WebProcessProxy::enableServiceWorkers()'
> collect2: error: ld returned 1 exit status

Oh probably ENABLE(SERVICE_WORKER) issue, I'll fix it.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 380875 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/56245134>

[Radar WebKit Bug Importer]

<rdar://problem/56245136>

[Philippe Normand]

Comment on attachment 380875 [details]
Patch

rs=me :)

[WebKit Commit Bot]

Comment on attachment 380875 [details]
Patch

Clearing flags on attachment: 380875

Committed r251073: <https://trac.webkit.org/changeset/251073>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.