Bug 136989 – [GTK][WPE] JSC bindings not introspectable

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Georges Basile Stavracas Neto]

Currently I was faced with a very annoying bug with Python3 and WebKit2 Gtk+.

I try to run a JavaScript code, say:

document.getYOffset

The code runs asynchronously, and I when it finished, I get a reference to the JavaScriptResult as expected. When I try to retrieve the JavaScriptCore.Value or JavaScriptCore.GlobalContext, it simply won't run. Even more, it gives me the following message: " Couldn't find conversion for foreign structure 'JavaScriptCore.Value' ".

Created attachment 260684 [details]
patch that adds function get_value_as_string to WebKitJavaScriptResult

Hi,

I had the same problem with the send-message-received singal of UserContentManager.

The problem is JavaScriptCore doesn't have a gobject based API and therefore the introspection doesn't know what to do with it.

There are some JavaScriptCore bindings for python, but all of them are unmaintained or less than ideal in some way or  the other.

Even with those bindings (or using cffi directly) I've found no way to get the actual JSValueRef address in memory and use it from python through the C API. (If someone knows how it would be possible to do this, please enlighten me. It seems you can't easily get around the "Couldn't find conversion for foreign structure 'JavaScriptCore.Value'" error)

For my use case (receiving a message) and I imagine a lot of other use cases what I want in the end is to get a string (in python or C) representing the value, not the JSValueRef itsself.

So I created a new function for WebKitJavaScriptResult named get_value_as_string, which simply converts the value represented by JSValueRef to a gchar* (user is responsible for freeing it) and returns it.

This also works for numbers by the way, they simply get converted to a string. Objects get converted to a string like "[object Object]", so this is at least letting you know that you need to convert the object to a string with something like JSON.stringify from within JavaScript. If anything can't be converted at all, NULL is returned.

I attached the patch. It should work both with 2.8.5 and with master.

Comment on attachment 260684 [details]
patch that adds function get_value_as_string to WebKitJavaScriptResult

https://bugs.webkit.org/show_bug.cgi?id=136989

I had the same problem with the send-message-received singal of UserContentManager.

The problem is JavaScriptCore doesn't have a gobject based API and therefore the introspection doesn't know what to do with it.

There are some JavaScriptCore bindings for python, but all of them are unmaintained or less than ideal in some way or  the other.

Even with those bindings (or using cffi directly) I've found no way to get the actual JSValueRef address in memory and use it from python through the C API. (If someone knows how it would be possible to do this, please enlighten me. It seems you can't easily get around the "Couldn't find conversion for foreign structure 'JavaScriptCore.Value'" error)

For my use case (receiving a message) and I imagine a lot of other use cases what I want in the end is to get a string (in python or C) representing the value, not the JSValueRef itsself.

So I created a new function for WebKitJavaScriptResult named get_value_as_string, which simply converts the value represented by JSValueRef to a gchar* (user is responsible for freeing it) and returns it.

This also works for numbers by the way, they simply get converted to a string. Objects get converted to a string like "[object Object]", so this is at least letting you know that you need to convert the object to a string with something like JSON.stringify from within JavaScript. If anything can't be converted at all, NULL is returned.

I attached the patch. It should work both with 2.8.5 and with master.

[Michael Catanzaro]

Comment on attachment 260684 [details]
patch that adds function get_value_as_string to WebKitJavaScriptResult

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

This patch looks good to me; all my comments are just regarding code style. It looks like you're already familiar with GNOME coding style, which is great, but WebKit style is rather different [1]. We only use the GNOME style in our public header files.

Carlos Lopez mentioned on IRC that the patch needs to include a changelog entry [2], which can be created with the prepare-ChangeLog script.

Since this patch adds new API, it needs to be discussed on the mailing list [3] and then approved by two GTK+ reviewers. Since it's a small addition I think it should be easy: just write a mail introducing your proposal; signing up for the list will make this easier so that your messages aren't queued for moderation, but isn't mandatory.

[1] http://www.webkit.org/coding/coding-style.html
[2] http://www.webkit.org/coding/contributing.html
[3] https://lists.webkit.org/mailman/listinfo/webkit-gtk

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:118
> + * gets converted to a string, if possible. In any case where this isn't possible NULL is returned. 

Write %NULL so that it appears pretty in the documentation

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:121
> + * string; free with g_free()

I don't think you need to mention how to free it; that should be apparent due to the (transfer full) annotation.

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:129
> +    gsize       str_length;

In WebKit we don't use this extra space between the type and the variable name, and the * always goes on the type, not the variable name. Also, we declare variables at the point of first use whenever possible, instead of at the top of functions. You can use 'check-webkit-style' (which will also be run during 'webkit-patch upload' if you use that) to find most style issues; I think it is smart enough to automatically find most of the style issues I found here.

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:131
> +    js_str_value = JSValueToStringCopy (context, javascriptResult->value, NULL);

Declare JSStringRef on this line, omit the space before the opening parenthesis in the function call, and use nullptr rather than NULL.

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:136
> +        return NULL;

Omit the braces around one-line statements, and use nullptr instead of NULL. I would also omit the comment here, since the behavior is clear from the doc comment up above.

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:139
> +    str_length = JSStringGetMaximumUTF8CStringSize (js_str_value);

Declare str_length on this line, and again omit the extra space before the opening parenthesis in the function call (and for the other function calls too).

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:140
> +    str_value = (gchar *)g_malloc (str_length);

Declare str_value on this line, use static_cast<gchar*> instead of the C cast.

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:142
> +    JSStringRelease (js_str_value);

Also, in all these cases, omit the extra space before the opening parenthesis in the function call.

[Michael Catanzaro]

Also when you've updated your patch, be sure to set the r? and cq? flags to request formal review (which someone else will give) and also automated commit (since you don't have commit rights yourself).

[Carlos Garcia Campos]

Thanks for the patch. I think the long term solution would be to have GObject bindings for JSC API, but for now I guess we need something current WebKit bindings could use. However, I'm not sure adding _as_string is the best approach.  Why as_string and not also as_number, as_bool, etc.? I prefer if we add something like as_gvalue() for example, that would work for bindings, but would also be more convenient to use for C API users and would cover all possible values of the result that can be represented as a GValue.

(In reply to comment #6)
> Thanks for the patch. I think the long term solution would be to have
> GObject bindings for JSC API, but for now I guess we need something current
> WebKit bindings could use. However, I'm not sure adding _as_string is the
> best approach.  Why as_string and not also as_number, as_bool, etc.? I
> prefer if we add something like as_gvalue() for example, that would work for
> bindings, but would also be more convenient to use for C API users and would
> cover all possible values of the result that can be represented as a GValue.

I agree with the long term solution.

Why as_string and not as_number and as_bool?

Because numbers and bools can (and do) automatically get "stringified" (I don't mean from the JS-code side, I mean by JSValueToStringCopy), so numbers and booleans are also "covered" with this solution with little extra "costs" and nothing extra to maintain. Furthermore the assumption can be made that people will send complex data as JSON strings with JSON.stringify and than decode the JSON with something like JSON.loads (in python).

The as_gvariant / as_gvalue idea, also occured to me, but seeing that strings can be very easily converted to ints/floats/booleans in python and also seeing that JSON.stringify (js) and JSON.loads (python) work flawlessly through postMessage()/as_string, I saw little need for as_number, as_bool or as_gvariant as it would complicate the code a little bit (I mean in the webkitjavascriptresult implementation, but also when you're using the API in python). I'm not against as_gvalue, but I'd like to have as_string, as it makes the python code really simple.

Also consider this example in the documenation from:
https://lazka.github.io/pgi-docs/index.html#WebKit2-4.0/classes/WebView.html#WebKit2.WebView.run_javascript_finish

static void
web_view_javascript_finished (GObject      *object,
                              GAsyncResult *result,
                              gpointer      user_data)
{
    WebKitJavascriptResult *js_result;
    JSValueRef              value;
    JSGlobalContextRef      context;
    GError                 *error = NULL;

    js_result = webkit_web_view_run_javascript_finish (WEBKIT_WEB_VIEW (object), result, &error);
    if (!js_result) {
        g_warning ("Error running javascript: %s", error->message);
        g_error_free (error);
        return;
    }

    context = webkit_javascript_result_get_global_context (js_result);
    value = webkit_javascript_result_get_value (js_result);
    if (JSValueIsString (context, value)) {
        JSStringRef js_str_value;
        gchar      *str_value;
        gsize       str_length;

        js_str_value = JSValueToStringCopy (context, value, NULL);
        str_length = JSStringGetMaximumUTF8CStringSize (js_str_value);
        str_value = (gchar *)g_malloc (str_length);
        JSStringGetUTF8CString (js_str_value, str_value, str_length);
        JSStringRelease (js_str_value);
        g_print ("Script result: %s\n", str_value);
        g_free (str_value);
    } else {
        g_warning ("Error running javascript: unexpected return value");
    }
    webkit_javascript_result_unref (js_result);
}

That's a lot of boilerplate code simply to get a string. I imagine that getting a string is a common end result you want from javascript_finish and from messageHandlers. In C you can at least do it, but look at how much code that is!
Now you can you use as_string in this C example and be done. So, this is an additional reason for having as_string beyond just enabling python scripts to use the API at all.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #6)
> > Thanks for the patch. I think the long term solution would be to have
> > GObject bindings for JSC API, but for now I guess we need something current
> > WebKit bindings could use. However, I'm not sure adding _as_string is the
> > best approach.  Why as_string and not also as_number, as_bool, etc.? I
> > prefer if we add something like as_gvalue() for example, that would work for
> > bindings, but would also be more convenient to use for C API users and would
> > cover all possible values of the result that can be represented as a GValue.
> 
> I agree with the long term solution.
> 
> Why as_string and not as_number and as_bool?
> 
> Because numbers and bools can (and do) automatically get "stringified" (I
> don't mean from the JS-code side, I mean by JSValueToStringCopy), so numbers
> and booleans are also "covered" with this solution with little extra "costs"
> and nothing extra to maintain. Furthermore the assumption can be made that
> people will send complex data as JSON strings with JSON.stringify and than
> decode the JSON with something like JSON.loads (in python).

In that case maybe it would be better to use to_string() since we are converting the value to a string in some cases.

> The as_gvariant / as_gvalue idea, also occured to me, but seeing that
> strings can be very easily converted to ints/floats/booleans in python and
> also seeing that JSON.stringify (js) and JSON.loads (python) work flawlessly
> through postMessage()/as_string, I saw little need for as_number, as_bool or
> as_gvariant as it would complicate the code a little bit (I mean in the
> webkitjavascriptresult implementation, but also when you're using the API in
> python). I'm not against as_gvalue, but I'd like to have as_string, as it
> makes the python code really simple.
> 
> Also consider this example in the documenation from:
> https://lazka.github.io/pgi-docs/index.html#WebKit2-4.0/classes/WebView.
> html#WebKit2.WebView.run_javascript_finish
> 
> static void
> web_view_javascript_finished (GObject      *object,
>                               GAsyncResult *result,
>                               gpointer      user_data)
> {
>     WebKitJavascriptResult *js_result;
>     JSValueRef              value;
>     JSGlobalContextRef      context;
>     GError                 *error = NULL;
> 
>     js_result = webkit_web_view_run_javascript_finish (WEBKIT_WEB_VIEW
> (object), result, &error);
>     if (!js_result) {
>         g_warning ("Error running javascript: %s", error->message);
>         g_error_free (error);
>         return;
>     }
> 
>     context = webkit_javascript_result_get_global_context (js_result);
>     value = webkit_javascript_result_get_value (js_result);
>     if (JSValueIsString (context, value)) {
>         JSStringRef js_str_value;
>         gchar      *str_value;
>         gsize       str_length;
> 
>         js_str_value = JSValueToStringCopy (context, value, NULL);
>         str_length = JSStringGetMaximumUTF8CStringSize (js_str_value);
>         str_value = (gchar *)g_malloc (str_length);
>         JSStringGetUTF8CString (js_str_value, str_value, str_length);
>         JSStringRelease (js_str_value);
>         g_print ("Script result: %s\n", str_value);
>         g_free (str_value);
>     } else {
>         g_warning ("Error running javascript: unexpected return value");
>     }
>     webkit_javascript_result_unref (js_result);
> }

Note that this example only uses JSValueToStringCopy if JSValueIsString() return true, to make it clear that we are expecting a string and getting a string.

> That's a lot of boilerplate code simply to get a string. I imagine that
> getting a string is a common end result you want from javascript_finish and
> from messageHandlers. In C you can at least do it, but look at how much code
> that is!

I agree, and that's why I said it would work for bindings but also would make the API more convenient.

> Now you can you use as_string in this C example and be done. So, this is an
> additional reason for having as_string beyond just enabling python scripts
> to use the API at all.

I agree, yes.

> In that case maybe it would be better to use to_string() since we are
> converting the value to a string in some cases.

I assume you mean .to_string(), because .get_value_to_string() would obviously not make sense. I chose .get_value_as_string(), because it's explicit, but now I think .to_string() is even better (I thought "as" implied conversion, too, but .to_string() is both shorter and more common in APIs, and as you said it implies conversion). +1 for .to_string() I'll change this in the next update to the patch, unless somebody has an even better idea or contrary opinion.

[Jack Goofy]

modified the patch from anewtobi with the comments from Michael.

--- a/Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp      2016-03-17 16:58:24.866081047 +0100
+++ b/Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp       2016-03-17 17:09:58.702068622 +0100
@@ -108,3 +108,34 @@
 {
     return javascriptResult->value;
 }
+
+
+/**
+ * webkit_javascript_result_to_string:
+ * @js_result: a #WebKitJavascriptResult
+ *
+ * Get string representing the value of @js_result. The value referenced by <function>JSValueRef</function>
+ * gets converted to a string, if possible. In any case where this isn't possible %NULL is returned. 
+ *
+ * Return value: (transfer full): a newly allocated result
+ * string; free with g_free()
+ */
+gchar* webkit_javascript_result_to_string(WebKitJavascriptResult* javascriptResult)
+{
+    JSGlobalContextRef context = webkit_javascript_result_get_global_context(javascriptResult);
+
+    JSStringRef js_str_value = JSValueToStringCopy(context, javascriptResult->value, nullptr);
+    
+    // If JSValueToStringCopy returns Null, this means JSValueRef couldn't be converted to a JSStringRef,
+    // so we return NULL
+    if (!js_str_value) {
+        return nullptr;
+    }
+
+    gsize str_length = JSStringGetMaximumUTF8CStringSize(js_str_value);
+    gchar* str_value = static_cast<gchar*>(g_malloc(str_length));
+    JSStringGetUTF8CString(js_str_value, str_value, str_length);
+    JSStringRelease(js_str_value);
+
+    return str_value;
+}



--- a/Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.h        2013-08-03 18:10:40.000000000 +0200
+++ a/Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.h 2016-03-17 17:15:28.362062718 +0100
@@ -50,6 +50,9 @@
 WEBKIT_API JSValueRef
 webkit_javascript_result_get_value          (WebKitJavascriptResult *js_result);
 
+WEBKIT_API gchar*
+webkit_javascript_result_to_string          (WebKitJavascriptResult *js_result);
+
 G_END_DECLS
 
 #endif

[Jack Goofy]

Created attachment 293003 [details]
patch that adds function get_value_to_string to WebKitJavaScriptResult

[Michael Catanzaro]

Thanks for your patch Jack!

I don't see any reason not to add webkit_javascript_result_to_string, but I think it belongs in a separate bug since it doesn't really solve the problem here. Georges wants to be able to use JSC from Python, that requires introspection support, and that in turn depends on bug #164061. So I think you should file a new bug and attach your patch there. Please also see https://webkit.org/contributing-code/ for tips on contributing patches (set the r? and cq? Bugzilla flags, give the patch a ChangeLog entry).

As for the patch itself, it looks sane to me, just one style nit in the header file:

WEBKIT_API gchar*

Since the header file follows GNOME coding style, it should be gchar * with a space before the *, unlike the WebKit coding style used in the rest of the code.

[Carlos Garcia Campos]

*** Bug 170621 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

Created attachment 335107 [details]
WIP patch

This is a WIP patch based on new API added in patch attached to bug #164061

[Carlos Garcia Campos]

Created attachment 335109 [details]
WIP patch

[Carlos Garcia Campos]

Created attachment 335110 [details]
WIP patch

[Carlos Garcia Campos]

Created attachment 335293 [details]
WIP patch

[Carlos Garcia Campos]

Created attachment 335930 [details]
WIP patch

[Michael Catanzaro]

Comment on attachment 335930 [details]
WIP patch

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

My attention span compares unfavorably to Donald Trump, so I'm just skimming through this...

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:66
>  #include <JavaScriptCore/JSRetainPtr.h>
> +#include <jsc/JSCContextPrivate.h>
>  #include <WebCore/CertificateInfo.h>

Again, style checker will complain about alphabetization when you set r?

> Source/WebKit/UIProcess/API/gtk/WebKitJavascriptResult.h:52
>  WEBKIT_API JSValueRef
>  webkit_javascript_result_get_value          (WebKitJavascriptResult *js_result);

Shall we deprecate it? I think so, otherwise it will be confusing.

> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:435
>  WEBKIT_API JSGlobalContextRef
>  webkit_web_view_get_javascript_global_context        (WebKitWebView             *web_view);

Ditto, we should probably deprecate all the JSC C API stuff.

In the unlikely event we ever decide that was a mistake, we can always un-deprecate it. Not like it's going away in the GTK+ 3 API. But this would allow us to not expose the C API at all in the GTK+ 4 API.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #19)
> Comment on attachment 335930 [details]
> WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335930&action=review
> 
> My attention span compares unfavorably to Donald Trump, so I'm just skimming
> through this...

Thanks for taking the time.

> > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:66
> >  #include <JavaScriptCore/JSRetainPtr.h>
> > +#include <jsc/JSCContextPrivate.h>
> >  #include <WebCore/CertificateInfo.h>
> 
> Again, style checker will complain about alphabetization when you set r?
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitJavascriptResult.h:52
> >  WEBKIT_API JSValueRef
> >  webkit_javascript_result_get_value          (WebKitJavascriptResult *js_result);
> 
> Shall we deprecate it? I think so, otherwise it will be confusing.

Yes, this is a WIP patch, I just did the minimum I needed to make thr whole thing work and port epiphany. I'll deprecate the old methods and document the new ones.

> > Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:435
> >  WEBKIT_API JSGlobalContextRef
> >  webkit_web_view_get_javascript_global_context        (WebKitWebView             *web_view);
> 
> Ditto, we should probably deprecate all the JSC C API stuff.

Ditto.

> In the unlikely event we ever decide that was a mistake, we can always
> un-deprecate it. Not like it's going away in the GTK+ 3 API. But this would
> allow us to not expose the C API at all in the GTK+ 4 API.

You don't have to convince me :-) my idea has always been to deprecate the JSC C API usage in WebKit API.

[Carlos Garcia Campos]

Created attachment 336120 [details]
Patch

[Carlos Garcia Campos]

Committed r229799: <https://trac.webkit.org/changeset/229799>