104615 2012-12-10 17:03:10 -0800 [Chromium] IndexedDB: Memory leak in IDBCallbacksProxy::onSuccess(PassRefPtr<IDBDatabaseBackendInterface>) 2012-12-14 14:33:01 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 104719 1 jsbell jsbell alecflett dgrogan tony webkit.review.bot oldest_to_newest 787856 0 jsbell 2012-12-10 17:03:10 -0800 https://code.google.com/p/chromium/issues/detail?id=122249 Valgrind says: { bug_122249d Memcheck:Leak fun:_Znw* fun:_ZN6WebKit17IDBCallbacksProxy9onSuccessEN3WTF10PassRefPtrIN7WebCore27IDBDatabaseBackendInterfaceEEE fun:_ZN7WebCore22IDBDatabaseBackendImpl19processPendingCallsEv fun:_ZN7WebCore22IDBDatabaseBackendImpl35transactionFinishedAndCompleteFiredEN3WTF10PassRefPtrINS_25IDBTransactionBackendImplEEE fun:_ZN7WebCore25IDBTransactionBackendImpl6commitEv } Leak is pretty obvious, here's a speculative fix but I haven't run it through valgrind yet: void WebIDBCallbacksImpl::onSuccess(WebIDBDatabase* webKitInstance) { + OwnPtr<WebIDBDatabase> webDatabase = adoptPtr(webKitInstance); if (m_databaseProxy) { m_callbacks->onSuccess(m_databaseProxy.release()); return; } - m_callbacks->onSuccess(IDBDatabaseBackendProxy::create(adoptPtr(webKitInstance))); + m_callbacks->onSuccess(IDBDatabaseBackendProxy::create(webDatabase.release())); } 787874 1 178674 jsbell 2012-12-10 17:24:13 -0800 Created attachment 178674 Patch 787875 2 jsbell 2012-12-10 17:25:24 -0800 dgrogan@, alecflett@ - please take a look Verified this addresses the leak w/ valgrind, at least in basics.html 787944 3 178674 dgrogan 2012-12-10 18:56:05 -0800 Comment on attachment 178674 Patch LGTM 787963 4 jsbell 2012-12-10 20:07:52 -0800 tony@ - r? 788553 5 178674 webkit.review.bot 2012-12-11 10:14:17 -0800 Comment on attachment 178674 Patch Clearing flags on attachment: 178674 Committed r137330: <http://trac.webkit.org/changeset/137330> 788554 6 webkit.review.bot 2012-12-11 10:14:21 -0800 All reviewed patches have been landed. Closing bug. 788800 7 webkit.review.bot 2012-12-11 14:25:56 -0800 Re-opened since this is blocked by bug 104719 791799 8 179521 jsbell 2012-12-14 13:15:29 -0800 Created attachment 179521 Patch 791804 9 jsbell 2012-12-14 13:17:22 -0800 The previous patch was on the receiving end, and didn't account for the "real" Chromium implementation which correct passed in the right objects. For the Chromium-DRT implementation, simply don't generate a new wrapper object if one was sent through in onUpgradeNeeded, and let it be ignored. dgrogan@ - another look? 791830 10 dgrogan 2012-12-14 13:57:20 -0800 In chromium, won't this cause IndexedDBCallbacksDatabase::onSuccess(WebKit::WebIDBDatabase* idb_object) to attempt to delete NULL? Also, to ensure I understand, this leak only happens in DRT, correct? Not chromium? 791841 11 jsbell 2012-12-14 14:05:15 -0800 (In reply to comment #10) > In chromium, won't this cause > > IndexedDBCallbacksDatabase::onSuccess(WebKit::WebIDBDatabase* idb_object) > > to attempt to delete NULL? Correct (but delete null is a safe no-op in C++) > Also, to ensure I understand, this leak only happens in DRT, correct? Not chromium? Correct. (Well, Chromium's DRT to be precise, it wouldn't affect other ports.) 791842 12 dgrogan 2012-12-14 14:06:07 -0800 LGTM 791847 13 jsbell 2012-12-14 14:10:41 -0800 tony@ - patch is completely different, so can you take another look? 791874 14 179521 webkit.review.bot 2012-12-14 14:32:57 -0800 Comment on attachment 179521 Patch Clearing flags on attachment: 179521 Committed r137773: <http://trac.webkit.org/changeset/137773> 791875 15 webkit.review.bot 2012-12-14 14:33:01 -0800 All reviewed patches have been landed. Closing bug. 178674 2012-12-10 17:24:13 -0800 2012-12-14 13:15:27 -0800 Patch bug-104615-20121210172135.patch text/plain 1771 jsbell U3VidmVyc2lvbiBSZXZpc2lvbjogMTM3MTc0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCmluZGV4 IDZmZjM4MjE0YTIwMmRjZDFjZTQyZGMyNmE2NTg2ZjQ4NTEzNTc1OWIuLmQ2YTU4MTRjYTA0ZmMw YjEzNWJjYTYwMDMxOGFjODVhYmZhY2I2NTcgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMTUgQEAKKzIwMTItMTItMTAgIEpvc2h1YSBCZWxsICA8anNiZWxsQGNocm9taXVt Lm9yZz4KKworICAgICAgICBbQ2hyb21pdW1dIEluZGV4ZWREQjogTWVtb3J5IGxlYWsgaW4gSURC Q2FsbGJhY2tzUHJveHk6Om9uU3VjY2VzcyhQYXNzUmVmUHRyPElEQkRhdGFiYXNlQmFja2VuZElu dGVyZmFjZT4pCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xMDQ2MTUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBFbnN1cmUgcGFzc2VkIHBvaW50ZXIgaXMgdHJlYXRlZCB3aXRoIG93bmVyc2hpcCBzZW1hbnRp Y3MuCisKKyAgICAgICAgKiBzcmMvV2ViSURCQ2FsbGJhY2tzSW1wbC5jcHA6CisgICAgICAgIChX ZWJLaXQ6OldlYklEQkNhbGxiYWNrc0ltcGw6Om9uU3VjY2Vzcyk6CisKIDIwMTItMTItMTAgIEFs ZXhpcyBNZW5hcmQgIDxhbGV4aXNAd2Via2l0Lm9yZz4KIAogICAgICAgICBbQ1NTMyBCYWNrZ3Jv dW5kcyBhbmQgQm9yZGVyc10gUmVtb3ZlIENTUzNfQkFDS0dST1VORCBmZWF0dXJlIGZsYWcuCmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJJREJDYWxsYmFja3NJbXBs LmNwcCBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL1dlYklEQkNhbGxiYWNrc0ltcGwuY3Bw CmluZGV4IDU3YzAzMTlkNDkyYWQ0NWIxMzI0ZWI2NzgwYjZhZWQyZjI3NDYyMDAuLmI2OWM4OTYy MWIzZTY1OGUwYmYyZDJkZTYyZWRhNzA5NzkzYTRjMTMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJL aXQvY2hyb21pdW0vc3JjL1dlYklEQkNhbGxiYWNrc0ltcGwuY3BwCisrKyBiL1NvdXJjZS9XZWJL aXQvY2hyb21pdW0vc3JjL1dlYklEQkNhbGxiYWNrc0ltcGwuY3BwCkBAIC03MywxMSArNzMsMTIg QEAgdm9pZCBXZWJJREJDYWxsYmFja3NJbXBsOjpvblN1Y2Nlc3MoV2ViSURCQ3Vyc29yKiBjdXJz b3IsIGNvbnN0IFdlYklEQktleSYga2V5LAogCiB2b2lkIFdlYklEQkNhbGxiYWNrc0ltcGw6Om9u U3VjY2VzcyhXZWJJREJEYXRhYmFzZSogd2ViS2l0SW5zdGFuY2UpCiB7CisgICAgT3duUHRyPFdl YklEQkRhdGFiYXNlPiB3ZWJEYXRhYmFzZSA9IGFkb3B0UHRyKHdlYktpdEluc3RhbmNlKTsKICAg ICBpZiAobV9kYXRhYmFzZVByb3h5KSB7CiAgICAgICAgIG1fY2FsbGJhY2tzLT5vblN1Y2Nlc3Mo bV9kYXRhYmFzZVByb3h5LnJlbGVhc2UoKSk7CiAgICAgICAgIHJldHVybjsKICAgICB9Ci0gICAg bV9jYWxsYmFja3MtPm9uU3VjY2VzcyhJREJEYXRhYmFzZUJhY2tlbmRQcm94eTo6Y3JlYXRlKGFk b3B0UHRyKHdlYktpdEluc3RhbmNlKSkpOworICAgIG1fY2FsbGJhY2tzLT5vblN1Y2Nlc3MoSURC RGF0YWJhc2VCYWNrZW5kUHJveHk6OmNyZWF0ZSh3ZWJEYXRhYmFzZS5yZWxlYXNlKCkpKTsKIH0K IAogdm9pZCBXZWJJREJDYWxsYmFja3NJbXBsOjpvblN1Y2Nlc3MoY29uc3QgV2ViSURCS2V5JiBr ZXkpCg== 179521 2012-12-14 13:15:29 -0800 2012-12-14 14:32:57 -0800 Patch bug-104615-20121214131249.patch text/plain 3109 jsbell U3VidmVyc2lvbiBSZXZpc2lvbjogMTM3NzM5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCmluZGV4 IDMwNTdjMGQ1ZWM4M2M1MjZmZWI0MTM3YzNkZTBjNzZmOTBkMjY0MmYuLmI5MDkxNWE3ZjY5MzY0 NjIxODg1Yjc4NjRmNmU2MDExMjYzMTFiZWEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMTkgQEAKKzIwMTItMTItMTQgIEpvc2h1YSBCZWxsICA8anNiZWxsQGNocm9taXVt Lm9yZz4KKworICAgICAgICBbQ2hyb21pdW1dIEluZGV4ZWREQjogTWVtb3J5IGxlYWsgaW4gSURC Q2FsbGJhY2tzUHJveHk6Om9uU3VjY2VzcyhQYXNzUmVmUHRyPElEQkRhdGFiYXNlQmFja2VuZElu dGVyZmFjZT4pCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xMDQ2MTUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBEb24ndCBjcmVhdGUgKGFuZCBsZWFrKSBhIG5ldyB3cmFwcGVyIG9iamVjdCBmb3Igb25TdWNj ZXNzIGlmIG9uZSB3YXMgY3JlYXRlZCBmb3Igb25VcGdyYWRlTmVlZGVkLgorCisgICAgICAgICog c3JjL0lEQkNhbGxiYWNrc1Byb3h5LmNwcDoKKyAgICAgICAgKFdlYktpdDo6SURCQ2FsbGJhY2tz UHJveHk6OklEQkNhbGxiYWNrc1Byb3h5KToKKyAgICAgICAgKFdlYktpdDo6SURCQ2FsbGJhY2tz UHJveHk6Om9uU3VjY2Vzcyk6CisgICAgICAgIChXZWJLaXQ6OklEQkNhbGxiYWNrc1Byb3h5Ojpv blVwZ3JhZGVOZWVkZWQpOgorICAgICAgICAqIHNyYy9JREJDYWxsYmFja3NQcm94eS5oOgorICAg ICAgICAoSURCQ2FsbGJhY2tzUHJveHkpOgorCiAyMDEyLTEyLTE0ICBEYXZpZCBEb3J3aW4gIDxk ZG9yd2luQGNocm9taXVtLm9yZz4KIAogICAgICAgICBFTUUgdjAuMTogUmVwb3J0IGRlZmF1bHRV UkwgaW4gS2V5TWVzc2FnZS4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3Jj L0lEQkNhbGxiYWNrc1Byb3h5LmNwcCBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL0lEQkNh bGxiYWNrc1Byb3h5LmNwcAppbmRleCA4MzdjYzMyYTZiMDAwMGRhZjk0N2IxZGU3NTFlNTY3NjA4 Mjg2ZGUxLi4xNjAxODRiZDk4NTQwZDgyNDkyZTFjMDU4YTQxYmNkNWFlMjExNzkwIDEwMDY0NAot LS0gYS9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9JREJDYWxsYmFja3NQcm94eS5jcHAKKysr IGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvSURCQ2FsbGJhY2tzUHJveHkuY3BwCkBAIC01 OSw2ICs1OSw3IEBAIFBhc3NSZWZQdHI8SURCQ2FsbGJhY2tzUHJveHk+IElEQkNhbGxiYWNrc1By b3h5OjpjcmVhdGUoUGFzc093blB0cjxXZWJJREJDYWxsYmFjCiAKIElEQkNhbGxiYWNrc1Byb3h5 OjpJREJDYWxsYmFja3NQcm94eShQYXNzT3duUHRyPFdlYklEQkNhbGxiYWNrcz4gY2FsbGJhY2tz KQogICAgIDogbV9jYWxsYmFja3MoY2FsbGJhY2tzKQorICAgICwgbV9kaWRDcmVhdGVQcm94eShm YWxzZSkKIHsKIH0KIApAQCAtNzksNyArODAsOCBAQCB2b2lkIElEQkNhbGxiYWNrc1Byb3h5Ojpv blN1Y2Nlc3MoUGFzc1JlZlB0cjxJREJDdXJzb3JCYWNrZW5kSW50ZXJmYWNlPiBpZGJDdXJzbwog dm9pZCBJREJDYWxsYmFja3NQcm94eTo6b25TdWNjZXNzKFBhc3NSZWZQdHI8SURCRGF0YWJhc2VC YWNrZW5kSW50ZXJmYWNlPiBiYWNrZW5kKQogewogICAgIEFTU0VSVChtX2RhdGFiYXNlQ2FsbGJh Y2tzLmdldCgpKTsKLSAgICBtX2NhbGxiYWNrcy0+b25TdWNjZXNzKG5ldyBXZWJJREJEYXRhYmFz ZUltcGwoYmFja2VuZCwgbV9kYXRhYmFzZUNhbGxiYWNrcy5yZWxlYXNlKCkpKTsKKyAgICBXZWJJ REJEYXRhYmFzZUltcGwqIGltcGwgPSBtX2RpZENyZWF0ZVByb3h5ID8gMCA6IG5ldyBXZWJJREJE YXRhYmFzZUltcGwoYmFja2VuZCwgbV9kYXRhYmFzZUNhbGxiYWNrcy5yZWxlYXNlKCkpOworICAg IG1fY2FsbGJhY2tzLT5vblN1Y2Nlc3MoaW1wbCk7CiB9CiAKIHZvaWQgSURCQ2FsbGJhY2tzUHJv eHk6Om9uU3VjY2VzcyhQYXNzUmVmUHRyPElEQktleT4gaWRiS2V5KQpAQCAtMTQ3LDYgKzE0OSw3 IEBAIHZvaWQgSURCQ2FsbGJhY2tzUHJveHk6Om9uQmxvY2tlZChpbnQ2NF90IGV4aXN0aW5nVmVy c2lvbikKIHZvaWQgSURCQ2FsbGJhY2tzUHJveHk6Om9uVXBncmFkZU5lZWRlZChpbnQ2NF90IG9s ZFZlcnNpb24sIFBhc3NSZWZQdHI8SURCVHJhbnNhY3Rpb25CYWNrZW5kSW50ZXJmYWNlPiB0cmFu c2FjdGlvbiwgUGFzc1JlZlB0cjxJREJEYXRhYmFzZUJhY2tlbmRJbnRlcmZhY2U+IGRhdGFiYXNl KQogewogICAgIEFTU0VSVChtX2RhdGFiYXNlQ2FsbGJhY2tzKTsKKyAgICBtX2RpZENyZWF0ZVBy b3h5ID0gdHJ1ZTsKICAgICBtX2NhbGxiYWNrcy0+b25VcGdyYWRlTmVlZGVkKG9sZFZlcnNpb24s IG5ldyBXZWJJREJUcmFuc2FjdGlvbkltcGwodHJhbnNhY3Rpb24pLCBuZXcgV2ViSURCRGF0YWJh c2VJbXBsKGRhdGFiYXNlLCBtX2RhdGFiYXNlQ2FsbGJhY2tzKSk7CiB9CiAKZGlmZiAtLWdpdCBh L1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL0lEQkNhbGxiYWNrc1Byb3h5LmggYi9Tb3VyY2Uv V2ViS2l0L2Nocm9taXVtL3NyYy9JREJDYWxsYmFja3NQcm94eS5oCmluZGV4IDY1ZjNiNzQwYTgy ZWU4YWUyZDgyYjAyNTkyM2JlMGU2NGUwYTQxNWEuLmU0ZTg4NDNkNjE0NWIzMzM2MjM4YmMyZGI1 N2NiNDMxOGEwNDVkZTYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL0lE QkNhbGxiYWNrc1Byb3h5LmgKKysrIGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvSURCQ2Fs bGJhY2tzUHJveHkuaApAQCAtNjgsNiArNjgsNyBAQCBwcml2YXRlOgogCiAgICAgT3duUHRyPFdl YklEQkNhbGxiYWNrcz4gbV9jYWxsYmFja3M7CiAgICAgUmVmUHRyPElEQkRhdGFiYXNlQ2FsbGJh Y2tzUHJveHk+IG1fZGF0YWJhc2VDYWxsYmFja3M7CisgICAgYm9vbCBtX2RpZENyZWF0ZVByb3h5 OwogfTsKIAogfSAvLyBuYW1lc3BhY2UgV2ViS2l0Cg==