| Summary: | [GTK][WPE] JSC crashes if a function expects a parameter but doesn't receive any | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alberto Garcia <berto> | ||||||
| Component: | WebKitGTK | Assignee: | Carlos Garcia Campos <cgarcia> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | aperez, bugs-noreply, cgarcia, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo | ||||||
| Priority: | P2 | ||||||||
| Version: | Other | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
We need to fix the crash, but I don't think we should throw a exception, it should just receive undefined. (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*”? We would pass undefined for the case of parameters expecting JSCValue, otherwise we will throw an exception. (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 :) Created attachment 424236 [details]
Patch
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? Committed r275031 (235752@main): <https://commits.webkit.org/235752@main> (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 :) |
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.