Bug 189544 – [GTK][WPE] Allow to run script dialogs asynchronously in the UI process

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

They are sync in the WebProcess, but we don't need to block the UI process while they are running. Our current API doesn't allow it, because it laways expect the dialog to be closed in the signal handler.

[Carlos Garcia Campos]

Created attachment 349542 [details]
Patch

[EWS Watchlist]

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

[Michael Catanzaro]

Comment on attachment 349542 [details]
Patch

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

Looks excellent, as usual.

> Source/WebKit/UIProcess/API/glib/WebKitScriptDialog.cpp:36
> +    return !!scriptDialog->completionHandler;

You need the !! with gboolean to convert to 0 or 1... but this is a bool, the conversion to true or false is automatic. (Anyway, doesn't hurt. I guess explicit is better than implicit....)

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2118
> +    webView->priv->currentScriptDialog = webkitScriptDialogCreate(WEBKIT_SCRIPT_DIALOG_ALERT, message, { }, [webView, completionHandler = WTFMove(completionHandler)](bool, const String&) {
> +        completionHandler();
> +        webView->priv->currentScriptDialog = nullptr;
> +    });

What about if:

 * webView->priv->currentScriptDialog is already non-null (a script dialog is currently shown)
 * This gets called again (is it possible?)
 * webView->priv->currentScriptDialog is overwritten with a new WebKitScriptDialog
 * Then: webView is destroyed for unrelated reason
 * webkit_script_dialog_close() is called only for the new WebKitScriptDialog, not the old one
 * completionHandler for the original WebKitScriptDialog remains outstanding (perhaps forever?)
 * If it ever gets called (not sure how it could, with webView destroyed) then webView pointer would be dangling -> use after free

I guess there should only be one dialog at a time? If so, ASSERT(!webView->priv->currentScriptDialog) would be appropriate here (and in all the below functions).

I think I would also add an ASSERT(WEBKIT_IS_WEB_VIEW(webView)) inside these completion handlers to catch if due to some bug it were to ever be dangling.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:572
> +        test->m_delayedScriptDialogs = !!i;

Again, no need for !! unless you really like it.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #3)
> Comment on attachment 349542 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349542&action=review
> 
> Looks excellent, as usual.
> 
> > Source/WebKit/UIProcess/API/glib/WebKitScriptDialog.cpp:36
> > +    return !!scriptDialog->completionHandler;
> 
> You need the !! with gboolean to convert to 0 or 1... but this is a bool,
> the conversion to true or false is automatic. (Anyway, doesn't hurt. I guess
> explicit is better than implicit....)

It's not bool, it's a Function<>. I know it implements bool operator, but in this case I think it's easier to understand with the explicit !!.

> > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2118
> > +    webView->priv->currentScriptDialog = webkitScriptDialogCreate(WEBKIT_SCRIPT_DIALOG_ALERT, message, { }, [webView, completionHandler = WTFMove(completionHandler)](bool, const String&) {
> > +        completionHandler();
> > +        webView->priv->currentScriptDialog = nullptr;
> > +    });
> 
> What about if:
> 
>  * webView->priv->currentScriptDialog is already non-null (a script dialog
> is currently shown)
>  * This gets called again (is it possible?)
>  * webView->priv->currentScriptDialog is overwritten with a new
> WebKitScriptDialog
>  * Then: webView is destroyed for unrelated reason
>  * webkit_script_dialog_close() is called only for the new
> WebKitScriptDialog, not the old one
>  * completionHandler for the original WebKitScriptDialog remains outstanding
> (perhaps forever?)
>  * If it ever gets called (not sure how it could, with webView destroyed)
> then webView pointer would be dangling -> use after free
> 
> I guess there should only be one dialog at a time? If so,
> ASSERT(!webView->priv->currentScriptDialog) would be appropriate here (and
> in all the below functions).

Alerts are sync in the web process, so we can't receive a second alert while one is running. The HTTP auth dialog happens during the network load, so there can't be an alert at that point. I'll add an assert anyway just in case I'm missing an edge case somewhere.

> I think I would also add an ASSERT(WEBKIT_IS_WEB_VIEW(webView)) inside these
> completion handlers to catch if due to some bug it were to ever be dangling.

WebKitWebView closes the dialog on dispose.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:572
> > +        test->m_delayedScriptDialogs = !!i;
> 
> Again, no need for !! unless you really like it.

Yes, in this case assigning an iterator to a boolean looked weird to me. What I would do here is = i != 0, but WebKit coding style doesn't allow to compare to 0, so again explicit !! looked easier to read for me.

[Carlos Garcia Campos]

Committed r235969: <https://trac.webkit.org/changeset/235969>