Bug 122561 – Objective-C API: blocks aren't callable via 'new'

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

Currently the only way for clients to vend new native objects to JavaScript code is via factory methods in the form of exported class methods or blocks. Blocks can be called like normal functions from JavaScript code, but they cannot be invoked with 'new'. This would give a simple way for clients to expose constructor-like behavior to their JavaScript code.

[Mark Hahnenberg]

Created attachment 213795 [details]
Patch

[Mark Hahnenberg]

<rdar://problem/14130717>

[Geoffrey Garen]

Comment on attachment 213795 [details]
Patch

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

r=me

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:496
> +    JSGlobalContextRef contextRef = [context JSGlobalContextRef];
> +    if (*exception) {
> +        ::JSValue *bogusObject = [::JSValue valueWithNewObjectInContext:context];
> +        return JSValueToObject(contextRef, [bogusObject JSValueRef], NULL);
> +    }
> +
> +    if (!JSValueIsObject(contextRef, result)) {
> +        *exception = toRef(JSC::createTypeError(toJS(contextRef), "Objective-C blocks called as constructors must return an object."));
> +        ::JSValue *bogusObject = [::JSValue valueWithNewObjectInContext:context];
> +        return JSValueToObject(contextRef, [bogusObject JSValueRef], NULL);
> +    }

Can we get away with just returning NULL here? In general, the C API allows a client to return NULL when throwing an exception. I think that would be nicer than making a bogus object.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:497
> +    return JSValueToObject(contextRef, result, exception);

This can just be a cast, since you've checked JSValueIsObject.

[Mark Hahnenberg]

(In reply to comment #3)
> (From update of attachment 213795 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213795&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/API/ObjCCallbackFunction.mm:496
> > +    JSGlobalContextRef contextRef = [context JSGlobalContextRef];
> > +    if (*exception) {
> > +        ::JSValue *bogusObject = [::JSValue valueWithNewObjectInContext:context];
> > +        return JSValueToObject(contextRef, [bogusObject JSValueRef], NULL);
> > +    }
> > +
> > +    if (!JSValueIsObject(contextRef, result)) {
> > +        *exception = toRef(JSC::createTypeError(toJS(contextRef), "Objective-C blocks called as constructors must return an object."));
> > +        ::JSValue *bogusObject = [::JSValue valueWithNewObjectInContext:context];
> > +        return JSValueToObject(contextRef, [bogusObject JSValueRef], NULL);
> > +    }
> 
> Can we get away with just returning NULL here? In general, the C API allows a client to return NULL when throwing an exception. I think that would be nicer than making a bogus object.

I ran into an issue where if I returned NULL, the caller (APICallbackFunction::construct) would see the null and throw a *different* TypeError than the one I wanted to throw. Maybe we could just change APICallbackFunction::construct to allow NULL?

> 
> > Source/JavaScriptCore/API/ObjCCallbackFunction.mm:497
> > +    return JSValueToObject(contextRef, result, exception);
> 
> This can just be a cast, since you've checked JSValueIsObject.

Okiedokie.

[Mark Hahnenberg]

Created attachment 213826 [details]
Patch

[Mark Hahnenberg]

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

I uploaded another patch for review because I changed the part where we create the block automatically add a new, empty object as the .prototype property of the block and to set the .constructor property on that prototype object properly rather than forcing the client to do it themselves.

[Mark Hahnenberg]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=213826) [details] [details]
> > Patch
> 
> I uploaded another patch for review because I changed the part where we create the block automatically add a new, empty object as the .prototype property of the block and to set the .constructor property on that prototype object properly rather than forcing the client to do all of that themselves.

...I changed the part where we create the block automatically TO add a new, empty...

[Geoffrey Garen]

> > Can we get away with just returning NULL here? In general, the C API allows a client to return NULL when throwing an exception. I think that would be nicer than making a bogus object.
> 
> I ran into an issue where if I returned NULL, the caller (APICallbackFunction::construct) would see the null and throw a *different* TypeError than the one I wanted to throw. Maybe we could just change APICallbackFunction::construct to allow NULL?

I think it's appropriate for APICallbackFunction::construct to allow NULL if and only if an exception has been thrown. So, you just have to change the "if (exception)" block to return explicitly, to avoid throwing a new exception.

[Geoffrey Garen]

> I think it's appropriate for APICallbackFunction::construct to allow NULL if and only if an exception has been thrown. So, you just have to change the "if (exception)" block to return explicitly, to avoid throwing a new exception.

... which is a good thing to do anyway, since returning an exception is supposed to mean "don't use the value I returned -- it's bogus". We want to return some value other than result, since result is bogus. Looks like we return the exception object when we throw a type error, so we should probably return the exception object in this case as well.

[Mark Hahnenberg]

Created attachment 213828 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 213828 [details]
Patch

Clearing flags on attachment: 213828

Committed r157234: <http://trac.webkit.org/changeset/157234>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.