155701 2016-03-20 10:56:44 -0700 SmallPtrSet leaks memory in its move assignment operator when !this->isSmall() 2016-03-23 01:21:12 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 saam saam benjamin cdumez cmarcelo commit-queue darin fpizlo ggaren gskachkov keith_miller mark.lam msaboff oliver sam sukolsak ysuzuki oldest_to_newest 1176598 0 saam 2016-03-20 10:56:44 -0700 ... 1176602 1 274546 saam 2016-03-20 11:28:12 -0700 Created attachment 274546 patch 1176630 2 274546 darin 2016-03-20 16:35:00 -0700 Comment on attachment 274546 patch View in context: https://bugs.webkit.org/attachment.cgi?id=274546&action=review > Source/WTF/wtf/SmallPtrSet.h:60 > + m_capacity = SmallArraySize; Normally I prefer to write operator= based on the constructor rather than vice versa. This line of code shows what happens when we don't do it that way. Might be good to return here later and set that right. 1176680 3 saam 2016-03-20 22:56:37 -0700 (In reply to comment #2) > Comment on attachment 274546 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=274546&action=review > > > Source/WTF/wtf/SmallPtrSet.h:60 > > + m_capacity = SmallArraySize; > > Normally I prefer to write operator= based on the constructor rather than > vice versa. This line of code shows what happens when we don't do it that > way. Might be good to return here later and set that right. Do you mean something like this: ``` SmallPtrSet& operator=(SmallPtrSet&& other) { SmallPtrSet newThis(WTFMove(other)); swap(newThis); return *this; } ``` ? 1177162 4 darin 2016-03-22 15:27:27 -0700 Yes, something like that. In simple classes there may be further tricks to do it more efficiently. 1177241 5 274717 saam 2016-03-22 17:53:56 -0700 Created attachment 274717 patch I stole this idea from the B3 code. We can destruct ourself and then construct the new object in our place inside the move assignment operator. Maybe this is too sketchy. But it's also clever. I'm not 100% sure. What do you think? 1177272 6 darin 2016-03-22 21:24:50 -0700 If this generates more efficient code than the traditional swap idiom, I am OK with it. 1177311 7 274717 commit-queue 2016-03-23 01:21:06 -0700 Comment on attachment 274717 patch Clearing flags on attachment: 274717 Committed r198579: <http://trac.webkit.org/changeset/198579> 1177312 8 commit-queue 2016-03-23 01:21:12 -0700 All reviewed patches have been landed. Closing bug. 274546 2016-03-20 11:28:12 -0700 2016-03-22 17:53:56 -0700 patch a-backup.diff text/plain 1259 saam SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxOTg0NzUpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDE2LTAzLTIwICBTYWFtIGJhcmF0aSAgPHNiYXJh dGlAYXBwbGUuY29tPgorCisgICAgICAgIFNtYWxsUHRyU2V0IGxlYWtzIG1lbW9yeSBpbiBpdHMg bW92ZSBhc3NpZ25tZW50IG9wZXJhdG9yIHdoZW4gIXRoaXMtPmlzU21hbGwoKQorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU1NzAxCisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiB3dGYvU21hbGxQdHJTZXQu aDoKKyAgICAgICAgKFdURjo6U21hbGxQdHJTZXQ6OlNtYWxsUHRyU2V0KToKKyAgICAgICAgKFdU Rjo6U21hbGxQdHJTZXQ6Om9wZXJhdG9yPSk6CisKIDIwMTYtMDMtMjAgIERhbiBCZXJuc3RlaW4g IDxtaXR6QGFwcGxlLmNvbT4KIAogICAgICAgICBVcGRhdGUgYnVpbGQgc2V0dGluZ3MKSW5kZXg6 IFNvdXJjZS9XVEYvd3RmL1NtYWxsUHRyU2V0LmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dURi93 dGYvU21hbGxQdHJTZXQuaAkocmV2aXNpb24gMTk4NDc1KQorKysgU291cmNlL1dURi93dGYvU21h bGxQdHJTZXQuaAkod29ya2luZyBjb3B5KQpAQCAtNTcsMTEgKzU3LDE2IEBAIHB1YmxpYzogCiAK ICAgICBTbWFsbFB0clNldChTbWFsbFB0clNldCYmIG90aGVyKQogICAgIHsKKyAgICAgICAgbV9j YXBhY2l0eSA9IFNtYWxsQXJyYXlTaXplOworICAgICAgICBBU1NFUlQoaXNTbWFsbCgpKTsKICAg ICAgICAgKnRoaXMgPSBXVEZNb3ZlKG90aGVyKTsKICAgICB9CiAKICAgICBTbWFsbFB0clNldCYg b3BlcmF0b3I9KFNtYWxsUHRyU2V0JiYgb3RoZXIpCiAgICAgeworICAgICAgICBBU1NFUlQodGhp cyAhPSAmb3RoZXIpOworICAgICAgICBpZiAoIWlzU21hbGwoKSkKKyAgICAgICAgICAgIGZhc3RG cmVlKG1fYnVmZmVyKTsKICAgICAgICAgbWVtY3B5KHRoaXMsICZvdGhlciwgc2l6ZW9mKFNtYWxs UHRyU2V0KSk7CiAgICAgICAgIG90aGVyLmluaXRpYWxpemUoKTsKICAgICAgICAgcmV0dXJuICp0 aGlzOwo= 274717 2016-03-22 17:53:56 -0700 2016-03-23 01:21:06 -0700 patch a-backup.diff text/plain 1307 saam SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxOTg1NjUpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDE2LTAzLTIyICBTYWFtIEJhcmF0aSAgPHNiYXJh dGlAYXBwbGUuY29tPgorCisgICAgICAgIFNtYWxsUHRyU2V0IGxlYWtzIG1lbW9yeSBpbiBpdHMg bW92ZSBhc3NpZ25tZW50IG9wZXJhdG9yIHdoZW4gIXRoaXMtPmlzU21hbGwoKQorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU1NzAxCisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiB3dGYvU21hbGxQdHJTZXQu aDoKKyAgICAgICAgKFdURjo6U21hbGxQdHJTZXQ6OlNtYWxsUHRyU2V0KToKKyAgICAgICAgKFdU Rjo6U21hbGxQdHJTZXQ6Om9wZXJhdG9yPSk6CisKIDIwMTYtMDMtMjIgIFBlciBBcm5lIFZvbGxh biAgPHBlYXZvQG91dGxvb2suY29tPgogCiAgICAgICAgIFtXaW5dIFs2NC1iaXRdIFJlbW92ZSBN U1ZDIDIwMTMgRk1BMyBCdWcgV29ya2Fyb3VuZApJbmRleDogU291cmNlL1dURi93dGYvU21hbGxQ dHJTZXQuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV1RGL3d0Zi9TbWFsbFB0clNldC5oCShyZXZp c2lvbiAxOTg1NjUpCisrKyBTb3VyY2UvV1RGL3d0Zi9TbWFsbFB0clNldC5oCSh3b3JraW5nIGNv cHkpCkBAIC01NywxMyArNTcsMTQgQEAgcHVibGljOiAKIAogICAgIFNtYWxsUHRyU2V0KFNtYWxs UHRyU2V0JiYgb3RoZXIpCiAgICAgewotICAgICAgICAqdGhpcyA9IFdURk1vdmUob3RoZXIpOwor ICAgICAgICBtZW1jcHkodGhpcywgJm90aGVyLCBzaXplb2YoU21hbGxQdHJTZXQpKTsKKyAgICAg ICAgb3RoZXIuaW5pdGlhbGl6ZSgpOwogICAgIH0KIAogICAgIFNtYWxsUHRyU2V0JiBvcGVyYXRv cj0oU21hbGxQdHJTZXQmJiBvdGhlcikKICAgICB7Ci0gICAgICAgIG1lbWNweSh0aGlzLCAmb3Ro ZXIsIHNpemVvZihTbWFsbFB0clNldCkpOwotICAgICAgICBvdGhlci5pbml0aWFsaXplKCk7Cisg ICAgICAgIHRoaXMtPn5TbWFsbFB0clNldCgpOworICAgICAgICBuZXcgKHRoaXMpIFNtYWxsUHRy U2V0KFdURk1vdmUob3RoZXIpKTsKICAgICAgICAgcmV0dXJuICp0aGlzOwogICAgIH0KIAo=