Bug 109242 – Work around a bug in Flash where NSException objects can be released too early

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anders Carlsson]

Work around a bug in Flash where NSException objects can be released too early

[Anders Carlsson]

Created attachment 187200 [details]
Patch

[Early Warning System Bot]

Comment on attachment 187200 [details]
Patch

Attachment 187200 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16445433

[EFL EWS Bot]

Comment on attachment 187200 [details]
Patch

Attachment 187200 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16438410

[Darin Adler]

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]

(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]

> 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]

Committed r142314: <http://trac.webkit.org/changeset/142314>