Work around a bug in Flash where NSException objects can be released too early
Created attachment 187200 [details] Patch
Comment on attachment 187200 [details] Patch Attachment 187200 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16445433
Comment on attachment 187200 [details] Patch Attachment 187200 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16438410
Comment on attachment 187200 [details] 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.
(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.
> 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.
Committed r142314: <http://trac.webkit.org/changeset/142314>