Bug 146102 – Do not store configuration parameters twice in WebProcessPool

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

Since we are copying the given WebProcessPoolConfiguration and keeping it as a member, we don't need to keep an additional copy of its contents as extra members.

[Carlos Garcia Campos]

Created attachment 255100 [details]
Patch

[Sam Weinig]

Comment on attachment 255100 [details]
Patch

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

> Source/WebKit2/UIProcess/WebProcessPool.cpp:357
>  void WebProcessPool::setUsesNetworkProcess(bool usesNetworkProcess)
>  {
>  #if ENABLE(NETWORK_PROCESS)
> -    m_usesNetworkProcess = usesNetworkProcess;
> +    m_configuration->setUseNetworkProcess(usesNetworkProcess);

This is a subtle change in behavior.  Before, the configuration was unchanged if someone called WebProcessPool::setUsesNetworkProcess() (via WKContextSetUsesNetworkProcess(...)), but now it will be mutated.  I'm not sure if this is a big deal in practice.  The same is true with WebProcessPool::setMaximumNumberOfProcesses() and WebProcessPool::setProcessModel().

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 255100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255100&action=review

Thanks for the review.

> > Source/WebKit2/UIProcess/WebProcessPool.cpp:357
> >  void WebProcessPool::setUsesNetworkProcess(bool usesNetworkProcess)
> >  {
> >  #if ENABLE(NETWORK_PROCESS)
> > -    m_usesNetworkProcess = usesNetworkProcess;
> > +    m_configuration->setUseNetworkProcess(usesNetworkProcess);
> 
> This is a subtle change in behavior.  Before, the configuration was
> unchanged if someone called WebProcessPool::setUsesNetworkProcess() (via
> WKContextSetUsesNetworkProcess(...)), but now it will be mutated.  I'm not
> sure if this is a big deal in practice.  The same is true with
> WebProcessPool::setMaximumNumberOfProcesses() and
> WebProcessPool::setProcessModel().

That's a good point, but there's no change in behaviour actually, because we were not using the copied configuration for those, but the members. So, you could create a WebProcessPool with usesNetworkProcess = true, a network process is ensured later, and then call WKContextSetUsesNetworkProcess(false) which set the m_usesNetworkProcess to false. WebProcessPool checked the value of m_usesNetworkProcess, so something is going to fail/crash at some point. Same would happen now with the configuration member only. We could try to prevent that in both cases, by ignoring any change in configuration after the first web process and the network process is launched.

[Carlos Garcia Campos]

Committed r185736: <http://trac.webkit.org/changeset/185736>