Bug 109242

Summary: Work around a bug in Flash where NSException objects can be released too early
Product: WebKit Reporter: Anders Carlsson <andersca>
Component: New BugsAssignee: Anders Carlsson <andersca>
Status: RESOLVED FIXED    
Severity: Normal CC: webkit-ews
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch darin: review+, webkit-ews: commit-queue-

Anders Carlsson
Reported 2013-02-07 16:43:00 PST
Work around a bug in Flash where NSException objects can be released too early
Attachments
Patch (5.80 KB, patch)
2013-02-07 16:45 PST, Anders Carlsson
darin: review+
webkit-ews: commit-queue-
Anders Carlsson
Comment 1 2013-02-07 16:45:48 PST
Early Warning System Bot
Comment 2 2013-02-07 17:04:28 PST
EFL EWS Bot
Comment 3 2013-02-07 17:32:27 PST
Darin Adler
Comment 4 2013-02-07 20:12:44 PST
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.
Anders Carlsson
Comment 5 2013-02-08 10:39:36 PST
(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.
Anders Carlsson
Comment 6 2013-02-08 10:47:44 PST
> 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.
Anders Carlsson
Comment 7 2013-02-08 12:48:21 PST
Note You need to log in before you can comment on or make changes to this bug.