[webkit-reviews] review denied: [Bug 202195] Reuse existing web processes for running service workers : [Attachment 380221] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied youenn fablet <youennf at gmail.com>'s
request for review:
Bug 202195: Reuse existing web processes for running service workers
https://bugs.webkit.org/show_bug.cgi?id=202195

Attachment 380221: Patch

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




--- Comment #14 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 380221
  --> https://bugs.webkit.org/attachment.cgi?id=380221
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.