[webkit-reviews] review granted: [Bug 109242] Work around a bug in Flash where NSException objects can be released too early : [Attachment 187200] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 109242: Work around a bug in Flash where NSException objects can be
released too early
https://bugs.webkit.org/show_bug.cgi?id=109242

Attachment 187200: Patch
https://bugs.webkit.org/attachment.cgi?id=187200&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.