154852 2016-03-01 04:05:20 -0800 NetworkCache: Web process leaks resource buffer when using shareable reasources 2016-03-01 23:11:11 -0800 1 1 1 Unclassified WebKit WebKit2 WebKit Local Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 152316 1 cgarcia webkit-unassigned agomez ap bugs-noreply cdumez darin koivisto oldest_to_newest 1169410 0 cgarcia 2016-03-01 04:05:20 -0800 This is causing us running out of fds when using a web process limit of one after long time running, since we never release the shareable resources. 1169414 1 272558 cgarcia 2016-03-01 04:10:56 -0800 Created attachment 272558 Patch 1169437 2 272558 darin 2016-03-01 08:33:56 -0800 Comment on attachment 272558 Patch Better still to replace PassRefPtr with RefPtr&& or Ref&&, but I’m sure someone will come along to do that later. 1169449 3 cgarcia 2016-03-01 08:58:09 -0800 (In reply to comment #2) > Comment on attachment 272558 [details] > Patch > > Better still to replace PassRefPtr with RefPtr&& or Ref&&, but I’m sure > someone will come along to do that later. I thought about that, there's tricky part in SubresourceLoader::didReceiveDataOrBuffer that calls ResourceLoader::didReceiveDataOrBuffer, but yes, it can be done. I decided to leave that for a follow up patch, to ensure the leak is fixed first. 1169467 4 cgarcia 2016-03-01 09:42:35 -0800 Committed r197402: <http://trac.webkit.org/changeset/197402> 1169745 5 ap 2016-03-01 22:39:59 -0800 I can see how this avoids refcount churn, however I don't see how this can fix a leak. What am I missing? + ResourceLoader::didReceiveBuffer() expects a PassRefPtr, but we + are passing a raw pointer making PassRefPtr to take another + reference instead of transfering the ownership as expected. This was compensated by the original RefPtr still having a pointer to the object, so there was also one more deref(). 1169765 6 cgarcia 2016-03-01 23:11:11 -0800 (In reply to comment #5) > I can see how this avoids refcount churn, however I don't see how this can > fix a leak. What am I missing? > > + ResourceLoader::didReceiveBuffer() expects a PassRefPtr, but we > + are passing a raw pointer making PassRefPtr to take another > + reference instead of transfering the ownership as expected. > > This was compensated by the original RefPtr still having a pointer to the > object, so there was also one more deref(). hmm, you are indeed right. When passing the raw pointer, the PassRefPtr takes another ref but the original RefPtr releases its own when didReceiveResource() finishes. I was doing a lot of tests and didn't see any ShareableResource freed before applying this patch, but I guess I didn't disable the memory cache either. So, maybe there's no leak but when using a single web process we end up with a lot of resources cached in memory keeping their fds alive. We might consider copying the data in the web process and releasing the mmap. 272558 2016-03-01 04:10:56 -0800 2016-03-01 08:33:56 -0800 Patch wk2-shareable-resources-leaked.diff text/plain 1736 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCBiMjE4ODFmLi45OWM4ZWJmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMTkg QEAKIDIwMTYtMDMtMDEgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgogCisgICAgICAgIE5ldHdvcmtDYWNoZTogV2ViIHByb2Nlc3MgbGVha3MgcmVzb3VyY2UgYnVm ZmVyIHdoZW4gdXNpbmcgc2hhcmVhYmxlIHJlYXNvdXJjZXMKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1NDg1MgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFJlc291cmNlTG9hZGVyOjpkaWRSZWNlaXZlQnVm ZmVyKCkgZXhwZWN0cyBhIFBhc3NSZWZQdHIsIGJ1dCB3ZQorICAgICAgICBhcmUgcGFzc2luZyBh IHJhdyBwb2ludGVyIG1ha2luZyBQYXNzUmVmUHRyIHRvIHRha2UgYW5vdGhlcgorICAgICAgICBy ZWZlcmVuY2UgaW5zdGVhZCBvZiB0cmFuc2ZlcmluZyB0aGUgb3duZXJzaGlwIGFzIGV4cGVjdGVk LgorCisgICAgICAgICogV2ViUHJvY2Vzcy9OZXR3b3JrL1dlYlJlc291cmNlTG9hZGVyLmNwcDoK KyAgICAgICAgKFdlYktpdDo6V2ViUmVzb3VyY2VMb2FkZXI6OmRpZFJlY2VpdmVSZXNvdXJjZSk6 CisKKzIwMTYtMDMtMDEgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgorCiAgICAgICAgIFVucmV2aWV3ZWQuIFVwZGF0ZSBPcHRpb25zR1RLLmNtYWtlIGFuZCBORVdT IGZvciAyLjExLjkxIHJlbGVhc2UuCiAKICAgICAgICAgKiBndGsvTkVXUzogQWRkIHJlbGVhc2Ug bm90ZXMgZm9yIDIuMTEuOTEuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNz L05ldHdvcmsvV2ViUmVzb3VyY2VMb2FkZXIuY3BwIGIvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vz cy9OZXR3b3JrL1dlYlJlc291cmNlTG9hZGVyLmNwcAppbmRleCBkNGVlNzI0Li4yYTBmNTU5IDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL05ldHdvcmsvV2ViUmVzb3VyY2VM b2FkZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvTmV0d29yay9XZWJSZXNv dXJjZUxvYWRlci5jcHAKQEAgLTE5NCw4ICsxOTQsOCBAQCB2b2lkIFdlYlJlc291cmNlTG9hZGVy OjpkaWRSZWNlaXZlUmVzb3VyY2UoY29uc3QgU2hhcmVhYmxlUmVzb3VyY2U6OkhhbmRsZSYgaGFu ZAogICAgIFJlZjxXZWJSZXNvdXJjZUxvYWRlcj4gcHJvdGVjdCgqdGhpcyk7CiAKICAgICAvLyBP bmx5IHNlbmQgZGF0YSB0byB0aGUgZGlkUmVjZWl2ZURhdGEgY2FsbGJhY2sgaWYgaXQgZXhpc3Rz LgotICAgIGlmIChidWZmZXItPnNpemUoKSkKLSAgICAgICAgbV9jb3JlTG9hZGVyLT5kaWRSZWNl aXZlQnVmZmVyKGJ1ZmZlci5nZXQoKSwgYnVmZmVyLT5zaXplKCksIERhdGFQYXlsb2FkV2hvbGVS ZXNvdXJjZSk7CisgICAgaWYgKHVuc2lnbmVkIGJ1ZmZlclNpemUgPSBidWZmZXItPnNpemUoKSkK KyAgICAgICAgbV9jb3JlTG9hZGVyLT5kaWRSZWNlaXZlQnVmZmVyKGJ1ZmZlci5yZWxlYXNlKCks IGJ1ZmZlclNpemUsIERhdGFQYXlsb2FkV2hvbGVSZXNvdXJjZSk7CiAKICAgICBpZiAoIW1fY29y ZUxvYWRlcikKICAgICAgICAgcmV0dXJuOwo=