[webkit-reviews] review denied: [Bug 136478] Initialize m_usesNetworkProcess earlier in WebProcess::initializeWebProcess() : [Attachment 237552] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied	review:
Bug 136478: Initialize m_usesNetworkProcess earlier in
WebProcess::initializeWebProcess()
https://bugs.webkit.org/show_bug.cgi?id=136478

Attachment 237552: Patch
https://bugs.webkit.org/attachment.cgi?id=237552&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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).