136478 2014-09-03 03:32:05 -0700 Initialize m_usesNetworkProcess earlier in WebProcess::initializeWebProcess() 2014-09-04 00:41:46 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED Gtk P2 Normal --- 1 cgarcia webkit-unassigned andersca ap beidson gustavo svillar oldest_to_newest 1032873 0 cgarcia 2014-09-03 03:32:05 -0700 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. 1032874 1 237552 cgarcia 2014-09-03 03:35:28 -0700 Created attachment 237552 Patch 1032875 2 237552 svillar 2014-09-03 03:38:16 -0700 Comment on attachment 237552 Patch Good catch! 1032885 3 cgarcia 2014-09-03 05:00:22 -0700 This needs to be approved by a WebKit2 owner. Anders, Brady? any objection? 1032905 4 ap 2014-09-03 08:48:54 -0700 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. 1032908 5 cgarcia 2014-09-03 09:08:56 -0700 (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? 1032915 6 237552 ap 2014-09-03 10:09:50 -0700 Comment on attachment 237552 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). 1033138 7 cgarcia 2014-09-03 23:02:23 -0700 (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. 1033140 8 237611 cgarcia 2014-09-03 23:18:25 -0700 Created attachment 237611 Updated patch 1033155 9 237611 ap 2014-09-03 23:52:32 -0700 Comment on attachment 237611 Updated patch r=me. Please wait for EWS results (Mac WK2 EWS is somewhat broken again, but it should provide the results eventually). 1033160 10 cgarcia 2014-09-04 00:36:00 -0700 (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! 1033161 11 cgarcia 2014-09-04 00:41:46 -0700 Committed r173255: <http://trac.webkit.org/changeset/173255> 237552 2014-09-03 03:35:28 -0700 2014-09-03 23:18:25 -0700 Patch wk2-web-process-network.diff text/plain 1887 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCAwNTMyNTZmLi5kOTkzOWNkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjAg QEAKKzIwMTQtMDktMDMgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgorCisgICAgICAgIEluaXRpYWxpemUgbV91c2VzTmV0d29ya1Byb2Nlc3MgZWFybGllciBpbiBX ZWJQcm9jZXNzOjppbml0aWFsaXplV2ViUHJvY2VzcygpCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzY0NzgKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGUgc291cCBuZXR3b3JrIGJhY2tlbmQgdXNlcyBX ZWJQcm9jZXNzOjp1c2VzTmV0d29ya1Byb2Nlc3MoKQorICAgICAgICBmcm9tIFdlYlByb2Nlc3M6 OnBsYXRmb3JtU2V0Q2FjaGVNb2RlbCgpIGFuZAorICAgICAgICBXZWJQcm9jZXNzOjpwbGF0Zm9y bUluaXRpYWxpemVXZWJQcm9jZXNzKCksIGJvdGggY2FsbGVkIGJlZm9yZSB0aGUKKyAgICAgICAg bV91c2VzTmV0d29ya1Byb2Nlc3MgbWVtYmVyIGhhcyBiZWVuIGluaXRpYWxpemVkLgorCisgICAg ICAgICogV2ViUHJvY2Vzcy9XZWJQcm9jZXNzLmNwcDoKKyAgICAgICAgKFdlYktpdDo6V2ViUHJv Y2Vzczo6aW5pdGlhbGl6ZVdlYlByb2Nlc3MpOiBNb3ZlIHRoZQorICAgICAgICBtX3VzZXNOZXR3 b3JrUHJvY2VzcyBpbml0aWFsaXphdGlvbiBhdCB0aGUgYmVnaW5uaW5nIHJpZ2h0IGJlZm9yZQor ICAgICAgICBjYWxsaW5nIHBsYXRmb3JtSW5pdGlhbGl6ZVdlYlByb2Nlc3MoKS4KKwogMjAxNC0w OS0wMiAgUGhpbGlwcGUgTm9ybWFuZCAgPHBub3JtYW5kQGlnYWxpYS5jb20+CiAKICAgICAgICAg VW5yZXZpZXdlZCwgR1RLIGJ1aWxkIGZpeCBhZnRlciByMTczMTYzLgpkaWZmIC0tZ2l0IGEvU291 cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQcm9jZXNzLmNwcCBiL1NvdXJjZS9XZWJLaXQyL1dl YlByb2Nlc3MvV2ViUHJvY2Vzcy5jcHAKaW5kZXggZTJkNzRlMi4uMjBhMzBhZSAxMDA2NDQKLS0t IGEvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQcm9jZXNzLmNwcAorKysgYi9Tb3VyY2Uv V2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlByb2Nlc3MuY3BwCkBAIC0yNzEsNiArMjcxLDEwIEBAIHZv aWQgV2ViUHJvY2Vzczo6aW5pdGlhbGl6ZVdlYlByb2Nlc3MoY29uc3QgV2ViUHJvY2Vzc0NyZWF0 aW9uUGFyYW1ldGVycyYgcGFyYW1lCiB7CiAgICAgQVNTRVJUKG1fcGFnZU1hcC5pc0VtcHR5KCkp OwogCisjaWYgRU5BQkxFKE5FVFdPUktfUFJPQ0VTUykKKyAgICBtX3VzZXNOZXR3b3JrUHJvY2Vz cyA9IHBhcmFtZXRlcnMudXNlc05ldHdvcmtQcm9jZXNzOworI2VuZGlmCisKICAgICBwbGF0Zm9y bUluaXRpYWxpemVXZWJQcm9jZXNzKHBhcmFtZXRlcnMsIGRlY29kZXIpOwogCiAgICAgV1RGOjpz ZXRDdXJyZW50VGhyZWFkSXNVc2VySW5pdGlhdGVkKCk7CkBAIC0zNDgsNyArMzUyLDYgQEAgdm9p ZCBXZWJQcm9jZXNzOjppbml0aWFsaXplV2ViUHJvY2Vzcyhjb25zdCBXZWJQcm9jZXNzQ3JlYXRp b25QYXJhbWV0ZXJzJiBwYXJhbWUKICAgICAgICAgTmV0d29ya1N0b3JhZ2VTZXNzaW9uOjpzd2l0 Y2hUb05ld1Rlc3RpbmdTZXNzaW9uKCk7CiAKICNpZiBFTkFCTEUoTkVUV09SS19QUk9DRVNTKQot ICAgIG1fdXNlc05ldHdvcmtQcm9jZXNzID0gcGFyYW1ldGVycy51c2VzTmV0d29ya1Byb2Nlc3M7 CiAgICAgZW5zdXJlTmV0d29ya1Byb2Nlc3NDb25uZWN0aW9uKCk7CiAKICNpZiBQTEFURk9STShD T0NPQSkK 237611 2014-09-03 23:18:25 -0700 2014-09-03 23:52:31 -0700 Updated patch wk2-uses-network-process.diff text/plain 3689 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCAwNTMyNTZmLi42ZTVjZGUyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjcg QEAKKzIwMTQtMDktMDMgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgorCisgICAgICAgIEluaXRpYWxpemUgbV91c2VzTmV0d29ya1Byb2Nlc3MgZWFybGllciBpbiBX ZWJQcm9jZXNzOjppbml0aWFsaXplV2ViUHJvY2VzcygpCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzY0NzgKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGUgc291cCBuZXR3b3JrIGJhY2tlbmQgdXNlcyBX ZWJQcm9jZXNzOjp1c2VzTmV0d29ya1Byb2Nlc3MoKQorICAgICAgICBmcm9tIFdlYlByb2Nlc3M6 OnBsYXRmb3JtU2V0Q2FjaGVNb2RlbCgpIGFuZAorICAgICAgICBXZWJQcm9jZXNzOjpwbGF0Zm9y bUluaXRpYWxpemVXZWJQcm9jZXNzKCksIGJvdGggY2FsbGVkIGJlZm9yZSB0aGUKKyAgICAgICAg bV91c2VzTmV0d29ya1Byb2Nlc3MgbWVtYmVyIGhhcyBiZWVuIGluaXRpYWxpemVkLgorCisgICAg ICAgICogV2ViUHJvY2Vzcy9XZWJQcm9jZXNzLmNwcDoKKyAgICAgICAgKFdlYktpdDo6V2ViUHJv Y2Vzczo6aW5pdGlhbGl6ZVdlYlByb2Nlc3MpOiBNb3ZlIHRoZQorICAgICAgICBtX3VzZXNOZXR3 b3JrUHJvY2VzcyBpbml0aWFsaXphdGlvbiBhdCB0aGUgYmVnaW5uaW5nIHJpZ2h0IGJlZm9yZQor ICAgICAgICBjYWxsaW5nIHBsYXRmb3JtSW5pdGlhbGl6ZVdlYlByb2Nlc3MoKS4KKyAgICAgICAg KiBXZWJQcm9jZXNzL2NvY29hL1dlYlByb2Nlc3NDb2NvYS5tbToKKyAgICAgICAgKFdlYktpdDo6 V2ViUHJvY2Vzczo6cGxhdGZvcm1TZXRDYWNoZU1vZGVsKTogUmVtb3ZlIHRoZSBlYXJseQorICAg ICAgICByZXR1cm4gd2hlbiB1c2luZyB0aGUgbmV0d29yayBwcm9jZXNzLCBzaW5jZSB0aGlzIHdh cyBkZWFkIGNvZGUKKyAgICAgICAgYmVmb3JlIHRoaXMgY2hhbmdlLCBhbmQgbm90IHJlYWxseSBu ZWVkZWQgbm93IHRoYXQKKyAgICAgICAgdXNlc05ldHdvcmtQcm9jZXNzKCkgd2lsbCByZXR1cm4g dHJ1ZS4KKyAgICAgICAgKFdlYktpdDo6V2ViUHJvY2Vzczo6cGxhdGZvcm1Jbml0aWFsaXplV2Vi UHJvY2Vzcyk6IFJlbW92ZSBjb21tZW50CisgICAgICAgIHRoYXQgaXMgbm90IGFjY3VyYXRlIHNp bmNlIHIxNzExNTYuCisKIDIwMTQtMDktMDIgIFBoaWxpcHBlIE5vcm1hbmQgIDxwbm9ybWFuZEBp Z2FsaWEuY29tPgogCiAgICAgICAgIFVucmV2aWV3ZWQsIEdUSyBidWlsZCBmaXggYWZ0ZXIgcjE3 MzE2My4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUHJvY2Vzcy5j cHAgYi9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlByb2Nlc3MuY3BwCmluZGV4IGUyZDc0 ZTIuLjIwYTMwYWUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUHJv Y2Vzcy5jcHAKKysrIGIvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQcm9jZXNzLmNwcApA QCAtMjcxLDYgKzI3MSwxMCBAQCB2b2lkIFdlYlByb2Nlc3M6OmluaXRpYWxpemVXZWJQcm9jZXNz KGNvbnN0IFdlYlByb2Nlc3NDcmVhdGlvblBhcmFtZXRlcnMmIHBhcmFtZQogewogICAgIEFTU0VS VChtX3BhZ2VNYXAuaXNFbXB0eSgpKTsKIAorI2lmIEVOQUJMRShORVRXT1JLX1BST0NFU1MpCisg ICAgbV91c2VzTmV0d29ya1Byb2Nlc3MgPSBwYXJhbWV0ZXJzLnVzZXNOZXR3b3JrUHJvY2VzczsK KyNlbmRpZgorCiAgICAgcGxhdGZvcm1Jbml0aWFsaXplV2ViUHJvY2VzcyhwYXJhbWV0ZXJzLCBk ZWNvZGVyKTsKIAogICAgIFdURjo6c2V0Q3VycmVudFRocmVhZElzVXNlckluaXRpYXRlZCgpOwpA QCAtMzQ4LDcgKzM1Miw2IEBAIHZvaWQgV2ViUHJvY2Vzczo6aW5pdGlhbGl6ZVdlYlByb2Nlc3Mo Y29uc3QgV2ViUHJvY2Vzc0NyZWF0aW9uUGFyYW1ldGVycyYgcGFyYW1lCiAgICAgICAgIE5ldHdv cmtTdG9yYWdlU2Vzc2lvbjo6c3dpdGNoVG9OZXdUZXN0aW5nU2Vzc2lvbigpOwogCiAjaWYgRU5B QkxFKE5FVFdPUktfUFJPQ0VTUykKLSAgICBtX3VzZXNOZXR3b3JrUHJvY2VzcyA9IHBhcmFtZXRl cnMudXNlc05ldHdvcmtQcm9jZXNzOwogICAgIGVuc3VyZU5ldHdvcmtQcm9jZXNzQ29ubmVjdGlv bigpOwogCiAjaWYgUExBVEZPUk0oQ09DT0EpCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9X ZWJQcm9jZXNzL2NvY29hL1dlYlByb2Nlc3NDb2NvYS5tbSBiL1NvdXJjZS9XZWJLaXQyL1dlYlBy b2Nlc3MvY29jb2EvV2ViUHJvY2Vzc0NvY29hLm1tCmluZGV4IDEzYWNiMTkuLjJjYmZlYWIgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvY29jb2EvV2ViUHJvY2Vzc0NvY29h Lm1tCisrKyBiL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvY29jb2EvV2ViUHJvY2Vzc0NvY29h Lm1tCkBAIC0xMjAsMTQgKzEyMCw2IEBAIHZvaWQgV2ViUHJvY2Vzczo6cGxhdGZvcm1TZXRDYWNo ZU1vZGVsKENhY2hlTW9kZWwgY2FjaGVNb2RlbCkKIAogICAgIE5TVVJMQ2FjaGUgKm5zdXJsQ2Fj aGUgPSBbTlNVUkxDYWNoZSBzaGFyZWRVUkxDYWNoZV07CiAKLSAgICAvLyBGSVhNRTogT25jZSB0 aGVyZSBpcyBubyBsb2FkaW5nIGJlaW5nIGRvbmUgaW4gdGhlIFdlYlByb2Nlc3MsIHdlIHNob3Vs ZCByZW1vdmUgdGhpcywKLSAgICAvLyBhcyBjYWxsaW5nIFtOU1VSTENhY2hlIHNoYXJlZFVSTENh Y2hlXSBpbml0aWFsaXplcyB0aGUgY2FjaGUsIHdoaWNoIHdlIHdvdWxkIHJhdGhlciBub3QgZG8u Ci0gICAgaWYgKHVzZXNOZXR3b3JrUHJvY2VzcygpKSB7Ci0gICAgICAgIFtuc3VybENhY2hlIHNl dE1lbW9yeUNhcGFjaXR5OjBdOwotICAgICAgICBbbnN1cmxDYWNoZSBzZXREaXNrQ2FwYWNpdHk6 MF07Ci0gICAgICAgIHJldHVybjsKLSAgICB9Ci0KICAgICBbbnN1cmxDYWNoZSBzZXRNZW1vcnlD YXBhY2l0eTp1cmxDYWNoZU1lbW9yeUNhcGFjaXR5XTsKICAgICBbbnN1cmxDYWNoZSBzZXREaXNr Q2FwYWNpdHk6c3RkOjptYXg8dW5zaWduZWQgbG9uZz4odXJsQ2FjaGVEaXNrQ2FwYWNpdHksIFtu c3VybENhY2hlIGRpc2tDYXBhY2l0eV0pXTsgLy8gRG9uJ3Qgc2hyaW5rIGEgYmlnIGRpc2sgY2Fj aGUsIHNpbmNlIHRoYXQgd291bGQgY2F1c2UgY2h1cm4uCiB9CkBAIC0xNzUsOCArMTY3LDYgQEAg dm9pZCBXZWJQcm9jZXNzOjpwbGF0Zm9ybUluaXRpYWxpemVXZWJQcm9jZXNzKGNvbnN0IFdlYlBy b2Nlc3NDcmVhdGlvblBhcmFtZXRlcnMKICNlbmRpZgogI2VuZGlmCiAKLSAgICAvLyBXaGVuIHRo ZSBuZXR3b3JrIHByb2Nlc3MgaXMgZW5hYmxlZCwgZWFjaCB3ZWIgcHJvY2VzcyB3YW50cyBhIHN0 YW5kLWFsb25lCi0gICAgLy8gTlNVUkxDYWNoZSwgd2hpY2ggaXQgY2FuIGRpc2FibGUgdG8gc2F2 ZSBtZW1vcnkuCiAjaWYgUExBVEZPUk0oSU9TKQogICAgIGlmICghcGFyYW1ldGVycy51aVByb2Nl c3NCdW5kbGVJZGVudGlmaWVyLmlzTnVsbCgpKSB7CiAgICAgICAgIFtOU1VSTENhY2hlIHNldFNo YXJlZFVSTENhY2hlOmFkb3B0TlMoW1tOU1VSTENhY2hlIGFsbG9jXQo=