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

[bugzilla-daemon at webkit.org]

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

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #349542|review?                     |review+
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180913/0367fa18/attachment-0001.html>