[webkit-reviews] review granted: [Bug 189544] [GTK][WPE] Allow to run script dialogs asynchronously in the UI process : [Attachment 349542] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 189544: [GTK][WPE] Allow to run script dialogs asynchronously in the UI
process
https://bugs.webkit.org/show_bug.cgi?id=189544

Attachment 349542: Patch

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




--- Comment #3 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 349542
  --> https://bugs.webkit.org/attachment.cgi?id=349542
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.