Bug 75543 – [GTK] Add webkit_web_view_run_javascript() to WebKit2 GTK+

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

C API provides:

typedef void (*WKPageRunJavaScriptFunction)(WKSerializedScriptValueRef, WKErrorRef, void*);
WK_EXPORT void WKPageRunJavaScriptInMainFrame(WKPageRef page, WKStringRef script, void* context, WKPageRunJavaScriptFunction function);

So we need to decide how to expose it in the wk2 GTK+ API. I have a couple of questions:
- how to handle the callback? do we use gio async pattern? or do we just provide our own custom callback?
- What does the callback return? libwebkitgtk+ depends on javascriptcore, can use expose JSValueRef in the API? or should we wrap it somehow?

[Carlos Garcia Campos]

Created attachment 126740 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Philippe Normand]

Comment on attachment 126740 [details]
Patch

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

LGTM! I was a bit worried by the JSValue exposure in the API but after talking with Carlos it seems we don't have much choice.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1432
> + * Finish an asynchronous operation started with webkit_web_view_can_execute_editing_command().

Hum copy/paste error? Not sure this has anything to do with webkit_web_view_can_execute_editing_command() ;)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1433
> + * The returned JSValueRef is a temporary value that should be consumed inmediately.

immediately

[Carlos Garcia Campos]

(In reply to comment #3) 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1432
> > + * Finish an asynchronous operation started with webkit_web_view_can_execute_editing_command().
> 
> Hum copy/paste error? Not sure this has anything to do with webkit_web_view_can_execute_editing_command() ;)

oops

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1433
> > + * The returned JSValueRef is a temporary value that should be consumed inmediately.
> 
> immediately

oh!

[Martin Robinson]

Comment on attachment 126740 [details]
Patch

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

Looks pretty good, but it's an error to access JavaScript values with a different global context.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1399
> +    JSGlobalContextRef scriptContext = JSGlobalContextCreate(0);
> +    JSValueRef scriptValue = WKSerializedScriptValueDeserialize(wkSerializedScriptValue, scriptContext, 0);
> +    g_simple_async_result_set_op_res_gpointer(result.get(), const_cast<OpaqueJSValue*>(scriptValue), 0);
> +    g_simple_async_result_complete(result.get());
> +    JSGlobalContextRelease(scriptContext);

Instead of creating a temporary JSGlobalContext, it would probably be better to create one with the WebView, so that JavaScriptCore values can stay alive longer.

> Source/WebKit2/UIProcess/API/gtk/tests/ScriptContext.cpp:45
> +    JSRetainPtr<JSStringRef> stringValue(Adopt, JSValueToStringCopy(m_context, value, 0));
> +    g_assert(stringValue);
> +
> +    size_t cStringLength = JSStringGetMaximumUTF8CStringSize(stringValue.get());
> +    char* cString = static_cast<char*>(g_malloc(cStringLength));
> +    JSStringGetUTF8CString(stringValue.get(), cString, cStringLength);

It's important to access the value with the same JSC context that created the value. Thus it should probably be passed as property of the AsyncResult or kept as a property of the WebKitWebView.

[Martin Robinson]

Comment on attachment 126740 [details]
Patch

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

It seems that there are three things we want to return for every JavaScript invocation: a JSContextRef, a JSValueRef and a JSValueRef for exceptions. I'm not sure the cleanest way to pass all this to the user with the GIO style API.

I'm am fairly certain though that it doesn't make sense to pass the exception as a GError. I think we should reserve GErrors for when we actually have problems executing the JavaScript. It's important that the client can distinguish between script exceptions and problems internal to WebKit. The exception must be a JSValueRef in the end anyhow.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1396
> +    JSValueRef scriptValue = WKSerializedScriptValueDeserialize(wkSerializedScriptValue, scriptContext, 0);

Instead of throwing away the exception it would be better to pass it to the client somehow. It's a pretty important piece of the story.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:474
> +    callbackData.expectedResult = "WebKitGTK+ Title";
> +    test->runJavaScriptAndWaitUntilFinished("window.document.getElementById('WebKitLink').title;", testWebViewRunJavaScriptFinishedCallback, &callbackData);

Instead of writing this with the use of a callback function, why not just store the results of the operation in WebKitWebView and read them after you call runJavaScriptAndWaitUntilFinished. This more closely matches how LoadTrackingTest works. I think it's a little more straight-forward too.

> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.h:52
> +    void runJavaScriptAndWaitUntilFinished(const char* js, JavascriptFinishedCallback, void* userData);

Would prefer something like scriptString instead of just "js"

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 126740 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126740&action=review
> 
> Looks pretty good, but it's an error to access JavaScript values with a different global context.

You mean from a different global context than the one executing the script? because the context where the script executes is in the web process.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1399
> > +    JSGlobalContextRef scriptContext = JSGlobalContextCreate(0);
> > +    JSValueRef scriptValue = WKSerializedScriptValueDeserialize(wkSerializedScriptValue, scriptContext, 0);
> > +    g_simple_async_result_set_op_res_gpointer(result.get(), const_cast<OpaqueJSValue*>(scriptValue), 0);
> > +    g_simple_async_result_complete(result.get());
> > +    JSGlobalContextRelease(scriptContext);
> 
> Instead of creating a temporary JSGlobalContext, it would probably be better to create one with the WebView, so that JavaScriptCore values can stay alive longer.

Ah, so the JSValueRef we return is actually a temp value because it's destroyed by the global context, right? So, is it correct to use the same global context for different scripts? Not that I don't know anything about javascript :-P

> > Source/WebKit2/UIProcess/API/gtk/tests/ScriptContext.cpp:45
> > +    JSRetainPtr<JSStringRef> stringValue(Adopt, JSValueToStringCopy(m_context, value, 0));
> > +    g_assert(stringValue);
> > +
> > +    size_t cStringLength = JSStringGetMaximumUTF8CStringSize(stringValue.get());
> > +    char* cString = static_cast<char*>(g_malloc(cStringLength));
> > +    JSStringGetUTF8CString(stringValue.get(), cString, cStringLength);
> 
> It's important to access the value with the same JSC context that created the value.

Ah, ok you mean the context that deserialized the value, not where the script executed.

> Thus it should probably be passed as property of the AsyncResult or kept as a property of the WebKitWebView.

So, if understand this correctly, we want to use a different context for every javascript execution, to make sure JSValueRefs are accessed with the same context that the one that created deserialized the value. So, I think we could make webkit_web_view_run_javascript_finish return a WebKitJavascripResult object containing the context, the value and the exception. That way the value will be alive until the WebKitJavaScript result is destroyed.

[Carlos Garcia Campos]

(In reply to comment #6)
> (From update of attachment 126740 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126740&action=review
> 
> It seems that there are three things we want to return for every JavaScript invocation: a JSContextRef, a JSValueRef and a JSValueRef for exceptions. I'm not sure the cleanest way to pass all this to the user with the GIO style API.

why not?

> I'm am fairly certain though that it doesn't make sense to pass the exception as a GError. I think we should reserve GErrors for when we actually have problems executing the JavaScript. It's important that the client can distinguish between script exceptions and problems internal to WebKit. The exception must be a JSValueRef in the end anyhow.

Agree, the GError passed to webkit_web_view_run_javascript_finish() wouldn't contain the javascript exection, because the script was correctly executed, if there's an error in the script that's not an error in the async operation. 

So as I suggested before, we could just return a WebKitJavascriptResult object (or boxed type, maybe) containing the context, the value and the exception.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1396
> > +    JSValueRef scriptValue = WKSerializedScriptValueDeserialize(wkSerializedScriptValue, scriptContext, 0);
> 
> Instead of throwing away the exception it would be better to pass it to the client somehow. It's a pretty important piece of the story.

Ok, I see now.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:474
> > +    callbackData.expectedResult = "WebKitGTK+ Title";
> > +    test->runJavaScriptAndWaitUntilFinished("window.document.getElementById('WebKitLink').title;", testWebViewRunJavaScriptFinishedCallback, &callbackData);
> 
> Instead of writing this with the use of a callback function, why not just store the results of the operation in WebKitWebView and read them after you call runJavaScriptAndWaitUntilFinished. 
This more closely matches how LoadTrackingTest works. I think it's a little more straight-forward too.

I didn't do that because with the current code the JSValueRef is a temporary value that is no longer alive after the execution of the callback. If we use the WebKitJavascriptResult, we could just store the result. I didn't like having to use a callback either.
 
> > Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.h:52
> > +    void runJavaScriptAndWaitUntilFinished(const char* js, JavascriptFinishedCallback, void* userData);
> 
> Would prefer something like scriptString instead of just "js"

Ok.

[Martin Robinson]

(In reply to comment #8)

> > It seems that there are three things we want to return for every JavaScript invocation: a JSContextRef, a JSValueRef and a JSValueRef for exceptions. I'm not sure the cleanest way to pass all this to the user with the GIO style API.
> 
> why not?

Sorry. This was a little unclear. What I meant here was that I couldn't quickly find any examples of this kind of situation with the GIO style APIs in devhelp. I guessed that you would just need to implement your own GAsyncResult, but I wasn't confident enough to suggest it. I didn't mean to say that we should do away with GIO style APIs.

> So as I suggested before, we could just return a WebKitJavascriptResult object (or boxed type, maybe) containing the context, the value and the exception.

That sound like a good plan.

> I didn't do that because with the current code the JSValueRef is a temporary value that is no longer alive after the execution of the callback. If we use the WebKitJavascriptResult, we could just store the result. I didn't like having to use a callback either.

Great.

One thing that I think would be useful is to create only one JSGlobalContext for the entire WebKitWebView. Every deserialized JavaScript result could use that same global context. This would be useful because it would allow the user to create other bits of JavaScript and even to pass the return values of these APIs to functions that they create. You could add a getter to WebKitWebView for this global context.

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > > It seems that there are three things we want to return for every JavaScript invocation: a JSContextRef, a JSValueRef and a JSValueRef for exceptions. I'm not sure the cleanest way to pass all this to the user with the GIO style API.
> > 
> > why not?
> 
> Sorry. This was a little unclear. What I meant here was that I couldn't quickly find any examples of this kind of situation with the GIO style APIs in devhelp.

There a lot of examples in GFile API, for example g_file_query_info_async(), g_file_query_info_finish() returns a GFileInfo object.

> I guessed that you would just need to implement your own GAsyncResult, but I wasn't confident enough to suggest it.

No, we can use a GSimpleAsyncResult and store the object with g_simple_async_result_set_op_res_gpointer().

> I didn't mean to say that we should do away with GIO style APIs.

Ok, no problem.

> > So as I suggested before, we could just return a WebKitJavascriptResult object (or boxed type, maybe) containing the context, the value and the exception.
> 
> That sound like a good plan.
> 
> > I didn't do that because with the current code the JSValueRef is a temporary value that is no longer alive after the execution of the callback. If we use the WebKitJavascriptResult, we could just store the result. I didn't like having to use a callback either.
> 
> Great.
> 
> One thing that I think would be useful is to create only one JSGlobalContext for the entire WebKitWebView. Every deserialized JavaScript result could use that same global context. This would be useful because it would allow the user to create other bits of JavaScript and even to pass the return values of these APIs to functions that they create. You could add a getter to WebKitWebView for this global context.

Ok, created on demand I guess?

[Martin Robinson]

(In reply to comment #10)

> There a lot of examples in GFile API, for example g_file_query_info_async(), g_file_query_info_finish() returns a GFileInfo object.

Ah, I see.

> > One thing that I think would be useful is to create only one JSGlobalContext for the entire WebKitWebView. Every deserialized JavaScript result could use that same global context. This would be useful because it would allow the user to create other bits of JavaScript and even to pass the return values of these APIs to functions that they create. You could add a getter to WebKitWebView for this global context.
> 
> Ok, created on demand I guess?

That should be fine.

[Carlos Garcia Campos]

I have found some problems reworking the patch

WebProcess executes the javascript with a NULL exception, and in case of error it sends a NULL serialized value to the UI process. That means that from the UI we only know that the script failed to execute when serialized value is NULL, but we don't have more information about the exception.
We only have information about the exception that might happen deserializing values in the UI process. 

I see two possible solutions:

 1. Create a JSObjectRef with generic exception message (something like, and exception ocurred executing script) and create a WebKitJavascriptResult with that exception.
 2. Add a new WebKitError (WEBKIT_JAVASCRIPT_ERROR,  WEBKIT_JAVASCRIPT_ERROR_EXCEPTION) and fill a GError to finish the async operation so no WebKitJavascriptResult is created.

With first solution all exceptions are handled the same way, and we keep that the GError passed to webkit_web_view_run_javascript_finish() is only for errors of the async operation and not javascript exception. On the other hand, users will expect real exception, with information about the line number and source file, for example. We would be providing such info only for exceptions raised when deserializing the result. Which is a bit confusing.

With the second option we make the async operation fail in case of error in the javascript execution, so we give a GError and the user will expect it to be a error defined by us, not a detailed javascript exception.

So, my proposal is:

Add WEBKIT_JAVASCRIPT_ERROR type with values WEBKIT_JAVASCRIPT_ERROR_EXECUTION and WEBKIT_JAVASCRIPT_ERROR_VALUE (or something like that) and make the async operation fail when the javascript failed to execute and when we fail to deserialize the value, filling a GError in both cases. Then WebKitJavascriptResult won't have any information about exceptions and its value will always be valid.

[Martin Robinson]

(In reply to comment #12)
> I have found some problems reworking the patch
> 
> WebProcess executes the javascript with a NULL exception, and in case of error it sends a NULL serialized value to the UI process. That means that from the UI we only know that the script failed to execute when serialized value is NULL, but we don't have more information about the exception.
> We only have information about the exception that might happen deserializing values in the UI process. 

Would it make sense to get a real value on the WebProcess side and just send that to the UIProcess? That seems like a logical step, unless there is some reason it isn't done now.

[Carlos Garcia Campos]

(In reply to comment #13)
> (In reply to comment #12)
> > I have found some problems reworking the patch
> > 
> > WebProcess executes the javascript with a NULL exception, and in case of error it sends a NULL serialized value to the UI process. That means that from the UI we only know that the script failed to execute when serialized value is NULL, but we don't have more information about the exception.
> > We only have information about the exception that might happen deserializing values in the UI process. 
> 
> Would it make sense to get a real value on the WebProcess side and just send that to the UIProcess? That seems like a logical step, unless there is some reason it isn't done now.

There are actually 3 possible exceptions involved:

1.- Exception raised by the script itself (This is the interesting one)
2.- Exception serializing the value in the web process (This is currently ignored)
3.- Exception deserializing the result value in the UI process

1 and 3 are probably very unlikely to happen. And the exception raised by the script itself, is emitted as an ErrorEvent so it ends up converted into a console message, so it's not possible to send that exception as a JSValueRef to the UI process. So, I guess we should provide a console-message signal that user might connect to to get exceptions and other messages. But addMessageToConsole is currently unimplemented in WebProcess. I'm not sure if there are pans to implement it, so I'm adding Anders and Sam to CC.

[Carlos Garcia Campos]

I've just filed:

[WK2] Implement WebChromeClient::addMessageToConsole()
https://bugs.webkit.org/show_bug.cgi?id=79482

[Carlos Garcia Campos]

Created attachment 132244 [details]
New patch

This patch adds WebKitJavascriptResult boxed type containing information about the result, the JSGlobalContextRef and the JSValueRef. That WebKitJavascriptResult is returned by webkit_web_view_run_javascript_finish(). I've left the exceptions out for now, for the reasons explained in previous comments. When bugs #79482 and #79918 are fixed, we can try to add the exception information to the WebKitJavascriptResult using the console, or just document that user should use the ::console-message signal to monitor script exceptions.

[Martin Robinson]

Comment on attachment 132244 [details]
New patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:38
> +struct _WebKitJavascriptResult {
> +    _WebKitJavascriptResult(WebKitWebView* view, WKSerializedScriptValueRef wkSerializedScriptValue)
> +        : webView(view)
> +        , referenceCount(1)
> +        {
> +            value = WKSerializedScriptValueDeserialize(wkSerializedScriptValue, webkit_web_view_get_javascript_global_context(view), 0);
> +        }
> +
> +    GRefPtr<WebKitWebView> webView;
> +    JSValueRef value;
> +
> +    int referenceCount;
> +};

Instead of implementing your own reference counted type you could make this class extend from RefCounted<_WebKitJavaScriptResult> and use ref() and unref().

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1504
> +        g_set_error_literal(&error, WEBKIT_JAVASCRIPT_ERROR, WEBKIT_JAVASCRIPT_ERROR_SCRIPT_FAILED, _("An exception was raised in Javascript"));

Javascript -> JavaScript

[Carlos Garcia Campos]

(In reply to comment #17)
> (From update of attachment 132244 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132244&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitJavascriptResult.cpp:38
> > +struct _WebKitJavascriptResult {
> > +    _WebKitJavascriptResult(WebKitWebView* view, WKSerializedScriptValueRef wkSerializedScriptValue)
> > +        : webView(view)
> > +        , referenceCount(1)
> > +        {
> > +            value = WKSerializedScriptValueDeserialize(wkSerializedScriptValue, webkit_web_view_get_javascript_global_context(view), 0);
> > +        }
> > +
> > +    GRefPtr<WebKitWebView> webView;
> > +    JSValueRef value;
> > +
> > +    int referenceCount;
> > +};
> 
> Instead of implementing your own reference counted type you could make this class extend from RefCounted<_WebKitJavaScriptResult> and use ref() and unref().

Is it worth converting that to a class? Is RefCounted better than our own implementation? it seems to use a mutex, while our implementation avoid the use of mutex by using atomic operations. I'll leave our current implementation for now, since it's pretty simple, and I'll write a patch to use RefCounted if it's really worth it and better.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1504
> > +        g_set_error_literal(&error, WEBKIT_JAVASCRIPT_ERROR, WEBKIT_JAVASCRIPT_ERROR_SCRIPT_FAILED, _("An exception was raised in Javascript"));
> 
> Javascript -> JavaScript

Ok.

[Carlos Garcia Campos]

Committed r111510: <http://trac.webkit.org/changeset/111510>