Bug 136478 – Initialize m_usesNetworkProcess earlier in WebProcess::initializeWebProcess()

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

[Carlos Garcia Campos]

I realized that we were initializing some network features like the disk cache in all the web processes even when using the network process. The reason is because we rely on WebProcess::usesNetworkProcess() method in WebProcess::platformSetCacheModel() and WebProcess::platformInitializeWebProcess() and both are called before m_usesNetworkProcess is initialized. So, we are initializing everything, but not freeing it, because after the initializeWebProcess(), usesNetworkProcess() always returns true.

[Carlos Garcia Campos]

Created attachment 237552 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 237552 [details]
Patch

Good catch!

[Carlos Garcia Campos]

This needs to be approved by a WebKit2 owner. Anders, Brady? any objection?

[Alexey Proskuryakov]

There is still some networking in WebProcess even when using NetworkProcess. Are you sure that you don't need the initialization? When we recently lost it on Mac accidentally, that caused trouble as we were trying to access the wrong cache directory. 

It is of course wrong to achieve that as a subtle side effect of initialization order.

[Carlos Garcia Campos]

(In reply to comment #4)
> There is still some networking in WebProcess even when using NetworkProcess. Are you sure that you don't need the initialization? When we recently lost it on Mac accidentally, that caused trouble as we were trying to access the wrong cache directory. 

Yes, the effect for us is that we don't use any disk cache for the loads made in the web process, that's not a problem.
 
> It is of course wrong to achieve that as a subtle side effect of initialization order.

Can I land the patch then?

[Alexey Proskuryakov]

Comment on attachment 237552 [details]
Patch

I needed to check Mac code, and now that I've done that, it looks like this patch breaks Mac.

What happens is that in platformInitializeWebProcess(), we make WebProcess use the same on-disk cache that UI process and NetworkProcess use. There is some sad history of mistakes there - we had code that seemed like it made us use a different code path, but it didn't because m_usesNetworkProcess was not initialized yet. The code was mistakenly changed to actually respect the network process setting in <http://trac.webkit.org/changeset/170155>, and then we started to hit sandbox violations accessing the wrong cache. Then in <http://trac.webkit.org/r171156> I reverted the behavior, removing the broken usesNetworkProcess() check. I forgot to remove a comment saying that we wanted a standalone cache.

After platformInitializeWebProcess(), initializeWebProcess() calls setCacheModel(), which calls platformSetCacheModel(), which once again checks usesNetworkProcess() and disables the cache. This is also currently dead code, because m_usesNetworkProcess is still not initialized. But after this proposed patch, m_usesNetworkProcess will be true, so we will disable the shared disk cache in all processes!

Here is what I think needs to be done to fix the patch:

1. Remove this dead code from WebProcessCocoa.mm:

    // FIXME: Once there is no loading being done in the WebProcess, we should remove this,
    // as calling [NSURLCache sharedURLCache] initializes the cache, which we would rather not do.
    if (usesNetworkProcess()) {
        [nsurlCache setMemoryCapacity:0];
        [nsurlCache setDiskCapacity:0];
        return;
    }

2. Remove this comment from the same file:

    // When the network process is enabled, each web process wants a stand-alone
    // NSURLCache, which it can disable to save memory.

3. Notify Apple folks working on WebKit performance that the optimization to disable caching in WebProcess has been broken for a long time, and that we are removing what remains of that code (I will do it).

[Carlos Garcia Campos]

(In reply to comment #6)
> (From update of attachment 237552 [details])
> I needed to check Mac code, and now that I've done that, it looks like this patch breaks Mac.

Thanks!

> What happens is that in platformInitializeWebProcess(), we make WebProcess use the same on-disk cache that UI process and NetworkProcess use. There is some sad history of mistakes there - we had code that seemed like it made us use a different code path, but it didn't because m_usesNetworkProcess was not initialized yet. The code was mistakenly changed to actually respect the network process setting in <http://trac.webkit.org/changeset/170155>,

That was also my first approach until I realized I also had the check in platformSetCacheModel().

> and then we started to hit sandbox violations accessing the wrong cache. Then in <http://trac.webkit.org/r171156> I reverted the behavior, removing the broken usesNetworkProcess() check. I forgot to remove a comment saying that we wanted a standalone cache.
> 
> After platformInitializeWebProcess(), initializeWebProcess() calls setCacheModel(), which calls platformSetCacheModel(), which once again checks usesNetworkProcess() and disables the cache. This is also currently dead code, because m_usesNetworkProcess is still not initialized. But after this proposed patch, m_usesNetworkProcess will be true, so we will disable the shared disk cache in all processes!

I see.

> Here is what I think needs to be done to fix the patch:
> 
> 1. Remove this dead code from WebProcessCocoa.mm:
> 
>     // FIXME: Once there is no loading being done in the WebProcess, we should remove this,
>     // as calling [NSURLCache sharedURLCache] initializes the cache, which we would rather not do.
>     if (usesNetworkProcess()) {
>         [nsurlCache setMemoryCapacity:0];
>         [nsurlCache setDiskCapacity:0];
>         return;
>     }
> 
> 2. Remove this comment from the same file:
> 
>     // When the network process is enabled, each web process wants a stand-alone
>     // NSURLCache, which it can disable to save memory.
> 
> 3. Notify Apple folks working on WebKit performance that the optimization to disable caching in WebProcess has been broken for a long time, and that we are removing what remains of that code (I will do it).

Ok.

[Carlos Garcia Campos]

Created attachment 237611 [details]
Updated patch

[Alexey Proskuryakov]

Comment on attachment 237611 [details]
Updated patch

r=me. Please wait for EWS results (Mac WK2 EWS is somewhat broken again, but it should provide the results eventually).

[Carlos Garcia Campos]

(In reply to comment #9)
> (From update of attachment 237611 [details])
> r=me. Please wait for EWS results (Mac WK2 EWS is somewhat broken again, but it should provide the results eventually).

It's green now :-) Thanks!

[Carlos Garcia Campos]

Committed r173255: <http://trac.webkit.org/changeset/173255>