Bug 223646 – [GTK][WPE] JSC crashes if a function expects a parameter but doesn't receive any

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alberto Garcia]

Created attachment 424039 [details]
Test case

The attached test case uses the JSC API to create a function named "foo()" that receives a parameter of type JSCValue:

    	jsc_value_new_function(ctx,
    		"foo",
                G_CALLBACK(foo_cb),
                NULL,            // user_data
    		NULL,            // destroy_notify
    		G_TYPE_NONE,     // return_type
    		1,               // n_params
                JSC_TYPE_VALUE)

The callback foo_cb() simply prints the value to the standard output, you can see it with ./test 'foo(4)' or ./test 'foo("Hello world")'

However if you call foo() without any parameters the process crashes before foo_cb() even gets called.

   GLib-GObject:ERROR:../../../gobject/gclosure.c:1169:value_to_ffi_type: assertion failed: (type != G_TYPE_INVALID)
   Aborted

This should produce an exception that the program can handle.

[Carlos Garcia Campos]

We need to fix the crash, but I don't think we should throw a exception, it should just receive undefined.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #1)
> We need to fix the crash, but I don't think we should throw a exception, it
> should just receive undefined.

I had a patch half done which would raise TypeError. I don't know if we
can reliably pass “undefined” to C functions... What would “undefined”
be e.g. for an “int32_t” or a “char*”?

[Carlos Garcia Campos]

We would pass undefined for the case of parameters expecting JSCValue, otherwise we will throw an exception.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #3)
> We would pass undefined for the case of parameters expecting JSCValue,
> otherwise we will throw an exception.

This sounds perfect to me :)

[Carlos Garcia Campos]

Created attachment 424236 [details]
Patch

[Adrian Perez]

Comment on attachment 424236 [details]
Patch

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

> Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp:131
> +            jscContextJSValueToGValue(context.get(), argumentIndex < argumentCount ? arguments[argumentIndex] : JSValueMakeUndefined(jsContext),

This is calling JSValueMakeUndefined() for each missing parameter. I suppose it's fast
anyway because “undefined” is a singleton.

We could use two loops, one for the parameters which were passed to the function call,
and a second one for the missing parameters, and call JSMakeUndefined only once. That
would also allow the conditional with the ternary operator in each iteration of the
loop 🤔️

> Source/JavaScriptCore/API/glib/JSCContext.cpp:465
> +static gpointer jscContextJSValueToWrappedObject(JSCContext* context, JSValueRef jsValue)

static inline?

[Carlos Garcia Campos]

Committed r275031 (235752@main): <https://commits.webkit.org/235752@main>

[Adrian Perez]

(In reply to Adrian Perez from comment #6)
> Comment on attachment 424236 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424236&action=review
> 
> > Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp:131
> > +            jscContextJSValueToGValue(context.get(), argumentIndex < argumentCount ? arguments[argumentIndex] : JSValueMakeUndefined(jsContext),
> 
> This is calling JSValueMakeUndefined() for each missing parameter. I suppose
> it's fast
> anyway because “undefined” is a singleton.
> 
> We could use two loops, one for the parameters which were passed to the
> function call,
> and a second one for the missing parameters, and call JSMakeUndefined only
> once. That
> would also allow the conditional with the ternary operator in each iteration
> of the
> loop 🤔️
> 
> > Source/JavaScriptCore/API/glib/JSCContext.cpp:465
> > +static gpointer jscContextJSValueToWrappedObject(JSCContext* context, JSValueRef jsValue)
> 
> static inline?

Forgot to say in the overall comment, to be clear: let's merge this
as-is, because anyway it is not worth it to optimize without profiling;
I was only throwing ideas around :)