Bug 189544 - [GTK][WPE] Allow to run script dialogs asynchronously in the UI process
Summary: [GTK][WPE] Allow to run script dialogs asynchronously in the UI process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 189545
  Show dependency treegraph
 
Reported: 2018-09-12 06:03 PDT by Carlos Garcia Campos
Modified: 2018-09-13 01:24 PDT (History)
5 users (show)

See Also:


Attachments
Patch (31.42 KB, patch)
2018-09-12 06:16 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2018-09-12 06:03:24 PDT
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.
Comment 1 Carlos Garcia Campos 2018-09-12 06:16:38 PDT
Created attachment 349542 [details]
Patch
Comment 2 Build Bot 2018-09-12 06:19:02 PDT
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
Comment 3 Michael Catanzaro 2018-09-12 20:32:12 PDT
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.
Comment 4 Carlos Garcia Campos 2018-09-12 23:49:18 PDT
(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.
Comment 5 Carlos Garcia Campos 2018-09-13 01:24:27 PDT
Committed r235969: <https://trac.webkit.org/changeset/235969>