Bug 119487 – [Gtk] Cancel authentication on load failed

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

[Brian Holt]

If loading fails or is stopped on a page which requires HTTP authentication, the default dialog does not close (as would be expected) and the authentication is not cancelled.

What this probably requires is the WebKitWebView retain a ref to an AuthenticationRequest in its private struct that it can cancel the request on the LoadFailed signal.

[Anton Obzhirov]

Created attachment 209098 [details]
Patch

[WebKit Commit Bot]

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

[Martin Robinson]

Comment on attachment 209098 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:99
> +    g_signal_connect(webView, "load-failed", G_CALLBACK(pageLoadFailed), authDialog);

You aren't disconnecting form the signal at all in the case that dialog is destroyed.

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.h:52
> +GtkWidget* webkitAuthenticationDialogNew(WebKitAuthenticationRequest*, CredentialStorageMode, WebKitWebView* webView);

You don't need the argument name here.

[Anton Obzhirov]

Created attachment 209191 [details]
Patch

[Martin Robinson]

Comment on attachment 209191 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:139
> +static void webkitAuthenticationDialogDispose(GObject *object)

Your asterisk is in the wrong place here.

[Anton Obzhirov]

Created attachment 209196 [details]
Patch

Updated with Martin's comments

[WebKit Commit Bot]

Comment on attachment 209196 [details]
Patch

Clearing flags on attachment: 209196

Committed r154329: <http://trac.webkit.org/changeset/154329>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

I'm sorry to be late here, but I don't think this is the right solution, since this would require to implement the same thing in the app side for people not using the default dialog. I think it would be possible to cancel the current active WebKitAuthenticationRequest on load failed. This way we could add a signal to the request, so that the user is notified when the request is cancelled. Also, I think this patch should include a unit test.

[Anton Obzhirov]

(In reply to comment #9)
> I'm sorry to be late here, but I don't think this is the right solution, since this would require to implement the same thing in the app side for people not using the default dialog. I think it would be possible to cancel the current active WebKitAuthenticationRequest on load failed. This way we could add a signal to the request, so that the user is notified when the request is cancelled. Also, I think this patch should include a unit test.

Hi, so do you think I should roll out this patch or it is better to create a new bug to implement it the way you said? Current solution does work anyway for the time being so it can stay before I implement the new one.

[Martin Robinson]

(In reply to comment #10)
 
> Hi, so do you think I should roll out this patch or it is better to create a new bug to implement it the way you said? Current solution does work anyway for the time being so it can stay before I implement the new one.

To met at least, it seems reasonable to add the cancellation API in another patch.

[Brian Holt]

(In reply to comment #9)
> I'm sorry to be late here, but I don't think this is the right solution, since this would require to implement the same thing in the app side for people not using the default dialog. I think it would be possible to cancel the current active WebKitAuthenticationRequest on load failed. This way we could add a signal to the request, so that the user is notified when the request is cancelled. Also, I think this patch should include a unit test.

So if I understand correctly, connect to the load-failed signal in the constructor of the WebKitAuthenticationRequest with the callback being webkit_authentication_request_cancel? Additionally then emit a new signal "authentication-failed" that clients can connect to?

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #9)
> > I'm sorry to be late here, but I don't think this is the right solution, since this would require to implement the same thing in the app side for people not using the default dialog. I think it would be possible to cancel the current active WebKitAuthenticationRequest on load failed. This way we could add a signal to the request, so that the user is notified when the request is cancelled. Also, I think this patch should include a unit test.
> 
> Hi, so do you think I should roll out this patch or it is better to create a new bug to implement it the way you said? Current solution does work anyway for the time being so it can stay before I implement the new one.

Don't need to roll it out, since as you say it works and it doesn't new API like my proposal, and maybe even others disagree with me.

[Carlos Garcia Campos]

(In reply to comment #12)
> (In reply to comment #9)
> > I'm sorry to be late here, but I don't think this is the right solution, since this would require to implement the same thing in the app side for people not using the default dialog. I think it would be possible to cancel the current active WebKitAuthenticationRequest on load failed. This way we could add a signal to the request, so that the user is notified when the request is cancelled. Also, I think this patch should include a unit test.
> 
> So if I understand correctly, connect to the load-failed signal in the constructor of the WebKitAuthenticationRequest with the callback being webkit_authentication_request_cancel? Additionally then emit a new signal "authentication-failed" that clients can connect to?

No, the view would keep a pointer (or ref) to the current request. When load-failed happens, the view checks if there's an active auth request and in that case it calls webkit_authentication_request_cancel. When webkit_authentication_request_cancel is called the request might emit a cancelled signal to notify it was cancelled, so that the users can destroy their UI.

[Anton Obzhirov]

(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #9)
> > > I'm sorry to be late here, but I don't think this is the right solution, since this would require to implement the same thing in the app side for people not using the default dialog. I think it would be possible to cancel the current active WebKitAuthenticationRequest on load failed. This way we could add a signal to the request, so that the user is notified when the request is cancelled. Also, I think this patch should include a unit test.
> > 
> > So if I understand correctly, connect to the load-failed signal in the constructor of the WebKitAuthenticationRequest with the callback being webkit_authentication_request_cancel? Additionally then emit a new signal "authentication-failed" that clients can connect to?
> 
> No, the view would keep a pointer (or ref) to the current request. When load-failed happens, the view checks if there's an active auth request and in that case it calls webkit_authentication_request_cancel. When webkit_authentication_request_cancel is called the request might emit a cancelled signal to notify it was cancelled, so that the users can destroy their UI.
Sounds good to me, I will create new bug/patch.