Bug 151580 – Make usesNetworkProcess always true

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Make usesNetworkProcess always true

[Alex Christensen]

Created attachment 266114 [details]
Patch

[Alex Christensen]

Created attachment 266116 [details]
Patch

[Alex Christensen]

Created attachment 266117 [details]
Patch

[Alex Christensen]

Created attachment 266119 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 266119 [details]
Patch

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

> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:-100
>      encoder << cookiePersistentStoragePath;
>      encoder << cookiePersistentStorageType;
>      encoder.encodeEnum(cookieAcceptPolicy);
> -    encoder << ignoreTLSErrors;

I think we can remove all other network parameters as well. At least diskCacheDirectory, cookiePersistentStoragePath, cookiePersistentStorageType, cookieAcceptPolicy and urlSchemesRegisteredForCustomProtocols

> Source/WebKit2/Shared/Network/CustomProtocols/Cocoa/CustomProtocolManagerCocoa.mm:140
>  void CustomProtocolManager::initialize(const WebProcessCreationParameters& parameters)
>  {
> -    ASSERT(parameters.urlSchemesRegisteredForCustomProtocols.isEmpty() || !parameters.usesNetworkProcess);
> -    if (parameters.usesNetworkProcess) {
> -        m_childProcess->parentProcessConnection()->removeWorkQueueMessageReceiver(Messages::CustomProtocolManager::messageReceiverName());
> -        m_messageQueue = nullptr;
> -        return;
> -    }
> -
> -    [NSURLProtocol registerClass:[WKCustomProtocol class]];
> -
> -    for (size_t i = 0; i < parameters.urlSchemesRegisteredForCustomProtocols.size(); ++i)
> -        registerScheme(parameters.urlSchemesRegisteredForCustomProtocols[i]);
> +    ASSERT(parameters.urlSchemesRegisteredForCustomProtocols.isEmpty());
> +    m_childProcess->parentProcessConnection()->removeWorkQueueMessageReceiver(Messages::CustomProtocolManager::messageReceiverName());
> +    m_messageQueue = nullptr;
>  }

I think we should remove this entirely and make CustomProtocolManager a NetworkProcessSupplement only.

[Darin Adler]

Comment on attachment 266119 [details]
Patch

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

>> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:-100
>> -    encoder << ignoreTLSErrors;
> 
> I think we can remove all other network parameters as well. At least diskCacheDirectory, cookiePersistentStoragePath, cookiePersistentStorageType, cookieAcceptPolicy and urlSchemesRegisteredForCustomProtocols

Should return and delete even more of this unused stuff.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:484
> +    if (networkProcess()) {

I don't understand why this file mixes networkProcess() and m_networkProcess.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:1011
> +    ASSERT(m_networkProcess);

Assertion seems like overkill with nearly no value.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:1012
> +    return m_networkProcess->createDownloadProxy(request);

Should just use the return value of ensureNetworkProcess.

[Alex Christensen]

Created attachment 266141 [details]
Patch

[Alex Christensen]

(In reply to comment #7)
> Created attachment 266141 [details]
> Patch

I addressed comments.  I plan to land this after Thanksgiving.  I'll be removing more code after that, too.

[Carlos Garcia Campos]

Comment on attachment 266141 [details]
Patch

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

> Source/WebKit2/Shared/Network/CustomProtocols/soup/CustomProtocolManagerSoup.cpp:54
>  void CustomProtocolManager::initialize(const WebProcessCreationParameters& parameters)
>  {
> -    ASSERT(parameters.urlSchemesRegisteredForCustomProtocols.isEmpty() || !parameters.usesNetworkProcess);
> -    if (parameters.usesNetworkProcess) {
> -        m_childProcess->parentProcessConnection()->removeWorkQueueMessageReceiver(Messages::CustomProtocolManager::messageReceiverName());
> -        m_messageQueue = nullptr;
> -        return;
> -    }
> -    for (size_t i = 0; i < parameters.urlSchemesRegisteredForCustomProtocols.size(); ++i)
> -        registerScheme(parameters.urlSchemesRegisteredForCustomProtocols[i]);
> +    ASSERT(parameters.urlSchemesRegisteredForCustomProtocols.isEmpty());
> +    m_childProcess->parentProcessConnection()->removeWorkQueueMessageReceiver(Messages::CustomProtocolManager::messageReceiverName());
> +    m_messageQueue = nullptr;
>  }

This should be removed entirely as well, please do it before landing.

[Csaba Osztrogonác]

Comment on attachment 266141 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/WebProcessPoolEfl.cpp:97
>      parameters.urlSchemesRegisteredForCustomProtocols = supplement<WebSoupCustomProtocolRequestManager>()->registeredSchemesForCustomProtocols();
>      supplement<WebCookieManagerProxy>()->getCookiePersistentStorage(parameters.cookiePersistentStoragePath, parameters.cookiePersistentStorageType);
>      parameters.cookieAcceptPolicy = m_initialHTTPCookieAcceptPolicy;
> -    parameters.ignoreTLSErrors = m_ignoreTLSErrors;
>      parameters.diskCacheDirectory = m_configuration->diskCacheDirectory();
>  }

I think these lines should be removed too to fix the EFL build.

../../Source/WebKit2/UIProcess/efl/WebProcessPoolEfl.cpp:93:16: error: 'struct WebKit::WebProcessCreationParameters' has no member named 'urlSchemesRegisteredForCustomProtocols'
../../Source/WebKit2/UIProcess/efl/WebProcessPoolEfl.cpp:94:80: error: 'struct WebKit::WebProcessCreationParameters' has no member named 'cookiePersistentStoragePath'
../../Source/WebKit2/UIProcess/efl/WebProcessPoolEfl.cpp:94:120: error: 'struct WebKit::WebProcessCreationParameters' has no member named 'cookiePersistentStorageType'
../../Source/WebKit2/UIProcess/efl/WebProcessPoolEfl.cpp:95:16: error: 'struct WebKit::WebProcessCreationParameters' has no member named 'cookieAcceptPolicy'
../../Source/WebKit2/UIProcess/efl/WebProcessPoolEfl.cpp:96:16: error: 'struct WebKit::WebProcessCreationParameters' has no member named 'diskCacheDirectory'

[Csaba Osztrogonác]

Created attachment 266244 [details]
EFL buildfix

This small patch fixed the EFL build for me, feel free to pick it up.

[Csaba Osztrogonác]

(In reply to comment #11)
> Created attachment 266244 [details]
> EFL buildfix
> 
> This small patch fixed the EFL build for me, feel free to pick it up.

But unfortunately tests are still failing with timeout as I mentioned 
it previously: https://bugs.webkit.org/show_bug.cgi?id=151418#c18

[Csaba Osztrogonác]

(In reply to comment #12)
> (In reply to comment #11)
> > Created attachment 266244 [details]
> > EFL buildfix
> > 
> > This small patch fixed the EFL build for me, feel free to pick it up.
> 
> But unfortunately tests are still failing with timeout as I mentioned 
> it previously: https://bugs.webkit.org/show_bug.cgi?id=151418#c18

It is a 15 months old regression, see bug137692 for details. It means that EFL
port didn't test enabled NetworkProcess at all nowadays. It's not so good. :-/

[Carlos Garcia Campos]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Created attachment 266244 [details]
> > > EFL buildfix
> > > 
> > > This small patch fixed the EFL build for me, feel free to pick it up.
> > 
> > But unfortunately tests are still failing with timeout as I mentioned 
> > it previously: https://bugs.webkit.org/show_bug.cgi?id=151418#c18
> 
> It is a 15 months old regression, see bug137692 for details. It means that
> EFL
> port didn't test enabled NetworkProcess at all nowadays. It's not so good.
> :-/

Looking at the patch proposed, it seems to me that it could be an integer overflow in a timer delay due to the std::chrono::milliseconds::max(). We had similar issues with std::chrono::microseconds::max(). The proposed patch seems to be more a workaround than a real fix.

[Alex Christensen]

Carlos and Ossy, thanks for looking into the build failures.
http://trac.webkit.org/changeset/192796

[Csaba Osztrogonác]

(In reply to comment #15)
> Carlos and Ossy, thanks for looking into the build failures.
> http://trac.webkit.org/changeset/192796

It made Networking completely broken on EFL - bug137692 .
And it made many http tests timeout on GTK too:
- before: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/12445
- after: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/12446

[Michael Catanzaro]

We never tested the network process until now, so the GTK tests are probably only now catching bugs that have always existed. Let's not block on those.

[Carlos Garcia Campos]

(In reply to comment #17)
> We never tested the network process until now, so the GTK tests are probably
> only now catching bugs that have always existed. Let's not block on those.

This is not true, we have run the layout tests with the network process enabled for long time, since r175776, see bug #138428

[Carlos Garcia Campos]

(In reply to comment #16)
> (In reply to comment #15)
> > Carlos and Ossy, thanks for looking into the build failures.
> > http://trac.webkit.org/changeset/192796
> 
> It made Networking completely broken on EFL - bug137692 .
> And it made many http tests timeout on GTK too:
> - before:
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20%28Tests%29/builds/12445
> - after:
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20%28Tests%29/builds/12446

GTK+ failures were because r192796 removed the m_ignoreTLSErrors initialization from WebProcessPool by mistake. Those tests use HTTPS and loads failed due to invalid certificate issues. It should be fixed now in r192885

[Radar WebKit Bug Importer]

<rdar://problem/24558287>