Bug 136478 - Initialize m_usesNetworkProcess earlier in WebProcess::initializeWebProcess()
Summary: Initialize m_usesNetworkProcess earlier in WebProcess::initializeWebProcess()
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: Gtk
Depends on:
Blocks:
 
Reported: 2014-09-03 03:32 PDT by Carlos Garcia Campos
Modified: 2014-09-04 00:41 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.84 KB, patch)
2014-09-03 03:35 PDT, Carlos Garcia Campos
ap: review-
Details | Formatted Diff | Diff
Updated patch (3.60 KB, patch)
2014-09-03 23:18 PDT, Carlos Garcia Campos
ap: 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 2014-09-03 03:32:05 PDT
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.
Comment 1 Carlos Garcia Campos 2014-09-03 03:35:28 PDT
Created attachment 237552 [details]
Patch
Comment 2 Sergio Villar Senin 2014-09-03 03:38:16 PDT
Comment on attachment 237552 [details]
Patch

Good catch!
Comment 3 Carlos Garcia Campos 2014-09-03 05:00:22 PDT
This needs to be approved by a WebKit2 owner. Anders, Brady? any objection?
Comment 4 Alexey Proskuryakov 2014-09-03 08:48:54 PDT
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.
Comment 5 Carlos Garcia Campos 2014-09-03 09:08:56 PDT
(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?
Comment 6 Alexey Proskuryakov 2014-09-03 10:09:50 PDT
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).
Comment 7 Carlos Garcia Campos 2014-09-03 23:02:23 PDT
(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.
Comment 8 Carlos Garcia Campos 2014-09-03 23:18:25 PDT
Created attachment 237611 [details]
Updated patch
Comment 9 Alexey Proskuryakov 2014-09-03 23:52:32 PDT
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).
Comment 10 Carlos Garcia Campos 2014-09-04 00:36:00 PDT
(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!
Comment 11 Carlos Garcia Campos 2014-09-04 00:41:46 PDT
Committed r173255: <http://trac.webkit.org/changeset/173255>