Bug 119487

Summary: [Gtk] Cancel authentication on load failed
Product: WebKit Reporter: Brian Holt <brian.holt>
Component: WebKitGTKAssignee: Anton Obzhirov <obzhirov>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, commit-queue, gustavo, mrobinson, obzhirov
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 99352    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Brian Holt 2013-08-05 07:16:18 PDT
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.
Comment 1 Anton Obzhirov 2013-08-19 10:07:48 PDT
Created attachment 209098 [details]
Patch
Comment 2 WebKit Commit Bot 2013-08-19 10:09:43 PDT
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
Comment 3 Martin Robinson 2013-08-19 10:09:50 PDT
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.
Comment 4 Anton Obzhirov 2013-08-20 06:44:28 PDT
Created attachment 209191 [details]
Patch
Comment 5 Martin Robinson 2013-08-20 07:12:29 PDT
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.
Comment 6 Anton Obzhirov 2013-08-20 07:59:06 PDT
Created attachment 209196 [details]
Patch

Updated with Martin's comments
Comment 7 WebKit Commit Bot 2013-08-20 08:56:33 PDT
Comment on attachment 209196 [details]
Patch

Clearing flags on attachment: 209196

Committed r154329: <http://trac.webkit.org/changeset/154329>
Comment 8 WebKit Commit Bot 2013-08-20 08:56:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Carlos Garcia Campos 2013-08-20 23:02:55 PDT
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.
Comment 10 Anton Obzhirov 2013-08-21 03:33:53 PDT
(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.
Comment 11 Martin Robinson 2013-08-21 07:41:22 PDT
(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.
Comment 12 Brian Holt 2013-08-21 08:08:39 PDT
(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?
Comment 13 Carlos Garcia Campos 2013-08-21 08:21:08 PDT
(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.
Comment 14 Carlos Garcia Campos 2013-08-21 08:24:19 PDT
(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.
Comment 15 Anton Obzhirov 2013-08-21 08:44:41 PDT
(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.