23315 2009-01-14 05:43:54 -0800 Mismatched new[] / delete in ByteArray 2009-01-14 11:08:05 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 deanm webkit-unassigned oliver oldest_to_newest 105660 0 deanm 2009-01-14 05:43:54 -0800 A ByteArray created with ByteArray::create is allocated with new unsigned char[]. There is then a placement new to initialize the ByteArray object on top of this memory. ByteArray is RefCounted, so it is eventually destroyed with delete. In theory to do it properly, the destructor for ByteArray should be manually called, and then the memory should be freed with delete[]. My reading of the C++ standard allows new/delete and new[]/delete[] to use completely separate and incompatible allocators, which means it's important that a buffer allocated with new[] is freed with delete[]. 105661 1 deanm 2009-01-14 05:56:49 -0800 One option that might be generally useful is to make RefCounted take a traits, in which Traits::destroy(Type*) is responsible for destroying the object. The trait would default to something like DefaultRefCountedTrait, which would implement the destroy(Type*) as a delete T; ByteArray could then supply it's own to do the proper ByteArray::~ByteArray() destructor, and then delete[]. I can try to make this patch, but I imagine any modifications to RefCounted as controversial, so I'll wait for some feedback. Thanks 105695 2 darin 2009-01-14 10:03:31 -0800 It's important that new/delete and new[]/delete[] match, but I don't see how an array could be a RefCounted. Let me take a look. 105696 3 darin 2009-01-14 10:07:05 -0800 OK, I see now. I don't think we should add traits to RefCounted just so it can be used by ByteArray, though. It doesn't need to inherit from RefCounted. I think it should just inherit from RefCountedBase and implement its own deref function. 105719 4 ddkilzer 2009-01-14 10:39:14 -0800 See also Bug 23123. 105720 5 26717 deanm 2009-01-14 10:39:29 -0800 Created attachment 26717 Patch attempt. 105722 6 26717 darin 2009-01-14 10:43:00 -0800 Comment on attachment 26717 Patch attempt. r=me 105723 7 26717 oliver 2009-01-14 10:43:21 -0800 Comment on attachment 26717 Patch attempt. r=me, do you have commit rights? (i lose track) 105733 8 deanm 2009-01-14 10:54:11 -0800 I don't, so feel free to commit it for me. Thanks. 105740 9 oliver 2009-01-14 11:08:05 -0800 Committing to http://svn.webkit.org/repository/webkit/trunk ... M JavaScriptCore/ChangeLog M JavaScriptCore/runtime/ByteArray.h Committed r39897 26717 2009-01-14 10:39:29 -0800 2009-01-14 10:43:00 -0800 Patch attempt. ref.patch text/plain 1699 deanm ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCA1N2U0OGU2Li44YWEzOGI4IDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUg QEAKKzIwMDktMDEtMTQgIERlYW4gTWNOYW1lZSAgPGRlYW5tQGNocm9taXVtLm9yZz4KKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBDb3JyZWN0bHkgbWF0 Y2ggYWxsb2NhdGlvbiBmdW5jdGlvbnMgYnkgaW1wbGVtZW50aW5nIGEgY3VzdG9tIGRlcmVmKCku CisKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIzMzE1 CisKKyAgICAgICAgKiBydW50aW1lL0J5dGVBcnJheS5oOgorICAgICAgICAoSlNDOjpCeXRlQXJy YXk6OmRlcmVmKToKKyAgICAgICAgKEpTQzo6Qnl0ZUFycmF5OjpCeXRlQXJyYXkpOgorCiAyMDA5 LTAxLTE0ICBEYW4gQmVybnN0ZWluICA8bWl0ekBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3 ZWQgYnkgSm9obiBTdWxsaXZhbi4KZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUv Qnl0ZUFycmF5LmggYi9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0J5dGVBcnJheS5oCmluZGV4IDc0 NDg5NDIuLmI0NGE0YmYgMTAwNjQ0Ci0tLSBhL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvQnl0ZUFy cmF5LmgKKysrIGIvSmF2YVNjcmlwdENvcmUvcnVudGltZS9CeXRlQXJyYXkuaApAQCAtMzAsNyAr MzAsNyBAQAogI2luY2x1ZGUgInd0Zi9SZWZDb3VudGVkLmgiCiAKIG5hbWVzcGFjZSBKU0Mgewot ICAgIGNsYXNzIEJ5dGVBcnJheSA6IHB1YmxpYyBSZWZDb3VudGVkPEJ5dGVBcnJheT4geworICAg IGNsYXNzIEJ5dGVBcnJheSA6IHB1YmxpYyBXVEY6OlJlZkNvdW50ZWRCYXNlIHsKICAgICBwdWJs aWM6CiAgICAgICAgIHVuc2lnbmVkIGxlbmd0aCgpIGNvbnN0IHsgcmV0dXJuIG1fc2l6ZTsgfQog CkBAIC01NSwxMSArNTUsMjIgQEAgbmFtZXNwYWNlIEpTQyB7CiAKICAgICAgICAgdW5zaWduZWQg Y2hhciogZGF0YSgpIHsgcmV0dXJuIG1fZGF0YTsgfQogCisgICAgICAgIHZvaWQgZGVyZWYoKQor ICAgICAgICB7CisgICAgICAgICAgICBpZiAoZGVyZWZCYXNlKCkpIHsKKyAgICAgICAgICAgICAg ICAvLyBXZSBhbGxvY2F0ZWQgd2l0aCBuZXcgdW5zaWduZWQgY2hhcltdIGluIGNyZWF0ZSgpLAor ICAgICAgICAgICAgICAgIC8vIGFuZCB0aGVuIHVzZWQgcGxhY2VtZW50IG5ldyB0byBjb25zdHJ1 Y3QgdGhlIG9iamVjdC4KKyAgICAgICAgICAgICAgICB0aGlzLT5+Qnl0ZUFycmF5KCk7CisgICAg ICAgICAgICAgICAgZGVsZXRlW10gcmVpbnRlcnByZXRfY2FzdDx1bnNpZ25lZCBjaGFyKj4odGhp cyk7CisgICAgICAgICAgICB9CisgICAgICAgIH0KKwogICAgICAgICBzdGF0aWMgUGFzc1JlZlB0 cjxCeXRlQXJyYXk+IGNyZWF0ZShzaXplX3Qgc2l6ZSk7CiAKICAgICBwcml2YXRlOgogICAgICAg ICBCeXRlQXJyYXkoc2l6ZV90IHNpemUpCi0gICAgICAgICAgICA6IG1fc2l6ZShzaXplKQorICAg ICAgICAgICAgOiBSZWZDb3VudGVkQmFzZSgxKQorICAgICAgICAgICAgLCBtX3NpemUoc2l6ZSkK ICAgICAgICAgewogICAgICAgICB9CiAgICAgICAgIHNpemVfdCBtX3NpemU7Cg==