27843 2009-07-30 11:26:21 -0700 (V8) Invalid member function pointer hashing in SVG bindings. 2009-07-30 12:32:19 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 0 deanm webkit-unassigned darin dglazkov oldest_to_newest 135894 0 deanm 2009-07-30 11:26:21 -0700 There is some code to create and reuse SVG wrappers in the binding code. Previously it hashed a struct with 1 pointer and 2 member function pointers. The problem is member function pointers are opaque, and at least on MSVC cannot be hashed reliably (and it doesn't make sense to try to read the opaque bytes). See http://www.codeproject.com/KB/cpp/FastDelegate.aspx for more details on the implementations. There are a few options, all of them involve not trying to hash the member function pointer. One would be to remove member function pointers all together and create thunk wrappers. Another option would be to use something like a std::map, since the compiler should implement operator<, etc correctly. The option I took was to add some more hashing material during the binding generation, so we can hash the pointer and a generated field hash together. It would probably be best to remove the use of member function pointers all together but that is a much bigger task. 135896 1 33794 deanm 2009-07-30 11:31:33 -0700 Created attachment 33794 Patch 135899 2 deanm 2009-07-30 11:35:04 -0700 (In reply to comment #0) > There is some code to create and reuse SVG wrappers in the binding code. > Previously it hashed a struct with 1 pointer and 2 member function pointers. > The problem is member function pointers are opaque, and at least on MSVC cannot > be hashed reliably (and it doesn't make sense to try to read the opaque bytes). > > See http://www.codeproject.com/KB/cpp/FastDelegate.aspx for more details on the > implementations. > > There are a few options, all of them involve not trying to hash the member > function pointer. One would be to remove member function pointers all together > and create thunk wrappers. Another option would be to use something like a > std::map, since the compiler should implement operator<, etc correctly. (It was just pointed out to me that the compiler only gives you == and != for member function pointers, so std::map is not a possible solution). > > The option I took was to add some more hashing material during the binding > generation, so we can hash the pointer and a generated field hash together. It > would probably be best to remove the use of member function pointers all > together but that is a much bigger task. 135929 3 33794 dglazkov 2009-07-30 12:18:28 -0700 Comment on attachment 33794 Patch r=me. 135936 4 dglazkov 2009-07-30 12:32:19 -0700 Landed as http://trac.webkit.org/changeset/46596. 33794 2009-07-30 11:31:33 -0700 2009-07-30 12:18:28 -0700 Patch x.diff text/plain 5610 deanm ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg NzViMWFiOS4uMjgyZjg5ZiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNyBAQAorMjAwOS0wNy0zMCAgRGVhbiBNY05hbWVl ICA8ZGVhbm1AY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIERvbid0IHRyeSB0byBoYXNoIG1lbWJlciBmdW5jdGlvbiBwb2ludGVy cywgaW5zdGVhZCB1c2UgYSBwcmVjb21wdXRlZCB2YWx1ZSBiYXNlZCBvbiB0aGUgZmllbGQuCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yNzg0MworCisg ICAgICAgICogYmluZGluZ3Mvc2NyaXB0cy9Db2RlR2VuZXJhdG9yVjgucG06CisgICAgICAgICog YmluZGluZ3MvdjgvVjhTVkdQT0RUeXBlV3JhcHBlci5oOgorICAgICAgICAoV2ViQ29yZTo6UE9E VHlwZVdyYXBwZXJDYWNoZUluZm86OlBPRFR5cGVXcmFwcGVyQ2FjaGVJbmZvKToKKyAgICAgICAg KFdlYkNvcmU6OlBPRFR5cGVXcmFwcGVyQ2FjaGVJbmZvOjpvcGVyYXRvcj09KToKKyAgICAgICAg KFdlYkNvcmU6OlBPRFR5cGVXcmFwcGVyQ2FjaGVJbmZvSGFzaDo6aGFzaCk6CisgICAgICAgIChX ZWJDb3JlOjpWOFNWR0R5bmFtaWNQT0RUeXBlV3JhcHBlckNhY2hlOjpsb29rdXBPckNyZWF0ZVdy YXBwZXIpOgorCiAyMDA5LTA3LTMwICBZb25nIExpICA8eW9uZy5saUB0b3JjaG1vYmlsZS5jb20+ CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgR2VvcmdlIFN0YWlrb3MuCmRpZmYgLS1naXQgYS9XZWJD b3JlL2JpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvclY4LnBtIGIvV2ViQ29yZS9iaW5kaW5n cy9zY3JpcHRzL0NvZGVHZW5lcmF0b3JWOC5wbQppbmRleCA4NjI2NDlmLi44NzIyZTQzIDEwMDY0 NAotLS0gYS9XZWJDb3JlL2JpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvclY4LnBtCisrKyBi L1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0cy9Db2RlR2VuZXJhdG9yVjgucG0KQEAgLTI3LDYgKzI3 LDcgQEAKIHBhY2thZ2UgQ29kZUdlbmVyYXRvclY4OwogCiB1c2UgRmlsZTo6c3RhdDsKK3VzZSBE aWdlc3Q6Ok1ENTsKIAogbXkgJG1vZHVsZSA9ICIiOwogbXkgJG91dHB1dERpciA9ICIiOwpAQCAt NTkzLDcgKzU5NCwxMCBAQCBFTkQKICAgICAgICAgICAgIH0KICAgICAgICAgfSBlbHNlIHsKICAg ICAgICAgICAgIGlmICgkaW1wbENsYXNzSXNBbmltYXRlZFR5cGUpIHsKLSAgICAgICAgICAgICAg ICBteSAkd3JhcHBlciA9ICJWOFNWR0R5bmFtaWNQT0RUeXBlV3JhcHBlckNhY2hlPCRuYXRpdmVU eXBlLCAkaW1wbENsYXNzTmFtZT46Omxvb2t1cE9yQ3JlYXRlV3JhcHBlcihpbXAsICYke2ltcGxD bGFzc05hbWV9OjokZ2V0dGVyLCAmJHtpbXBsQ2xhc3NOYW1lfTo6JHNldHRlcikiOworICAgICAg ICAgICAgICAgICMgV2UgY2FuJ3QgaGFzaCBtZW1iZXIgZnVuY3Rpb24gcG9pbnRlcnMsIHNvIGlu c3RlYWQgZ2VuZXJhdGUKKyAgICAgICAgICAgICAgICAjIHNvbWUgaGFzaGluZyBtYXRlcmlhbCBi YXNlZCBvbiB0aGUgbmFtZXMgb2YgdGhlIG1ldGhvZHMuCisgICAgICAgICAgICAgICAgbXkgJGhh c2hoZXggPSBzdWJzdHIoRGlnZXN0OjpNRDU6Om1kNV9oZXgoIiR7aW1wbENsYXNzTmFtZX06OiRn ZXR0ZXIgJHtpbXBsQ2xhc3NOYW1lfTo6JHNldHRlcikiKSwgMCwgOCk7CisgICAgICAgICAgICAg ICAgbXkgJHdyYXBwZXIgPSAiVjhTVkdEeW5hbWljUE9EVHlwZVdyYXBwZXJDYWNoZTwkbmF0aXZl VHlwZSwgJGltcGxDbGFzc05hbWU+Ojpsb29rdXBPckNyZWF0ZVdyYXBwZXIoaW1wLCAmJHtpbXBs Q2xhc3NOYW1lfTo6JGdldHRlciwgJiR7aW1wbENsYXNzTmFtZX06OiRzZXR0ZXIsIDB4JGhhc2ho ZXgpIjsKICAgICAgICAgICAgICAgICBwdXNoKEBpbXBsQ29udGVudERlY2xzLCAiICAgIFJlZlB0 cjxWOFNWR1BPRFR5cGVXcmFwcGVyPCIgLiAkbmF0aXZlVHlwZSAuICI+ID4gd3JhcHBlciA9ICR3 cmFwcGVyO1xuIik7CiAgICAgICAgICAgICB9IGVsc2UgewogICAgICAgICAgICAgICAgIG15ICR3 cmFwcGVyID0gR2VuZXJhdGVTVkdTdGF0aWNQb2RUeXBlV3JhcHBlcigkcmV0dXJuVHlwZSwgJGdl dHRlclN0cmluZyk7CmRpZmYgLS1naXQgYS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4U1ZHUE9EVHlw ZVdyYXBwZXIuaCBiL1dlYkNvcmUvYmluZGluZ3MvdjgvVjhTVkdQT0RUeXBlV3JhcHBlci5oCmlu ZGV4IDgzZWM2ZDcuLjg1ZjEzZmEgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvYmluZGluZ3MvdjgvVjhT VkdQT0RUeXBlV3JhcHBlci5oCisrKyBiL1dlYkNvcmUvYmluZGluZ3MvdjgvVjhTVkdQT0RUeXBl V3JhcHBlci5oCkBAIC0zMCwxMSArMzAsMTQgQEAKIAogI2lmIEVOQUJMRShTVkcpCiAKKyNpbmNs dWRlIDx1dGlsaXR5PgorCiAjaW5jbHVkZSAiU1ZHRWxlbWVudC5oIgogI2luY2x1ZGUgIlNWR0xp c3QuaCIKICNpbmNsdWRlICJWOFByb3h5LmgiCiAKICNpbmNsdWRlIDx3dGYvQXNzZXJ0aW9ucy5o PgorI2luY2x1ZGUgPHd0Zi9IYXNoRnVuY3Rpb25zLmg+CiAjaW5jbHVkZSA8d3RmL0hhc2hNYXAu aD4KICNpbmNsdWRlIDx3dGYvUmVmQ291bnRlZC5oPgogI2luY2x1ZGUgPHd0Zi9TdGRMaWJFeHRy YXMuaD4KQEAgLTI1MSw2ICsyNTQsNyBAQCBzdHJ1Y3QgUE9EVHlwZVdyYXBwZXJDYWNoZUluZm8g ewogICAgICAgICA6IGNyZWF0b3IoMCkKICAgICAgICAgLCBnZXR0ZXIoMCkKICAgICAgICAgLCBz ZXR0ZXIoMCkKKyAgICAgICAgLCBmaWVsZEhhc2goMCkKICAgICB7IH0KIAogICAgIC8vIERlbGV0 ZWQgdmFsdWUKQEAgLTI1OCw2ICsyNjIsNyBAQCBzdHJ1Y3QgUE9EVHlwZVdyYXBwZXJDYWNoZUlu Zm8gewogICAgICAgICA6IGNyZWF0b3IocmVpbnRlcnByZXRfY2FzdDxQT0RUeXBlQ3JlYXRvcio+ KC0xKSkKICAgICAgICAgLCBnZXR0ZXIoMCkKICAgICAgICAgLCBzZXR0ZXIoMCkKKyAgICAgICAg LCBmaWVsZEhhc2goMCkKICAgICB7CiAgICAgfQogCkBAIC0yNjYsMTAgKzI3MSwxMSBAQCBzdHJ1 Y3QgUE9EVHlwZVdyYXBwZXJDYWNoZUluZm8gewogICAgICAgICByZXR1cm4gY3JlYXRvciA9PSBy ZWludGVycHJldF9jYXN0PFBPRFR5cGVDcmVhdG9yKj4oLTEpOwogICAgIH0KIAotICAgIFBPRFR5 cGVXcmFwcGVyQ2FjaGVJbmZvKFBPRFR5cGVDcmVhdG9yKiBfY3JlYXRvciwgR2V0dGVyTWV0aG9k IF9nZXR0ZXIsIFNldHRlck1ldGhvZCBfc2V0dGVyKQorICAgIFBPRFR5cGVXcmFwcGVyQ2FjaGVJ bmZvKFBPRFR5cGVDcmVhdG9yKiBfY3JlYXRvciwgR2V0dGVyTWV0aG9kIF9nZXR0ZXIsIFNldHRl ck1ldGhvZCBfc2V0dGVyLCB1bnNpZ25lZCBfZmllbGRIYXNoKQogICAgICAgICA6IGNyZWF0b3Io X2NyZWF0b3IpCiAgICAgICAgICwgZ2V0dGVyKF9nZXR0ZXIpCiAgICAgICAgICwgc2V0dGVyKF9z ZXR0ZXIpCisgICAgICAgICwgZmllbGRIYXNoKF9maWVsZEhhc2gpCiAgICAgewogICAgICAgICBB U1NFUlQoY3JlYXRvcik7CiAgICAgICAgIEFTU0VSVChnZXR0ZXIpOwpAQCAtMjc3LDIyICsyODMs MjMgQEAgc3RydWN0IFBPRFR5cGVXcmFwcGVyQ2FjaGVJbmZvIHsKIAogICAgIGJvb2wgb3BlcmF0 b3I9PShjb25zdCBQT0RUeXBlV3JhcHBlckNhY2hlSW5mbyYgb3RoZXIpIGNvbnN0CiAgICAgewot ICAgICAgICByZXR1cm4gY3JlYXRvciA9PSBvdGhlci5jcmVhdG9yICYmIGdldHRlciA9PSBvdGhl ci5nZXR0ZXIgJiYgc2V0dGVyID09IG90aGVyLnNldHRlcjsKKyAgICAgICAgcmV0dXJuIGNyZWF0 b3IgPT0gb3RoZXIuY3JlYXRvciAmJiBmaWVsZEhhc2ggPT0gb3RoZXIuZmllbGRIYXNoICYmIGdl dHRlciA9PSBvdGhlci5nZXR0ZXIgJiYgc2V0dGVyID09IG90aGVyLnNldHRlcjsKICAgICB9CiAK ICAgICBQT0RUeXBlQ3JlYXRvciogY3JlYXRvcjsKICAgICBHZXR0ZXJNZXRob2QgZ2V0dGVyOwog ICAgIFNldHRlck1ldGhvZCBzZXR0ZXI7CisgICAgdW5zaWduZWQgZmllbGRIYXNoOwogfTsKIAog dGVtcGxhdGU8dHlwZW5hbWUgUE9EVHlwZSwgdHlwZW5hbWUgUE9EVHlwZUNyZWF0b3I+CiBzdHJ1 Y3QgUE9EVHlwZVdyYXBwZXJDYWNoZUluZm9IYXNoIHsKICAgICBzdGF0aWMgdW5zaWduZWQgaGFz aChjb25zdCBQT0RUeXBlV3JhcHBlckNhY2hlSW5mbzxQT0RUeXBlLCBQT0RUeXBlQ3JlYXRvcj4m IGluZm8pCiAgICAgewotICAgICAgICB1bnNpZ25lZCBjcmVhdG9yID0gcmVpbnRlcnByZXRfY2Fz dDx1bnNpZ25lZD4oaW5mby5jcmVhdG9yKTsKLSAgICAgICAgdW5zaWduZWQgZ2V0dGVyID0gcmVp bnRlcnByZXRfY2FzdDx1bnNpZ25lZD4oKih2b2lkKiopJmluZm8uZ2V0dGVyKTsKLSAgICAgICAg dW5zaWduZWQgc2V0dGVyID0gcmVpbnRlcnByZXRfY2FzdDx1bnNpZ25lZD4oKih2b2lkKiopJmlu Zm8uc2V0dGVyKTsKLSAgICAgICAgcmV0dXJuIChjcmVhdG9yICogMTMpICsgZ2V0dGVyIF4gKHNl dHRlciA+PiAyKTsKKyAgICAgICAgLy8gV2UgY2FuJ3QgaGFzaCBtZW1iZXIgZnVuY3Rpb24gcG9p bnRlcnMsIGJ1dCB3ZSBoYXZlIGVub3VnaCBtYXRlcmlhbAorICAgICAgICAvLyB0byBoYXNoIHRo ZSBwb2ludGVyIGFuZCBmaWVsZCBpZGVudGlmaWVyLCBhbmQgb24gYSBjb2xsaXNpb24KKyAgICAg ICAgLy8gb3BlcmF0b3I9PSB3aWxsIHN0aWxsIGRpZmZlcmVudGlhdGUgdGhlIG1lbWJlciBmdW5j dGlvbiBwb2ludGVycy4KKyAgICAgICAgcmV0dXJuIFdURjo6UGFpckhhc2g8dm9pZCosIHVuc2ln bmVkPjo6aGFzaChzdGQ6OnBhaXI8dm9pZCosIHVuc2lnbmVkPihpbmZvLmNyZWF0b3IsIGluZm8u ZmllbGRIYXNoKSk7CiAgICAgfQogCiAgICAgc3RhdGljIGJvb2wgZXF1YWwoY29uc3QgUE9EVHlw ZVdyYXBwZXJDYWNoZUluZm88UE9EVHlwZSwgUE9EVHlwZUNyZWF0b3I+JiBhLCBjb25zdCBQT0RU eXBlV3JhcHBlckNhY2hlSW5mbzxQT0RUeXBlLCBQT0RUeXBlQ3JlYXRvcj4mIGIpCkBAIC0zNTAs MTAgKzM1NywxMCBAQCBwdWJsaWM6CiAgICAgfQogCiAgICAgLy8gVXNlZCBmb3IgcmVhZHdyaXRl IGF0dHJpYnV0ZXMgb25seQotICAgIHN0YXRpYyBQYXNzUmVmUHRyPFdyYXBwZXJCYXNlPiBsb29r dXBPckNyZWF0ZVdyYXBwZXIoUE9EVHlwZUNyZWF0b3IqIGNyZWF0b3IsIEdldHRlck1ldGhvZCBn ZXR0ZXIsIFNldHRlck1ldGhvZCBzZXR0ZXIpCisgICAgc3RhdGljIFBhc3NSZWZQdHI8V3JhcHBl ckJhc2U+IGxvb2t1cE9yQ3JlYXRlV3JhcHBlcihQT0RUeXBlQ3JlYXRvciogY3JlYXRvciwgR2V0 dGVyTWV0aG9kIGdldHRlciwgU2V0dGVyTWV0aG9kIHNldHRlciwgdW5zaWduZWQgZmllbGRIYXNo KQogICAgIHsKICAgICAgICAgRHluYW1pY1dyYXBwZXJIYXNoTWFwJiBtYXAoZHluYW1pY1dyYXBw ZXJIYXNoTWFwKCkpOwotICAgICAgICBDYWNoZUluZm8gaW5mbyhjcmVhdG9yLCBnZXR0ZXIsIHNl dHRlcik7CisgICAgICAgIENhY2hlSW5mbyBpbmZvKGNyZWF0b3IsIGdldHRlciwgc2V0dGVyLCBm aWVsZEhhc2gpOwogCiAgICAgICAgIGlmIChtYXAuY29udGFpbnMoaW5mbykpCiAgICAgICAgICAg ICByZXR1cm4gbWFwLmdldChpbmZvKTsK