Summary: | Initialize m_usesNetworkProcess earlier in WebProcess::initializeWebProcess() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | andersca, ap, beidson, gustavo, svillar | ||||||
Priority: | P2 | Keywords: | Gtk | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2014-09-03 03:32:05 PDT
Created attachment 237552 [details]
Patch
Comment on attachment 237552 [details]
Patch
Good catch!
This needs to be approved by a WebKit2 owner. Anders, Brady? any objection? 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. (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 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). (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. Created attachment 237611 [details]
Updated patch
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).
(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! Committed r173255: <http://trac.webkit.org/changeset/173255> |