109242 2013-02-07 16:43:00 -0800 Work around a bug in Flash where NSException objects can be released too early 2013-02-08 12:48:21 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 andersca andersca webkit-ews oldest_to_newest 828011 0 andersca 2013-02-07 16:43:00 -0800 Work around a bug in Flash where NSException objects can be released too early 828014 1 187200 andersca 2013-02-07 16:45:48 -0800 Created attachment 187200 Patch 828028 2 187200 webkit-ews 2013-02-07 17:04:28 -0800 Comment on attachment 187200 Patch Attachment 187200 did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16445433 828052 3 187200 eflews.bot 2013-02-07 17:32:27 -0800 Comment on attachment 187200 Patch Attachment 187200 did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16438410 828127 4 187200 darin 2013-02-07 20:12:44 -0800 Comment on attachment 187200 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187200&action=review Is leaking them all permanently the best way to handle this? I suppose there is an infinitesimal number of these objects created during normal operation, so probably OK to leak them all. > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h:155 > + void platformPreInitialize(); It would be kind to provide empty implementations for all the other platforms, but you could also just let the people working on those platforms do it. I don’t know what the correct etiquette is for this these days in here. > Source/WebKit2/WebProcess/Plugins/Netscape/mac/NetscapePluginMac.mm:192 > +static void NSException_release(id self, SEL _cmd) Leave out the argument names? > Source/WebKit2/WebProcess/Plugins/Netscape/mac/NetscapePluginMac.mm:202 > + dispatch_once(&once, ^{ Is platformPreInitialize called more than once? > Source/WebKit2/WebProcess/Plugins/Netscape/mac/NetscapePluginMac.mm:210 > + Method exceptionReleaseMethod = class_getInstanceMethod([NSException class], @selector(release)); > + if (exceptionReleaseMethod == class_getInstanceMethod([NSObject class], @selector(release))) { > + if (!class_addMethod([NSException class], @selector(release), reinterpret_cast<IMP>(NSException_release), method_getTypeEncoding(exceptionReleaseMethod))) > + ASSERT_NOT_REACHED(); > + } else { > + // The method already exists on NSException; replace its implementation. > + method_setImplementation(exceptionReleaseMethod, reinterpret_cast<IMP>(NSException_release)); > + } It would be cleaner to use class_replaceMethod instead of class_getInstanceMethod/class_addMethod/method_setImplementation. I don’t think you need to call class_getInstanceMethod at all unless you’re doing it to avoid hard coding the type encoding string. 828672 5 andersca 2013-02-08 10:39:36 -0800 (In reply to comment #4) > (From update of attachment 187200 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=187200&action=review > > Is leaking them all permanently the best way to handle this? I suppose there is an infinitesimal number of these objects created during normal operation, so probably OK to leak them all. > I thought of releasing them after a delay (sort of a manual autorelease pool), but I couldn't think of a good way to do it that wouldn't potentially introduce race conditions. > > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h:155 > > + void platformPreInitialize(); > > It would be kind to provide empty implementations for all the other platforms, but you could also just let the people working on those platforms do it. I don’t know what the correct etiquette is for this these days in here. I don't know which implementations are built nowadays so I usually let the port maintainers fix it. > > > Source/WebKit2/WebProcess/Plugins/Netscape/mac/NetscapePluginMac.mm:192 > > +static void NSException_release(id self, SEL _cmd) > > Leave out the argument names? Sure. > > > Source/WebKit2/WebProcess/Plugins/Netscape/mac/NetscapePluginMac.mm:202 > > + dispatch_once(&once, ^{ > > Is platformPreInitialize called more than once? > > > Source/WebKit2/WebProcess/Plugins/Netscape/mac/NetscapePluginMac.mm:210 > > + Method exceptionReleaseMethod = class_getInstanceMethod([NSException class], @selector(release)); > > + if (exceptionReleaseMethod == class_getInstanceMethod([NSObject class], @selector(release))) { > > + if (!class_addMethod([NSException class], @selector(release), reinterpret_cast<IMP>(NSException_release), method_getTypeEncoding(exceptionReleaseMethod))) > > + ASSERT_NOT_REACHED(); > > + } else { > > + // The method already exists on NSException; replace its implementation. > > + method_setImplementation(exceptionReleaseMethod, reinterpret_cast<IMP>(NSException_release)); > > + } > > It would be cleaner to use class_replaceMethod instead of class_getInstanceMethod/class_addMethod/method_setImplementation. I don’t think you need to call class_getInstanceMethod at all unless you’re doing it to avoid hard coding the type encoding string. Indeed. I thought there was a way to do this without having to check for the method. I don't like hard-coding the type encoding so I'll use class_getInstanceMethod for that. 828679 6 andersca 2013-02-08 10:47:44 -0800 > Is platformPreInitialize called more than once? Yes, it's called every time a plug-in instance is created. I couldn't find a better place to do this since we need access to the quirks. 828722 7 andersca 2013-02-08 12:48:21 -0800 Committed r142314: <http://trac.webkit.org/changeset/142314> 187200 2013-02-07 16:45:48 -0800 2013-02-07 20:12:44 -0800 Patch bug-109242-20130207164224.patch text/plain 5944 andersca U3VidmVyc2lvbiBSZXZpc2lvbjogMTQyMTYzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggZDljMTlhYzNlNThhMDgx MmNlZmIyN2Q2ZTYxZTMwMWE5MjRiMjg1Yi4uZWNlNzJjOGM3NTI3Y2E1NmQ2ZDI0MWM4ZTJhMmFj MGZjOTgxYzYxOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDMzIEBACisyMDEzLTAyLTA3ICBBbmRl cnMgQ2FybHNzb24gIDxhbmRlcnNjYUBhcHBsZS5jb20+CisKKyAgICAgICAgV29yayBhcm91bmQg YSBidWcgaW4gRmxhc2ggd2hlcmUgTlNFeGNlcHRpb24gb2JqZWN0cyBjYW4gYmUgcmVsZWFzZWQg dG9vIGVhcmx5CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xMDkyNDIKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzEzMDAzNDcwPgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogU2hhcmVkL1BsdWdpbnMvTmV0 c2NhcGUvbWFjL05ldHNjYXBlUGx1Z2luTW9kdWxlTWFjLm1tOgorICAgICAgICAoV2ViS2l0OjpO ZXRzY2FwZVBsdWdpbk1vZHVsZTo6ZGV0ZXJtaW5lUXVpcmtzKToKKyAgICAgICAgU2V0IHRoZSBu ZXcgcGx1Zy1pbiBxdWlyay4KKworICAgICAgICAqIFNoYXJlZC9QbHVnaW5zL1BsdWdpblF1aXJr cy5oOgorICAgICAgICBBZGQgYSBuZXcgcGx1Zy1pbiBxdWlyay4KKworICAgICAgICAqIFdlYlBy b2Nlc3MvUGx1Z2lucy9OZXRzY2FwZS9OZXRzY2FwZVBsdWdpbi5jcHA6CisgICAgICAgIChXZWJL aXQ6Ok5ldHNjYXBlUGx1Z2luOjppbml0aWFsaXplKToKKyAgICAgICAgQ2FsbCBwbGF0Zm9ybVBy ZUluaXRpYWxpemUuCisKKyAgICAgICAgKiBXZWJQcm9jZXNzL1BsdWdpbnMvTmV0c2NhcGUvTmV0 c2NhcGVQbHVnaW4uaDoKKyAgICAgICAgKE5ldHNjYXBlUGx1Z2luKToKKyAgICAgICAgQWRkIHBs YXRmb3JtUHJlSW5pdGlhbGl6ZS4KKworICAgICAgICAqIFdlYlByb2Nlc3MvUGx1Z2lucy9OZXRz Y2FwZS9tYWMvTmV0c2NhcGVQbHVnaW5NYWMubW06CisgICAgICAgIChXZWJLaXQ6Ok5TRXhjZXB0 aW9uX3JlbGVhc2UpOgorICAgICAgICBBZGQgbmV3IGVtcHR5IGZ1bmN0aW9uLgorCisgICAgICAg IChXZWJLaXQ6Ok5ldHNjYXBlUGx1Z2luOjpwbGF0Zm9ybVByZUluaXRpYWxpemUpOgorICAgICAg ICBQYXRjaCAtW05TRXhjZXB0aW9uIHJlbGVhc2VdIHRvIGJlIGEgbm8tb3AuCisKIDIwMTMtMDIt MDYgIFNhbSBXZWluaWcgIDxzYW1Ad2Via2l0Lm9yZz4KIAogICAgICAgICBNYWtlIFdlYlBhZ2VQ cm94eSBhbmQgc3ViLW9iamVjdHMgTWVzc2FnZVJlY2VpdmVycwpkaWZmIC0tZ2l0IGEvU291cmNl L1dlYktpdDIvU2hhcmVkL1BsdWdpbnMvTmV0c2NhcGUvbWFjL05ldHNjYXBlUGx1Z2luTW9kdWxl TWFjLm1tIGIvU291cmNlL1dlYktpdDIvU2hhcmVkL1BsdWdpbnMvTmV0c2NhcGUvbWFjL05ldHNj YXBlUGx1Z2luTW9kdWxlTWFjLm1tCmluZGV4IDE3NDVmZTJkN2EwYTQ4YzhhYTBkNzVkMDE1N2Jh MmNjZmVjMDJlYWYuLmYyZTZkNDMzYjM5MWJkOTg3NWM4M2ZlMTJlMzdhZTAzMjRjMDNjMjMgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1NoYXJlZC9QbHVnaW5zL05ldHNjYXBlL21hYy9OZXRz Y2FwZVBsdWdpbk1vZHVsZU1hYy5tbQorKysgYi9Tb3VyY2UvV2ViS2l0Mi9TaGFyZWQvUGx1Z2lu cy9OZXRzY2FwZS9tYWMvTmV0c2NhcGVQbHVnaW5Nb2R1bGVNYWMubW0KQEAgLTQ4MSw2ICs0ODEs OSBAQCB2b2lkIE5ldHNjYXBlUGx1Z2luTW9kdWxlOjpkZXRlcm1pbmVRdWlya3MoKQogCiAgICAg ICAgIC8vIEZsYXNoIHJldHVybnMgYSByZXRhaW5lZCBDb3JlIEFuaW1hdGlvbiBsYXllci4KICAg ICAgICAgbV9wbHVnaW5RdWlya3MuYWRkKFBsdWdpblF1aXJrczo6UmV0dXJuc1JldGFpbmVkQ29y ZUFuaW1hdGlvbkxheWVyKTsKKworICAgICAgICAvLyBGbGFzaCBoYXMgYSBidWcgd2hlcmUgTlNF eGNlcHRpb25zIGNhbiBiZSByZWxlYXNlZCB0b28gZWFybHkuCisgICAgICAgIG1fcGx1Z2luUXVp cmtzLmFkZChQbHVnaW5RdWlya3M6OkxlYWtBbGxUaHJvd25OU0V4Y2VwdGlvbnMpOwogICAgIH0K IAogICAgIGlmIChwbHVnaW4uYnVuZGxlSWRlbnRpZmllciA9PSAiY29tLm1pY3Jvc29mdC5TaWx2 ZXJsaWdodFBsdWdpbiIpIHsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL1NoYXJlZC9QbHVn aW5zL1BsdWdpblF1aXJrcy5oIGIvU291cmNlL1dlYktpdDIvU2hhcmVkL1BsdWdpbnMvUGx1Z2lu UXVpcmtzLmgKaW5kZXggZTk5ZjcxYWE5NGM3MjA4YzEyMzllYjk0M2FhOWExM2Y3YzA1OTYxYi4u MzIwNWMxNWY4NDg2YmE0M2I0NDhmMGFmNDkzMzc5NTU2MGNlMzAxOCAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYktpdDIvU2hhcmVkL1BsdWdpbnMvUGx1Z2luUXVpcmtzLmgKKysrIGIvU291cmNlL1dl YktpdDIvU2hhcmVkL1BsdWdpbnMvUGx1Z2luUXVpcmtzLmgKQEAgLTY4LDYgKzY4LDEwIEBAIHB1 YmxpYzoKICAgICAgICAgLy8gd2hpY2ggaXMgZW5hYmxlZCBpZiBpdCBkb2Vzbid0IGZpbmQgVmVy c2lvbi8zIGluIHRoZSB1c2VyLWFnZW50LgogICAgICAgICBBcHBlbmRWZXJzaW9uM1VzZXJBZ2Vu dCwKIAorICAgICAgICAvLyBXaGV0aGVyIGFsbCB0aHJvd24gTlNFeGNlcHRpb25zIHNob3VsZCBi ZSBsZWFrZWQuCisgICAgICAgIC8vIDxyZGFyOi8vcHJvYmxlbS8xMzAwMzQ3MD4gQWRvYmUgRmxh c2ggaGFzIGEgYnVnIHdoZXJlIGV4Y2VwdGlvbnMgYXJlIHJlbGVhc2VkIHRvbyBlYXJseS4KKyAg ICAgICAgTGVha0FsbFRocm93bk5TRXhjZXB0aW9ucywKKwogI2lmbmRlZiBOUF9OT19RVUlDS0RS QVcKICAgICAgICAgLy8gQWxsb3cgdGhlIHBsdWctaW4gdG8gdXNlIHRoZSBRdWlja0RyYXcgZHJh d2luZyBtb2RlbCwgc2luY2Ugd2Uga25vdyB0aGF0IHRoZSBwbHVnLWluCiAgICAgICAgIC8vIHdp bGwgbmV2ZXIgcGFpbnQgb3IgcmVjZWl2ZSBldmVudHMuIFVzZWQgYnkgdGhlIEFwcGxlQ29ubmVj dCBwbHVnLWluLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9QbHVnaW5z L05ldHNjYXBlL05ldHNjYXBlUGx1Z2luLmNwcCBiL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3Mv UGx1Z2lucy9OZXRzY2FwZS9OZXRzY2FwZVBsdWdpbi5jcHAKaW5kZXggM2JlMzU4ZTM3NWY5YzM0 MmYzNzYwNWQ3NzFhMmM3M2NiOTU1N2M5Yi4uMzFjYmUxYWUxN2YwZWIwY2I0NDk3ZTk4ZGQ2YzY3 N2I0NGZkZjRkMiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9QbHVnaW5z L05ldHNjYXBlL05ldHNjYXBlUGx1Z2luLmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9j ZXNzL1BsdWdpbnMvTmV0c2NhcGUvTmV0c2NhcGVQbHVnaW4uY3BwCkBAIC02MzAsNiArNjMwLDgg QEAgYm9vbCBOZXRzY2FwZVBsdWdpbjo6aW5pdGlhbGl6ZShjb25zdCBQYXJhbWV0ZXJzJiBwYXJh bWV0ZXJzKQogICAgIG1fbGF5ZXJIb3N0aW5nTW9kZSA9IHBhcmFtZXRlcnMubGF5ZXJIb3N0aW5n TW9kZTsKICNlbmRpZgogCisgICAgcGxhdGZvcm1QcmVJbml0aWFsaXplKCk7CisKICAgICBOZXRz Y2FwZVBsdWdpbiogcHJldmlvdXNOUFBOZXdQbHVnaW4gPSBjdXJyZW50TlBQTmV3UGx1Z2luOwog ICAgIAogICAgIG1faW5OUFBOZXcgPSB0cnVlOwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIv V2ViUHJvY2Vzcy9QbHVnaW5zL05ldHNjYXBlL05ldHNjYXBlUGx1Z2luLmggYi9Tb3VyY2UvV2Vi S2l0Mi9XZWJQcm9jZXNzL1BsdWdpbnMvTmV0c2NhcGUvTmV0c2NhcGVQbHVnaW4uaAppbmRleCBk ODM3M2I0ZDNhMWY1ZjM0ZGFkNDllNjZhMGJkZWNiNWQ5NWJlYzkxLi5mNTRmOTFiZWZhZmU0ZTQ1 MTIwNjRhNWNiZGJkYjI1YjNkZDBiMWJjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9XZWJQ cm9jZXNzL1BsdWdpbnMvTmV0c2NhcGUvTmV0c2NhcGVQbHVnaW4uaAorKysgYi9Tb3VyY2UvV2Vi S2l0Mi9XZWJQcm9jZXNzL1BsdWdpbnMvTmV0c2NhcGUvTmV0c2NhcGVQbHVnaW4uaApAQCAtMTUy LDYgKzE1Miw3IEBAIHByaXZhdGU6CiAKICAgICBjb25zdCBjaGFyKiB1c2VyQWdlbnQoKTsKIAor ICAgIHZvaWQgcGxhdGZvcm1QcmVJbml0aWFsaXplKCk7CiAgICAgYm9vbCBwbGF0Zm9ybVBvc3RJ bml0aWFsaXplKCk7CiAgICAgdm9pZCBwbGF0Zm9ybURlc3Ryb3koKTsKICAgICBib29sIHBsYXRm b3JtSW52YWxpZGF0ZShjb25zdCBXZWJDb3JlOjpJbnRSZWN0Jik7CmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1BsdWdpbnMvTmV0c2NhcGUvbWFjL05ldHNjYXBlUGx1Z2lu TWFjLm1tIGIvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9QbHVnaW5zL05ldHNjYXBlL21hYy9O ZXRzY2FwZVBsdWdpbk1hYy5tbQppbmRleCA0ZWRjMzJmZjQyMTExM2M1MDRiOGI3NTgxZGZmOGI0 ODFkYzY4MDJiLi4wMjE4MDk5YmU1ZGY0MDdmYzMzZWJlZDhmOTVjNDgzYTI0MTcyYWZlIDEwMDY0 NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1BsdWdpbnMvTmV0c2NhcGUvbWFjL05l dHNjYXBlUGx1Z2luTWFjLm1tCisrKyBiL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvUGx1Z2lu cy9OZXRzY2FwZS9tYWMvTmV0c2NhcGVQbHVnaW5NYWMubW0KQEAgLTE4OSw2ICsxODksMjkgQEAg c3RhdGljIFdpbmRvd01hcCYgd2luZG93TWFwKCkKIH0KICNlbmRpZgogCitzdGF0aWMgdm9pZCBO U0V4Y2VwdGlvbl9yZWxlYXNlKGlkIHNlbGYsIFNFTCBfY21kKQoreworICAgIC8vIERvIG5vdGhp bmcuCit9CisKK3ZvaWQgTmV0c2NhcGVQbHVnaW46OnBsYXRmb3JtUHJlSW5pdGlhbGl6ZSgpCit7 CisgICAgaWYgKG1fcGx1Z2luTW9kdWxlLT5wbHVnaW5RdWlya3MoKS5jb250YWlucyhQbHVnaW5R dWlya3M6OkxlYWtBbGxUaHJvd25OU0V4Y2VwdGlvbnMpKSB7CisgICAgICAgIC8vIFBhdGNoIC1b TlNFeGNlcHRpb24gcmVsZWFzZV0gdG8gbm90IHJlbGVhc2UgdGhlIG9iamVjdC4KKyAgICAgICAg c3RhdGljIGRpc3BhdGNoX29uY2VfdCBvbmNlOworICAgICAgICBkaXNwYXRjaF9vbmNlKCZvbmNl LCBeeworICAgICAgICAgICAgTWV0aG9kIGV4Y2VwdGlvblJlbGVhc2VNZXRob2QgPSBjbGFzc19n ZXRJbnN0YW5jZU1ldGhvZChbTlNFeGNlcHRpb24gY2xhc3NdLCBAc2VsZWN0b3IocmVsZWFzZSkp OworICAgICAgICAgICAgaWYgKGV4Y2VwdGlvblJlbGVhc2VNZXRob2QgPT0gY2xhc3NfZ2V0SW5z dGFuY2VNZXRob2QoW05TT2JqZWN0IGNsYXNzXSwgQHNlbGVjdG9yKHJlbGVhc2UpKSkgeworICAg ICAgICAgICAgICAgIGlmICghY2xhc3NfYWRkTWV0aG9kKFtOU0V4Y2VwdGlvbiBjbGFzc10sIEBz ZWxlY3RvcihyZWxlYXNlKSwgcmVpbnRlcnByZXRfY2FzdDxJTVA+KE5TRXhjZXB0aW9uX3JlbGVh c2UpLCBtZXRob2RfZ2V0VHlwZUVuY29kaW5nKGV4Y2VwdGlvblJlbGVhc2VNZXRob2QpKSkKKyAg ICAgICAgICAgICAgICAgICAgQVNTRVJUX05PVF9SRUFDSEVEKCk7CisgICAgICAgICAgICB9IGVs c2UgeworICAgICAgICAgICAgICAgIC8vIFRoZSBtZXRob2QgYWxyZWFkeSBleGlzdHMgb24gTlNF eGNlcHRpb247IHJlcGxhY2UgaXRzIGltcGxlbWVudGF0aW9uLgorICAgICAgICAgICAgICAgIG1l dGhvZF9zZXRJbXBsZW1lbnRhdGlvbihleGNlcHRpb25SZWxlYXNlTWV0aG9kLCByZWludGVycHJl dF9jYXN0PElNUD4oTlNFeGNlcHRpb25fcmVsZWFzZSkpOworICAgICAgICAgICAgfQorICAgICAg ICB9KTsKKyAgICB9Cit9CisKIGJvb2wgTmV0c2NhcGVQbHVnaW46OnBsYXRmb3JtUG9zdEluaXRp YWxpemUoKQogewogICAgIGlmIChtX2RyYXdpbmdNb2RlbCA9PSBzdGF0aWNfY2FzdDxOUERyYXdp bmdNb2RlbD4oLTEpKSB7Cg==