Bug 146102 - Do not store configuration parameters twice in WebProcessPool
Summary: Do not store configuration parameters twice in WebProcessPool
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-18 01:25 PDT by Carlos Garcia Campos
Modified: 2015-06-18 23:00 PDT (History)
3 users (show)

See Also:


Attachments
Patch (27.60 KB, patch)
2015-06-18 01:29 PDT, Carlos Garcia Campos
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2015-06-18 01:25:02 PDT
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.
Comment 1 Carlos Garcia Campos 2015-06-18 01:29:07 PDT
Created attachment 255100 [details]
Patch
Comment 2 Sam Weinig 2015-06-18 19:48:37 PDT
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().
Comment 3 Carlos Garcia Campos 2015-06-18 22:57:37 PDT
(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.
Comment 4 Carlos Garcia Campos 2015-06-18 23:00:32 PDT
Committed r185736: <http://trac.webkit.org/changeset/185736>