Bug 246852 – [GTK] Crash on authentication dialog with GTK 4

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

Created attachment 463147 [details]
webkit-gtk-5 crash on authentication dialog

When accessing a page with basic authentication (such as on a cheap router)
I'm encountering the following crash on gtk-4.8.1 and webkitgtk-3.38.1

Created attachment 463425 [details]
fix-gtk4-auth-dialog-crashes.-take-one.patch

This is on top of 2.38.1 and seems to fix my crash and additional gtk4 issues, please review and adapt to master after fixing comments.

[Michael Catanzaro]

Hi, to land this patch you'll want to rebase it on main rather than 2.38.1, then open a pull request following the instructions at https://webkit.org/contributing-code/. Thanks!

Sorry, I'm not set up to work with the WebKit git codebase.  As I indicated I only provided the patch and sufficient information for you 
(or someone  other designated develope) and use as fit to fix the problems
indicated) - is a review by the developers not possible?

[Michael Catanzaro]

I'm not going to review a stable branch patch, sorry.

[Michael Catanzaro]

BTW it is still possible to request patch review on Bugzilla by setting the r? Bugzilla flag (but don't use that if your patch does not apply on main).

(In reply to Michael Catanzaro from comment #4)
> I'm not going to review a stable branch patch, sorry.

Hello Michael, I tried rebasing my patches on top of 2.39.1 which I think was released three days ago and this patch applied without any changes.

I think this patch should still valid on master, Could you please try
it?

[Michael Catanzaro]

In that case, you can set the r? Bugzilla flag to request review and start the CI bots running. It's easier for both of us if you use GitHub, but I can review it here too if you prefer.

I do see that HTTP auth is completely broken, and I've no doubt that you've fixed it, which is great, thanks. That said, to land the patch it really has to comply with our rules. E.g. the commit message has to be in the expected format or commit-queue will reject it. So please do review https://webkit.org/contributing-code/ again.

I didn't spot the r? flag but I clicked on a button which requests ews analysis. Maybe the flag will show up after some time.
The patch should of course not be committed without removing some
personal temporary inline comments first. But I expect the  commit message
style bots will catch it first.

[Michael Catanzaro]

Here are the errors from the style bot:

ERROR: Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:69:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
ERROR: Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:192:  Declaration has space between type name and * in GtkWidget *tmplabel  [whitespace/declaration] [3]
ERROR: Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1890:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/API/gtk/WebKitWebViewDialog.cpp:127:  Should have a space between // and comment  [whitespace/comments] [4]

It won't check your commit message for you until you try to commit, at which point it will fail. Just try to make yours look like what everybody else does and you'll be fine. The title of the bug report is the first line, then a link to the bug on the next line, then a blank line, then "Reviewed by NOBODOY (OOPS!)", then a blank line, then describe your commit, then follow up with the yucky GNU changelog.

[Michael Catanzaro]

Comment on attachment 463425 [details]
fix-gtk4-auth-dialog-crashes.-take-one.patch

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

OK, you're on the right track here. Thanks for your patch. Besides fixing the style bot and rewriting the commit message, there are some more things to change....

> Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:69
> -    bool rememberPassword = gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(priv->rememberCheckButton));
> +    bool rememberPassword =
> +#if USE(GTK4)
> +        gtk_check_button_get_active(GTK_CHECK_BUTTON(priv->rememberCheckButton))
> +#else
> +        gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(priv->rememberCheckButton))
> +#endif
> +        ;

Good change, but style nit; let's avoid breaking statements or control with preprocessor guards; instead, just duplicate a small amount of code.

#if USE(GTK4)
    bool rememberPassword = gtk_check_button_get_active(GTK_CHECK_BUTTON(priv->rememberCheckButton));
#else
    bool rememberPassword = gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(priv->rememberCheckButton));
#endif

It's easier to read this way, and you're only duplicating the "bool rememberPassword =" and the semicolon, which is hardly a big deal.

> Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:194
> +    // ;madhu 221105 Gtk4 creates a checkbutton with a `label'
> +    // property. Don't try to set linewrap on a non-existent label in
> +    // that case.
> +    GtkWidget *tmplabel = gtk_check_button_get_child(GTK_CHECK_BUTTON(priv->rememberCheckButton));
> +    if (tmplabel)
> +        gtk_label_set_line_wrap(GTK_LABEL(tmplabel), TRUE);

So the problem here is gtk_check_button_get_child() returns nullptr? Presumably it always returns nullptr, so the `if (tmplable)` is designed to catch a case that will never be hit, correct? Looks like this code here just needs to be removed or replaced with some other way of doing things.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1863
> +// ;madhu 221105 focus is not a valid signal for a Dialog Window in gtk4. use move-focus instead

Please remove all your personal comments, as they don't belong in the final version of the code.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewDialog.cpp:128
> +//;madhu 221105
> +    gtk_widget_set_can_focus(GTK_WIDGET(object), TRUE);

Style nit: add an extra blank line here.

BTW you know this is a GTK 3 block, right? Why are you changing this for GTK 3?

[Michael Catanzaro]

Comment on attachment 463425 [details]
fix-gtk4-auth-dialog-crashes.-take-one.patch

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

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1873
> +#if USE(GTK4)
> +static void webkitWebViewBaseMoveFocus(GtkWidget* widget, GtkDirectionType direction)
> +{
> +    WebKitWebViewBasePrivate* priv = WEBKIT_WEB_VIEW_BASE(widget)->priv;
> +    if (priv->dialog) {
> +        g_signal_emit_by_name(priv->dialog, "move-focus", direction);
> +    }
> +
> +    GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->move_focus(widget, direction);
> +}

Question for developers familiar with GTK: how bad is this? Somehow I suspect there is a better way than using g_signal_emit_by_name(), and yet this seems like it should work....

(In reply to Michael Catanzaro from comment #10)

>> Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:194
>> +    if (tmplabel)
>> +        gtk_label_set_line_wrap(GTK_LABEL(tmplabel), TRUE);

> So the problem here is gtk_check_button_get_child() returns nullptr?
> Presumably it always returns nullptr, so the `if (tmplable)` is
> designed to catch a case that will never be hit, correct? Looks like
> this code here just needs to be removed or replaced with some other
> way of doing things.

Yes. I'm pretty sure it always returns null. I would replace the #if
USE(GTK) block with just that comment and no code, but perhaps that is
bad style to have a block with no code.

Let me know if you want to remove the gtk4 section and just have a if
!USE(GTK4) block.

> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1863
> > +// ;madhu 221105 focus is not a valid signal for a Dialog Window in gtk4. use move-focus instead
>
> Please remove all your personal comments, as they don't belong in the final version of the code.

Yes of course. Should I leave a comment here that says
// focus is not a valid signal for a Dialog Window in gtk4. use move-focus instead
or is it the commit message enough? I'd leave it in

> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewDialog.cpp:128
> > +//;madhu 221105
> > +    gtk_widget_set_can_focus(GTK_WIDGET(object), TRUE);
>
> Style nit: add an extra blank line here.
>
> BTW you know this is a GTK 3 block, right? Why are you changing this
> for GTK 3?

I'll just remove this altogether. the commit message for this says
this is probably unnecessary, I probably thought this was required
when reading the gtk-doc for the function since i didn't know if the
default state was to "can_set_focus" TRUE, but if it worked before it
should be OK.

Thanks for the comments,I'll try to submit a new patch once I get your feedback on the 2
points

[Michael Catanzaro]

(In reply to enometh from comment #12)
> Let me know if you want to remove the gtk4 section and just have a if
> !USE(GTK4) block.

Yes, do that.

> > Please remove all your personal comments, as they don't belong in the final version of the code.
> 
> Yes of course. Should I leave a comment here that says
> // focus is not a valid signal for a Dialog Window in gtk4. use move-focus
> instead
> or is it the commit message enough? I'd leave it in

The commit message is enough. No need to retain the comment. The fact that you've done something different in the USE(GTK4) block than in the !USE(GTK4) block is already enough documentation that the signal has changed.

Created attachment 463532 [details]
fix gtk4 auth dialog crashes. take two

[Michael Catanzaro]

Comment on attachment 463532 [details]
fix gtk4 auth dialog crashes. take two

OK, your changelog format in your commit message is a little weird, but I don't care.

You have one remaining style error to fix:

ERROR: Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1889:  One line control clauses should not use braces.  [whitespace/braces] [4]

[Michael Catanzaro]

When you fix this, set the cq? flag to request commit.

Created attachment 463548 [details]
fix gtk4 auth dialog crashes. take three

I removed some empty lines from the commit message and think it makes it look like what everybody else does, (i.e. slightly less readable..
 :)

[EWS]

Commit message contains (OOPS!), blocking PR #None

[Michael Catanzaro]

Hmm, I guess commit-queue doesn't replace the Reviewed by line anymore. That used to be done automatically. Oh well. You will have to manually replace the "NOBODY (OOPS!)" with "Michael Catanzaro" for it to be accepted.

Created attachment 463556 [details]
fix gtk4 auth dialog crashes. take three

[EWS]

Committed 256742@main (267e69155f83): <https://commits.webkit.org/256742@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 463556 [details].