33675 2010-01-14 07:51:12 -0800 [Android] DOMWrapperMap::Visitor needs virtual destructor 2010-01-18 04:54:54 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Android Android RESOLVED FIXED P2 Normal --- 0 andreip benm abarth android-webkit-unforking benm oldest_to_newest 180719 0 andreip 2010-01-14 07:51:12 -0800 For reasons similar to https://bugs.webkit.org/show_bug.cgi?id=33390 The DOMWrapperMap::Visitor class in V8DOMMap.h needs a virtual destructor. 180720 1 46568 andreip 2010-01-14 07:56:52 -0800 Created attachment 46568 Add the virtual destructor to DOMWrapperMap::Visitor. 181007 2 46568 abarth 2010-01-14 16:54:05 -0800 Comment on attachment 46568 Add the virtual destructor to DOMWrapperMap::Visitor. This isn't right. Vistor objects are never deleted via the Vistor type. Can we make the destructor private instead? 181022 3 46568 eric 2010-01-14 17:20:27 -0800 Comment on attachment 46568 Add the virtual destructor to DOMWrapperMap::Visitor. r- based on Adam's comment above. 181200 4 andreip 2010-01-15 04:24:04 -0800 (In reply to comment #2) > (From update of attachment 46568 [details]) > This isn't right. Vistor objects are never deleted via the Vistor type. Can > we make the destructor private instead? Well, they're not deleted now but if someone else added code to do so, we'd have a disaster on our hand, right? Clearly, we can make it private either as derived classes would not be able to call it. What we can do is make it protected, instead, if you are sure that one is never meant to delete objects instantiated from derived types via a pointer to the base type. 181202 5 46671 andreip 2010-01-15 04:29:52 -0800 Created attachment 46671 Add protected virtual destructor to DOMWrapperMap::Visitor. Made virtual dtor protected. Also fixed indentation of visitDOMWrapper. 181569 6 46671 abarth 2010-01-16 15:43:13 -0800 Comment on attachment 46671 Add protected virtual destructor to DOMWrapperMap::Visitor. Looks good, but please update the ChangeLog to match what the patch actually does. :) 181570 7 abarth 2010-01-16 15:44:42 -0800 Also, you patch does not apply properly to top of tree (according to the EWS). If you'd like this landed automatically, please either update your tree or create the patch using svn-create-patch or "webkit-patch upload". 181826 8 andreip 2010-01-18 04:44:11 -0800 Thanks for the review Adam. (In reply to comment #7) > Also, you patch does not apply properly to top of tree (according to the EWS). > If you'd like this landed automatically, please either update your tree or > create the patch using svn-create-patch or "webkit-patch upload". Hmm, but svn-create-patch is exactly what I used to create the patch. Anyway, Ben Murdoch was kind enough to help land this manually. 181827 9 benm 2010-01-18 04:47:19 -0800 Will update Changelog as requested and land manually. 181831 10 benm 2010-01-18 04:54:54 -0800 Landed as r53401. 46568 2010-01-14 07:56:52 -0800 2010-01-15 04:29:52 -0800 Add the virtual destructor to DOMWrapperMap::Visitor. 33675.patch text/plain 1142 andreip SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1MzI2NikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTcgQEAKKzIwMTAtMDEtMTQgIEFuZHJlaSBQb3Blc2N1ICA8YW5kcmVpcEBnb29n bGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IFtBbmRyb2lkXSBET01XcmFwcGVyTWFwOjpWaXNpdG9yIG5lZWRzIHZpcnR1YWwgZGVzdHJ1Y3Rv cgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzM2NzUK KworICAgICAgICBBZGQgdmlydHVhbCBkdG9yIHRvIERPTVdyYXBwZXJNYXA6OlZpc2l0b3IuCisK KyAgICAgICAgTm8gbmV3IHRlc3RzIG5lZWRlZCwgZnVuY3Rpb25hbGl0eSBub3QgY2hhbmdlZC4K KworICAgICAgICAqIGJpbmRpbmdzL3Y4L1Y4RE9NTWFwLmg6CisgICAgICAgIChXZWJDb3JlOjpE T01XcmFwcGVyTWFwOjpWaXNpdG9yOjp+VmlzaXRvcik6CisKIDIwMTAtMDEtMTQgIE1hcnRpbiBS b2JpbnNvbiAgPG1hcnRpbi5qYW1lcy5yb2JpbnNvbkBnbWFpbC5jb20+CiAKICAgICAgICAgUmV2 aWV3ZWQgYnkgWGFuIExvcGV6LgpJbmRleDogV2ViQ29yZS9iaW5kaW5ncy92OC9WOERPTU1hcC5o Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT0KLS0tIFdlYkNvcmUvYmluZGluZ3MvdjgvVjhET01NYXAuaAkocmV2aXNpb24g NTE5NzYpCisrKyBXZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4RE9NTWFwLmgJKHdvcmtpbmcgY29weSkK QEAgLTk3LDYgKzk3LDcgQEAgbmFtZXNwYWNlIFdlYkNvcmUgewogICAgICAgICBjbGFzcyBWaXNp dG9yIHsKICAgICAgICAgcHVibGljOgogICAgICAgICAgIHZpcnR1YWwgdm9pZCB2aXNpdERPTVdy YXBwZXIoS2V5VHlwZSoga2V5LCB2ODo6UGVyc2lzdGVudDx2ODo6T2JqZWN0PiBvYmplY3QpID0g MDsKKyAgICAgICAgICB2aXJ0dWFsIH5WaXNpdG9yKCkgeyB9CiAgICAgICAgIH07CiAgICAgfTsK IAo= 46671 2010-01-15 04:29:52 -0800 2010-01-16 15:43:13 -0800 Add protected virtual destructor to DOMWrapperMap::Visitor. 33675.patch text/plain 1262 andreip SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1MzI2NikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTcgQEAKKzIwMTAtMDEtMTQgIEFuZHJlaSBQb3Blc2N1ICA8YW5kcmVpcEBnb29n bGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IFtBbmRyb2lkXSBET01XcmFwcGVyTWFwOjpWaXNpdG9yIG5lZWRzIHZpcnR1YWwgZGVzdHJ1Y3Rv cgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzM2NzUK KworICAgICAgICBBZGQgdmlydHVhbCBkdG9yIHRvIERPTVdyYXBwZXJNYXA6OlZpc2l0b3IuCisK KyAgICAgICAgTm8gbmV3IHRlc3RzIG5lZWRlZCwgZnVuY3Rpb25hbGl0eSBub3QgY2hhbmdlZC4K KworICAgICAgICAqIGJpbmRpbmdzL3Y4L1Y4RE9NTWFwLmg6CisgICAgICAgIChXZWJDb3JlOjpE T01XcmFwcGVyTWFwOjpWaXNpdG9yOjp+VmlzaXRvcik6CisKIDIwMTAtMDEtMTQgIE1hcnRpbiBS b2JpbnNvbiAgPG1hcnRpbi5qYW1lcy5yb2JpbnNvbkBnbWFpbC5jb20+CiAKICAgICAgICAgUmV2 aWV3ZWQgYnkgWGFuIExvcGV6LgpJbmRleDogV2ViQ29yZS9iaW5kaW5ncy92OC9WOERPTU1hcC5o Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT0KLS0tIFdlYkNvcmUvYmluZGluZ3MvdjgvVjhET01NYXAuaAkocmV2aXNpb24g NTE5NzYpCisrKyBXZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4RE9NTWFwLmgJKHdvcmtpbmcgY29weSkK QEAgLTk2LDcgKzk2LDkgQEAgbmFtZXNwYWNlIFdlYkNvcmUgewogCiAgICAgICAgIGNsYXNzIFZp c2l0b3IgewogICAgICAgICBwdWJsaWM6Ci0gICAgICAgICAgdmlydHVhbCB2b2lkIHZpc2l0RE9N V3JhcHBlcihLZXlUeXBlKiBrZXksIHY4OjpQZXJzaXN0ZW50PHY4OjpPYmplY3Q+IG9iamVjdCkg PSAwOworICAgICAgICAgICAgdmlydHVhbCB2b2lkIHZpc2l0RE9NV3JhcHBlcihLZXlUeXBlKiBr ZXksIHY4OjpQZXJzaXN0ZW50PHY4OjpPYmplY3Q+IG9iamVjdCkgPSAwOworICAgICAgICBwcm90 ZWN0ZWQ6CisgICAgICAgICAgICB2aXJ0dWFsIH5WaXNpdG9yKCkgeyB9CiAgICAgICAgIH07CiAg ICAgfTsKIAo=