26347 2009-06-12 11:22:10 -0700 UString leaks memory when creating sharedBuffers for SmallStrings 2009-06-15 10:34:41 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Mac OS X 10.5 RESOLVED WONTFIX P2 Normal --- 1 atwilson atwilson levin oldest_to_newest 125488 0 atwilson 2009-06-12 11:22:10 -0700 The sharedBuffer code in UString creates shared buffers for small strings, because the code just checks the BaseString.len field which is 256 (the BaseString is shared for all the SmallStrings). Since the items in SmallStringStorage.m_reps never have destroy() called on them, the sharedBuffer leaks. I think it's a good idea to fix the bug so we don't create sharedBuffer objects for SmallStrings, but it's also a good idea to change SmallStringsStorage to properly clean up its contents (right now, none of its Rep instances or its BaseString instance have their destroy() function invoked, which will undoubtedly lead to more leaks in the future). 125491 1 levin 2009-06-12 11:30:08 -0700 I know what to do for this one. 125494 2 atwilson 2009-06-12 11:34:08 -0700 I'm working on a patch locally to change how SmallStringsStorage handles its internal references, btw. You should still make the sharedBuffer change though. 125658 3 31243 atwilson 2009-06-13 12:12:45 -0700 Created attachment 31243 proposed patch 125755 4 31243 eric 2009-06-14 02:22:08 -0700 Comment on attachment 31243 proposed patch We run run-webkit-tests with --leaks from time to time. So it is theoretically possible to make a test case for this if the tests don't already trip the leak. 125773 5 atwilson 2009-06-14 09:05:03 -0700 With levin's fix for bug 26360, there's no longer a leak. I'm presuming that his layout tests for that bug will cover this case as well. This patch is a cleanup to make sure a similar class of leaks can't happen in the future. That said, I have been able to reproduce this using my layout tests for MessagePorts and workers, but I can't check in that test yet as it is pending yet another patch. 125790 6 31243 darin 2009-06-14 11:41:14 -0700 Comment on attachment 31243 proposed patch This seems quite awkward to me. Is there some way we can address this without adding so much new explicit code to the SmallStringsStorage class? > + (JSC::SmallStringsStorage::~SmallStringsStorage): > + Manually invokes destroy on the locally-defined Rep objects. Tab in the change log here. The person landing this will have to fix it. > + * runtime/UString.cpp: > + (JSC::UString::Rep::destroy): > + Kept for backwards compatibility (exported API) Compatibility with what? Exported where? These implementation details of JavaScriptCore should be internal and WebCore and JavaScriptGlue should be the only clients. I think this comment is a bit misleading. > + * runtime/UString.h: > + (JSC::UString::Rep::deref): > + Changed to call the new destroyInternal() API. This patch puts its new ChangeLog entry in the middle of the new file, not at the top. > + // Cleans up the internal references for this object and frees it > void destroy(); > > + // Cleans up internal references with the option of freeing the object > + void destroyInternal(bool freeObject); Maybe we can come up with a name clearer than "destroyInternal" for this. It seems weak to have a boolean just to control whether "delete this" is called. I suggest adding a function that does all of destroy except for the "delete this" part and then have the normal destroy call it first, and then do "delete this". If we can't think of a better name, then I suggest destroyWithoutDeleting() for the new function. Lets avoid the mysterious boolean design pattern. I think it's confusing to refer to "delete this" in comments as "freeing the object". I'd much prefer that it say "deleting". I'm going to say review- due to the comments above, but I agree it is crucial that we fix this soon since we just introduced the leak. 125803 7 levin 2009-06-14 13:04:36 -0700 I hate to add this comment because I know you spent a lot time tracking this down (and it was my bug). While it could be argued that the particular structure of this class caused the bug, it really was a bug in my change that happened in UString, and it has now been fixed. SmallString is tricky in how it does things but it addresses the use of single letter string which seems to be common. I'm concerned that this change may make the performance worse because it changes the locality of the items in this class. Right now all of the data is sequential in one memory location, so the data layout looks like this: m_characters 512 bytes m_base 48 bytes m_reps 6144 bytes After the change, the data layout would look more like this m_characters 4 bytes --> pointer to real location m_base 4 bytes -> pointer to real location m_reps 6144 bytes Those pointers go to some other page(s) in memory. Given this and that the bug has been fixed. I don't think this patch should be done. However, if this change goes forward anyway, you need to change the "new UChar[numCharactersToStore]" to use fastMalloc (because that is how UString allocates it m_data) but you could just use Rep::createEmptyBuffer to create the BaseString (and there is no need to keep the m_characters member no matter what). 125980 8 atwilson 2009-06-15 10:34:41 -0700 Dave has a good point about changing the memory locality of SmallStrings. Rather than try to address Darin's comments, I'm marking this as WONTFIX. The code is leak-free now, and it doesn't seem worth it to make potentially performance-affecting changes in code that is fragile but not actually broken. 31243 2009-06-13 12:12:45 -0700 2009-06-14 11:41:14 -0700 proposed patch patch.txt text/plain 5978 atwilson ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCBkY2YzNzI2Li5iZjhkYzJkIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xODQsNiArMTg0 LDMwIEBACiAgICAgICAgIChKU0M6OkpTT05PYmplY3Q6OmNsYXNzSW5mbyk6CiAgICAgICAgIChK U0M6OkpTT05PYmplY3Q6OmNyZWF0ZVN0cnVjdHVyZSk6CiAKKzIwMDktMDYtMTMgIERyZXcgV2ls c29uICA8YXR3aWxzb25AZ29vZ2xlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICA8aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTI2MzQ3PgorICAgICAgICBDbGVhbmVkIHVwIFNtYWxsU3RyaW5nc1N0b3JhZ2UgdG8gcHJv cGVybHkgY2xlYW4gdXAgaXRzIFVTdHJpbmc6OlJlcCBhbmQgVVN0cmluZzo6QmFzZVN0cmluZyBp bnN0YW5jZXMuCisgICAgICAgIFRoaXMgc2hvdWxkIGF2b2lkIGZ1dHVyZSBtZW1vcnkgbGVha3Mg bGlrZSB0aGUgb25lIGRlc2NyaWJlZCBpbiA8aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTI2MzYwPgorCisgICAgICAgICogSmF2YVNjcmlwdENvcmUuZXhwOgorICAgICAg ICBFeHBvcnRlZCBkZXN0cm95SW50ZXJuYWwoKSBzbyBkZXBlbmRlbnQgbGlicmFyaWVzIGNhbiBj YWxsIGl0LgorICAgICAgICAqIHJ1bnRpbWUvU21hbGxTdHJpbmdzLmNwcDoKKyAgICAgICAgKEpT Qzo6U21hbGxTdHJpbmdzU3RvcmFnZTo6U21hbGxTdHJpbmdzU3RvcmFnZSk6CisgICAgICAgIE5v dyBkeW5hbWljYWxseSBhbGxvY2F0ZXMgQmFzZVN0cmluZyBzbyBpdCBjYW4gYmUgcHJvcGVybHkg cmVmLWNvdW50ZWQgYW5kIGRlc3Ryb3llZC4KKyAgICAgICAgKEpTQzo6U21hbGxTdHJpbmdzU3Rv cmFnZTo6flNtYWxsU3RyaW5nc1N0b3JhZ2UpOgorCU1hbnVhbGx5IGludm9rZXMgZGVzdHJveSBv biB0aGUgbG9jYWxseS1kZWZpbmVkIFJlcCBvYmplY3RzLgorICAgICAgICAqIHJ1bnRpbWUvVVN0 cmluZy5jcHA6CisgICAgICAgIChKU0M6OlVTdHJpbmc6OlJlcDo6ZGVzdHJveSk6CisgICAgICAg IEtlcHQgZm9yIGJhY2t3YXJkcyBjb21wYXRpYmlsaXR5IChleHBvcnRlZCBBUEkpCisgICAgICAg IChKU0M6OlVTdHJpbmc6OlJlcDo6ZGVzdHJveUludGVybmFsKToKKyAgICAgICAgQWRkZWQgYWJp bGl0eSB0byBjbGVhbiB1cCBvYmplY3Qgd2l0aG91dCBhY3R1YWxseSBmcmVlaW5nIHRoZSByZWZl cmVuY2UsIHNpbmNlIFNtYWxsU3RyaW5nU3RvcmFnZSB1c2VzIHN0YXRpY2FsbHktZGVmaW5lZCBS ZXAgb2JqZWN0cy4KKyAgICAgICAgKiBydW50aW1lL1VTdHJpbmcuaDoKKyAgICAgICAgKEpTQzo6 VVN0cmluZzo6UmVwOjpkZXJlZik6CisgICAgICAgIENoYW5nZWQgdG8gY2FsbCB0aGUgbmV3IGRl c3Ryb3lJbnRlcm5hbCgpIEFQSS4KKwogMjAwOS0wNi0wOSAgRHJldyBXaWxzb24gIDxhdHdpbHNv bkBnb29nbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgpkaWZm IC0tZ2l0IGEvSmF2YVNjcmlwdENvcmUvSmF2YVNjcmlwdENvcmUuZXhwIGIvSmF2YVNjcmlwdENv cmUvSmF2YVNjcmlwdENvcmUuZXhwCmluZGV4IDdmYjhmMDQuLjUzNzMxMDQgMTAwNjQ0Ci0tLSBh L0phdmFTY3JpcHRDb3JlL0phdmFTY3JpcHRDb3JlLmV4cAorKysgYi9KYXZhU2NyaXB0Q29yZS9K YXZhU2NyaXB0Q29yZS5leHAKQEAgLTIyMyw2ICsyMjMsNyBAQCBfX1pOM0pTQzdVU3RyaW5nM1Jl cDExY29tcHV0ZUhhc2hFUEt0aQogX19aTjNKU0M3VVN0cmluZzNSZXAxNG51bGxCYXNlU3RyaW5n RQogX19aTjNKU0M3VVN0cmluZzNSZXA2Y3JlYXRlRVB0aU4zV1RGMTBQYXNzUmVmUHRySU5TM18y MUNyb3NzVGhyZWFkUmVmQ291bnRlZElOUzNfMTZPd25GYXN0TWFsbG9jUHRySXRFRUVFRUUKIF9f Wk4zSlNDN1VTdHJpbmczUmVwN2Rlc3Ryb3lFdgorX19aTjNKU0M3VVN0cmluZzNSZXAxNWRlc3Ry b3lJbnRlcm5hbEViCiBfX1pOM0pTQzdVU3RyaW5nNGZyb21FaQogX19aTjNKU0M3VVN0cmluZzRm cm9tRWoKIF9fWk4zSlNDN1VTdHJpbmc2YXBwZW5kRVBLYwpkaWZmIC0tZ2l0IGEvSmF2YVNjcmlw dENvcmUvcnVudGltZS9TbWFsbFN0cmluZ3MuY3BwIGIvSmF2YVNjcmlwdENvcmUvcnVudGltZS9T bWFsbFN0cmluZ3MuY3BwCmluZGV4IDg3YjQ5ZjAuLjM5ZWI3MTIgMTAwNjQ0Ci0tLSBhL0phdmFT Y3JpcHRDb3JlL3J1bnRpbWUvU21hbGxTdHJpbmdzLmNwcAorKysgYi9KYXZhU2NyaXB0Q29yZS9y dW50aW1lL1NtYWxsU3RyaW5ncy5jcHAKQEAgLTM3LDIzICszNywyNCBAQCBzdGF0aWMgY29uc3Qg dW5zaWduZWQgbnVtQ2hhcmFjdGVyc1RvU3RvcmUgPSAweDEwMDsKIGNsYXNzIFNtYWxsU3RyaW5n c1N0b3JhZ2UgOiBOb25jb3B5YWJsZSB7CiBwdWJsaWM6CiAgICAgU21hbGxTdHJpbmdzU3RvcmFn ZSgpOworICAgIH5TbWFsbFN0cmluZ3NTdG9yYWdlKCk7CiAKICAgICBVU3RyaW5nOjpSZXAqIHJl cCh1bnNpZ25lZCBjaGFyIGNoYXJhY3RlcikgeyByZXR1cm4gJm1fcmVwc1tjaGFyYWN0ZXJdOyB9 CiAKIHByaXZhdGU6Ci0gICAgVUNoYXIgbV9jaGFyYWN0ZXJzW251bUNoYXJhY3RlcnNUb1N0b3Jl XTsKLSAgICBVU3RyaW5nOjpCYXNlU3RyaW5nIG1fYmFzZTsKKyAgICBVQ2hhciogbV9jaGFyYWN0 ZXJzOyAgICAgLy8gRnJlZWQgYnkgbV9iYXNlLmRlc3Ryb3lJbnRlcm5hbCgpCisgICAgUmVmUHRy PFVTdHJpbmc6OkJhc2VTdHJpbmc+IG1fYmFzZTsKICAgICBVU3RyaW5nOjpSZXAgbV9yZXBzW251 bUNoYXJhY3RlcnNUb1N0b3JlXTsKIH07CiAKIFNtYWxsU3RyaW5nc1N0b3JhZ2U6OlNtYWxsU3Ry aW5nc1N0b3JhZ2UoKQotICAgIDogbV9iYXNlKG1fY2hhcmFjdGVycywgbnVtQ2hhcmFjdGVyc1Rv U3RvcmUpCisgICAgOiBtX2NoYXJhY3RlcnMobmV3IFVDaGFyW251bUNoYXJhY3RlcnNUb1N0b3Jl XSkKKyAgICAsIG1fYmFzZShhZG9wdFJlZihuZXcgVVN0cmluZzo6QmFzZVN0cmluZyhtX2NoYXJh Y3RlcnMsIG51bUNoYXJhY3RlcnNUb1N0b3JlKSkpCiB7Ci0gICAgbV9iYXNlLnJjID0gbnVtQ2hh cmFjdGVyc1RvU3RvcmUgKyAxOwogICAgIC8vIG1ha2Ugc3VyZSBVU3RyaW5nIGRvZXNuJ3QgdHJ5 IHRvIHJldXNlIHRoZSBidWZmZXIgYnkgcHJldGVuZGluZyB3ZSBoYXZlIG9uZSBtb3JlIGNoYXJh Y3RlciBpbiBpdAotICAgIG1fYmFzZS51c2VkQ2FwYWNpdHkgPSBudW1DaGFyYWN0ZXJzVG9TdG9y ZSArIDE7Ci0gICAgbV9iYXNlLmNhcGFjaXR5ID0gbnVtQ2hhcmFjdGVyc1RvU3RvcmUgKyAxOwot ICAgIG1fYmFzZS5jaGVja0NvbnNpc3RlbmN5KCk7CisgICAgbV9iYXNlLT51c2VkQ2FwYWNpdHkg PSBudW1DaGFyYWN0ZXJzVG9TdG9yZSArIDE7CisgICAgbV9iYXNlLT5jYXBhY2l0eSA9IG51bUNo YXJhY3RlcnNUb1N0b3JlICsgMTsKKyAgICBtX2Jhc2UtPmNoZWNrQ29uc2lzdGVuY3koKTsKIAog ICAgIGZvciAodW5zaWduZWQgaSA9IDA7IGkgPCBudW1DaGFyYWN0ZXJzVG9TdG9yZTsgKytpKQog ICAgICAgICBtX2NoYXJhY3RlcnNbaV0gPSBpOwpAQCAtNjMsMTEgKzY0LDE5IEBAIFNtYWxsU3Ry aW5nc1N0b3JhZ2U6OlNtYWxsU3RyaW5nc1N0b3JhZ2UoKQogICAgICAgICBtX3JlcHNbaV0ub2Zm c2V0ID0gaTsKICAgICAgICAgbV9yZXBzW2ldLmxlbiA9IDE7CiAgICAgICAgIG1fcmVwc1tpXS5y YyA9IDE7Ci0gICAgICAgIG1fcmVwc1tpXS5zZXRCYXNlU3RyaW5nKCZtX2Jhc2UpOworICAgICAg ICBtX3JlcHNbaV0uc2V0QmFzZVN0cmluZyhtX2Jhc2UpOwogICAgICAgICBtX3JlcHNbaV0uY2hl Y2tDb25zaXN0ZW5jeSgpOwogICAgIH0KIH0KIAorCitTbWFsbFN0cmluZ3NTdG9yYWdlOjp+U21h bGxTdHJpbmdzU3RvcmFnZSgpCit7CisgICAgZm9yICh1bnNpZ25lZCBpID0gMDsgaSA8IG51bUNo YXJhY3RlcnNUb1N0b3JlOyArK2kpIHsKKyAgICAgICAgbV9yZXBzW2ldLmRlc3Ryb3lJbnRlcm5h bChmYWxzZSk7CisgICAgfQorfQorCiBTbWFsbFN0cmluZ3M6OlNtYWxsU3RyaW5ncygpCiAgICAg OiBtX2VtcHR5U3RyaW5nKDApCiAgICAgLCBtX3N0b3JhZ2UoMCkKZGlmZiAtLWdpdCBhL0phdmFT Y3JpcHRDb3JlL3J1bnRpbWUvVVN0cmluZy5jcHAgYi9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1VT dHJpbmcuY3BwCmluZGV4IDExODc1MWUuLjIyOThkNDMgMTAwNjQ0Ci0tLSBhL0phdmFTY3JpcHRD b3JlL3J1bnRpbWUvVVN0cmluZy5jcHAKKysrIGIvSmF2YVNjcmlwdENvcmUvcnVudGltZS9VU3Ry aW5nLmNwcApAQCAtMjU0LDYgKzI1NCwxMSBAQCBVU3RyaW5nOjpTaGFyZWRVQ2hhciogVVN0cmlu Zzo6UmVwOjpzaGFyZWRCdWZmZXIoKQogCiB2b2lkIFVTdHJpbmc6OlJlcDo6ZGVzdHJveSgpCiB7 CisgICAgZGVzdHJveUludGVybmFsKHRydWUpOworfQorCit2b2lkIFVTdHJpbmc6OlJlcDo6ZGVz dHJveUludGVybmFsKGJvb2wgZnJlZU9iamVjdCkKK3sKICAgICBjaGVja0NvbnNpc3RlbmN5KCk7 CiAKICAgICAvLyBTdGF0aWMgbnVsbCBhbmQgZW1wdHkgc3RyaW5ncyBjYW4gbmV2ZXIgYmUgZGVz dHJveWVkLCBidXQgd2UgY2Fubm90IHJlbHkgb24gCkBAIC0yNzEsNyArMjc2LDggQEAgdm9pZCBV U3RyaW5nOjpSZXA6OmRlc3Ryb3koKQogICAgICAgICB9IGVsc2UKICAgICAgICAgICAgIGJhc2Ut PmRlcmVmKCk7CiAKLSAgICAgICAgZGVsZXRlIHRoaXM7CisgICAgICAgIGlmIChmcmVlT2JqZWN0 KQorICAgICAgICAgICAgZGVsZXRlIHRoaXM7CiAgICAgfQogfQogCmRpZmYgLS1naXQgYS9KYXZh U2NyaXB0Q29yZS9ydW50aW1lL1VTdHJpbmcuaCBiL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvVVN0 cmluZy5oCmluZGV4IGQwMWI3NWQuLjcxMTU4N2IgMTAwNjQ0Ci0tLSBhL0phdmFTY3JpcHRDb3Jl L3J1bnRpbWUvVVN0cmluZy5oCisrKyBiL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvVVN0cmluZy5o CkBAIC0xMDgsOCArMTA4LDEzIEBAIG5hbWVzcGFjZSBKU0MgewogICAgICAgICAgICAgc3RhdGlj IFBhc3NSZWZQdHI8UmVwPiBjcmVhdGUoVUNoYXIqLCBpbnQsIFBhc3NSZWZQdHI8U2hhcmVkVUNo YXI+KTsKIAogICAgICAgICAgICAgU2hhcmVkVUNoYXIqIHNoYXJlZEJ1ZmZlcigpOworCisgICAg ICAgICAgICAvLyBDbGVhbnMgdXAgdGhlIGludGVybmFsIHJlZmVyZW5jZXMgZm9yIHRoaXMgb2Jq ZWN0IGFuZCBmcmVlcyBpdAogICAgICAgICAgICAgdm9pZCBkZXN0cm95KCk7CiAKKyAgICAgICAg ICAgIC8vIENsZWFucyB1cCBpbnRlcm5hbCByZWZlcmVuY2VzIHdpdGggdGhlIG9wdGlvbiBvZiBm cmVlaW5nIHRoZSBvYmplY3QKKyAgICAgICAgICAgIHZvaWQgZGVzdHJveUludGVybmFsKGJvb2wg ZnJlZU9iamVjdCk7CisKICAgICAgICAgICAgIGJvb2wgYmFzZUlzU2VsZigpIGNvbnN0IHsgcmV0 dXJuIG1faWRlbnRpZmllclRhYmxlQW5kRmxhZ3MuaXNGbGFnU2V0KEJhc2VTdHJpbmdGbGFnKTsg fQogICAgICAgICAgICAgVUNoYXIqIGRhdGEoKSBjb25zdDsKICAgICAgICAgICAgIGludCBzaXpl KCkgY29uc3QgeyByZXR1cm4gbGVuOyB9CkBAIC0xMzEsNyArMTM2LDcgQEAgbmFtZXNwYWNlIEpT QyB7CiAgICAgICAgICAgICBjb25zdCBCYXNlU3RyaW5nKiBiYXNlU3RyaW5nKCkgY29uc3Q7CiAK ICAgICAgICAgICAgIFJlcCogcmVmKCkgeyArK3JjOyByZXR1cm4gdGhpczsgfQotICAgICAgICAg ICAgQUxXQVlTX0lOTElORSB2b2lkIGRlcmVmKCkgeyBpZiAoLS1yYyA9PSAwKSBkZXN0cm95KCk7 IH0KKyAgICAgICAgICAgIEFMV0FZU19JTkxJTkUgdm9pZCBkZXJlZigpIHsgaWYgKC0tcmMgPT0g MCkgZGVzdHJveUludGVybmFsKHRydWUpOyB9CiAKICAgICAgICAgICAgIHZvaWQgY2hlY2tDb25z aXN0ZW5jeSgpIGNvbnN0OwogICAgICAgICAgICAgZW51bSBVU3RyaW5nRmxhZ3Mgewo=