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-

Description Anders Carlsson 2013-02-07 16:43:00 PST
Work around a bug in Flash where NSException objects can be released too early
Comment 1 Anders Carlsson 2013-02-07 16:45:48 PST
Created attachment 187200 [details]
Patch
Comment 2 Early Warning System Bot 2013-02-07 17:04:28 PST
Comment on attachment 187200 [details]
Patch

Attachment 187200 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16445433
Comment 3 EFL EWS Bot 2013-02-07 17:32:27 PST
Comment on attachment 187200 [details]
Patch

Attachment 187200 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16438410
Comment 4 Darin Adler 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.
Comment 5 Anders Carlsson 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.
Comment 6 Anders Carlsson 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.
Comment 7 Anders Carlsson 2013-02-08 12:48:21 PST
Committed r142314: <http://trac.webkit.org/changeset/142314>