Obj-C API: JSExport doesn't work for methods that contain protocols in their type signature

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

The bug is in parseObjCType in ObjcRuntimeExtras.h. When we see an '@' in the type signature of a method, we assume that what follows the '@' is a class name, so we call objc_getClass, and if that returns nil then we give up on the method and don't export it.

This assumption doesn't work in the case of id<Protocol> because it's the name of the protocol that follows the '@', not the name of a class. We should have another fallback case for protocol names.

There's another case that also doesn't work, and that's the case of a named class with a specified prototype in a method signature (e.g. NSObject<MyProtocol>). There the substring of the type signature that represents the class is "NSObject<MyProtocol>", which will also cause objc_getClass to return nil.

[Mark Hahnenberg]

<rdar://problem/15954242>

[Mark Hahnenberg]

Created attachment 223728 [details]
Patch

[Filip Pizlo]

Comment on attachment 223728 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223728&action=review

> Source/JavaScriptCore/API/ObjcRuntimeExtras.h:198
> +                position = index(++position, '"') + 1;

This feels at first like something that would give you victory at the IOCCC.  I eventually figured it.  But maybe you an come up with a better idiom for this at some point?

[Mark Hahnenberg]

(In reply to comment #3)
> (From update of attachment 223728 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223728&action=review
> 
> > Source/JavaScriptCore/API/ObjcRuntimeExtras.h:198
> > +                position = index(++position, '"') + 1;
> 
> This feels at first like something that would give you victory at the IOCCC.  I eventually figured it.  But maybe you an come up with a better idiom for this at some point?

I guess at this point, begin == position and we don't need to increment anything because we already know we're not pointing at a '"', so i can just change it to:

position = index(begin, '"') + 1;

Copy, paste, modify...not even once.

[Geoffrey Garen]

> This feels at first like something that would give you victory at the IOCCC.

Probably only second place, if Gavin also entered.

[Mark Hahnenberg]

Created attachment 223729 [details]
Patch

[Mark Hahnenberg]

(In reply to comment #6)
> Created an attachment (id=223729) [details]
> Patch

I decided to take Phil's advice to heart and actually write this code in a more understandable fashion.

[WebKit Commit Bot]

Comment on attachment 223729 [details]
Patch

Clearing flags on attachment: 223729

Committed r163876: <http://trac.webkit.org/changeset/163876>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 223729 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223729&action=review

> Source/JavaScriptCore/API/ObjcRuntimeExtras.h:197
> +            const char* protocolPosition = index(begin, '<');
> +            const char* endOfType = index(begin, '"');

It’s a little strange that we use index() here instead of strchr(). I think that index() is an archaic, pre-standardization, name for the same thing.

[Mark Hahnenberg]

(In reply to comment #10)
> (From update of attachment 223729 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223729&action=review
> 
> > Source/JavaScriptCore/API/ObjcRuntimeExtras.h:197
> > +            const char* protocolPosition = index(begin, '<');
> > +            const char* endOfType = index(begin, '"');
> 
> It’s a little strange that we use index() here instead of strchr(). I think that index() is an archaic, pre-standardization, name for the same thing.

index() does look like it's been deprecated in favor of strchr(). I filed bug 128610 to get rid index() in ObjcRuntimeExtras.h