[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

--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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.

-- 
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/209aba59/attachment.html>