Bug 99914

Created attachment 171097 [details] Patch

Comment on attachment 171097 [details] Patch Attachment 171097 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14557043

Created attachment 171098 [details] Fix the EFL build

Comment on attachment 171098 [details] Fix the EFL build View in context: https://bugs.webkit.org/attachment.cgi?id=171098&action=review This looks great in general, it would be great if Dan or Sergio could take a look at it too. I'm fine with splitting the patch and adding the password storing support in a follow up patch, but I would wait until the other patch is ready to land both patches together. This way there's only one revision with the regression and we can make sure we won't make a release with regressions. > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:109 > - GOwnPtr<char>description(g_strdup_printf(_("A username and password are being requested by the site %s"), uri->host)); > - GtkWidget* descriptionLabel = gtk_label_new(description.get()); > + String description = String::format(_("A username and password are being requested by the site %s"), > + m_challenge.protectionSpace().host().utf8().data()); I guess String::format can receive a String, so we don't need to convert it to utf8, since the whole string is going to be converted again to utf8 below in gtk_label_new. I think it would be better to use GOwnPtr<char> as we had and avoid charset conversions, we would only need to convert the host. > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:207 > + bool rememberCredentials = gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(m_rememberCheckButton)); > + m_challenge.authenticationClient()->receivedCredential(m_challenge, > + Credential(String::fromUTF8(username), String::fromUTF8(password), > + rememberCredentials ? CredentialPersistencePermanent : CredentialPersistenceForSession)); Maybe we could split this to make it readable? Credential credential = Credential(String::fromUTF8(username), String::fromUTF8(password), rememberCredentials ? CredentialPersistencePermanent : CredentialPersistenceForSession); m_challenge.authenticationClient()->receivedCredential(m_challenge, credential); > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:210 > void GtkAuthenticationDialog::authenticationDialogResponseCallback(GtkWidget*, gint responseID, GtkAuthenticationDialog* dialog) Shouldn't we handle here also the case when the dialog is cancelled and call authenticationClient()->receivedCancellation() so that the message is unpaused? or where is it handled? > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:868 > + if (client()) { > + soup_session_pause_message(challenge.soupSession(), challenge.soupMessage()); > client()->didReceiveAuthenticationChallenge(this, challenge); > + } I'm not sure if it can happen, but the message is only paused when there's a client, but unpaused in the methods below without checking the client. So, I think we should either check for the client below in the other methods, or rather pause always the message here. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:878 > + soup_session_unpause_message(challenge.soupSession(), challenge.soupMessage()); > + > + clearAuthentication(); This is repeated in the two methods below, maybe we can add a simple helper finishAuthentication() that unpuases the message and clears the authentication. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1069 > // We don't need to pass a client here, because GTK+ does not yet use the AuthenticationClient to > // respond to the AuthenticationChallenge -- instead dealing with the Soup objects directly. Should we remove this comment now that we are passing the client? > Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp:78 > + GRefPtr<SoupSession> m_session; This is unused, no?

Comment on attachment 171098 [details] Fix the EFL build View in context: https://bugs.webkit.org/attachment.cgi?id=171098&action=review >> Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:109 >> + m_challenge.protectionSpace().host().utf8().data()); > > I guess String::format can receive a String, so we don't need to convert it to utf8, since the whole string is going to be converted again to utf8 below in gtk_label_new. I think it would be better to use GOwnPtr<char> as we had and avoid charset conversions, we would only need to convert the host. C++ objects cannot be passed through varargs, so String::format cannot take a string. I think performance isn't such a huge issue here. Also consider that we'd be doing charset conversion anyway to convert m_challenge.protectionSpace().host() to UTF-8, so the performance difference is linear. >> Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:207 >> + rememberCredentials ? CredentialPersistencePermanent : CredentialPersistenceForSession)); > > Maybe we could split this to make it readable? > > Credential credential = Credential(String::fromUTF8(username), String::fromUTF8(password), rememberCredentials ? CredentialPersistencePermanent : CredentialPersistenceForSession); > m_challenge.authenticationClient()->receivedCredential(m_challenge, credential); I split this into two lines. >> Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:210 >> void GtkAuthenticationDialog::authenticationDialogResponseCallback(GtkWidget*, gint responseID, GtkAuthenticationDialog* dialog) > > Shouldn't we handle here also the case when the dialog is cancelled and call authenticationClient()->receivedCancellation() so that the message is unpaused? or where is it handled? Yes, good catch. >> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:868 >> + } > > I'm not sure if it can happen, but the message is only paused when there's a client, but unpaused in the methods below without checking the client. So, I think we should either check for the client below in the other methods, or rather pause always the message here. The methods below are only called from the client, so it's correct here to only pause the message if there's a client. If we pause the message with no client, it will never be unpaused. The tricky bit is ensuring that the client always responds to the authentication client interface implementation. I'm thinking about ways to make that a little less error prone. >> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:878 >> + clearAuthentication(); > > This is repeated in the two methods below, maybe we can add a simple helper finishAuthentication() that unpuases the message and clears the authentication. I actually tried this, but it ended up adding more code than removing. >> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1069 >> // respond to the AuthenticationChallenge -- instead dealing with the Soup objects directly. > > Should we remove this comment now that we are passing the client? Yes. Thought I did. :) >> Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp:78 >> + GRefPtr<SoupSession> m_session; > > This is unused, no? Yes. Left over from before the challenge carried a SoupSession.

(In reply to comment #4) > (From update of attachment 171098 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171098&action=review > > This looks great in general, it would be great if Dan or Sergio could take a look at it too. I'm fine with splitting the patch and adding the password storing support in a follow up patch, but I would wait until the other patch is ready to land both patches together. This way there's only one revision with the regression and we can make sure we won't make a release with regressions. I'd prefer to do it in a way that the patches don't bitrot and sit on the bugs for a long time. I'll coordinate with you about the release and ensure that cross-session storage is in place for release-time. That's better for me than just letting the patches sit here.

(In reply to comment #5) > (From update of attachment 171098 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171098&action=review > > >> Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:109 > >> + m_challenge.protectionSpace().host().utf8().data()); > > > > I guess String::format can receive a String, so we don't need to convert it to utf8, since the whole string is going to be converted again to utf8 below in gtk_label_new. I think it would be better to use GOwnPtr<char> as we had and avoid charset conversions, we would only need to convert the host. > > C++ objects cannot be passed through varargs, so String::format cannot take a string. I think performance isn't such a huge issue here. Also consider that we'd be doing charset conversion anyway to convert m_challenge.protectionSpace().host() to UTF-8, so the performance difference is linear. Yes, but as I said we would only need to convert the host. It's not a huge performance penalty, of course, but why using String when what we need is a utf8 char* to pass it to soup? > >> Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:207 > >> + rememberCredentials ? CredentialPersistencePermanent : CredentialPersistenceForSession)); > > > > Maybe we could split this to make it readable? > > > > Credential credential = Credential(String::fromUTF8(username), String::fromUTF8(password), rememberCredentials ? CredentialPersistencePermanent : CredentialPersistenceForSession); > > m_challenge.authenticationClient()->receivedCredential(m_challenge, credential); > > I split this into two lines. Thanks :-) > >> Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:210 > >> void GtkAuthenticationDialog::authenticationDialogResponseCallback(GtkWidget*, gint responseID, GtkAuthenticationDialog* dialog) > > > > Shouldn't we handle here also the case when the dialog is cancelled and call authenticationClient()->receivedCancellation() so that the message is unpaused? or where is it handled? > > Yes, good catch. > > >> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:868 > >> + } > > > > I'm not sure if it can happen, but the message is only paused when there's a client, but unpaused in the methods below without checking the client. So, I think we should either check for the client below in the other methods, or rather pause always the message here. > > The methods below are only called from the client, so it's correct here to only pause the message if there's a client. If we pause the message with no client, it will never be unpaused. The tricky bit is ensuring that the client always responds to the authentication client interface implementation. I'm thinking about ways to make that a little less error prone. Ah, I see, what confused me was that the other functions also check if client() is NULL. > >> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:878 > >> + clearAuthentication(); > > > > This is repeated in the two methods below, maybe we can add a simple helper finishAuthentication() that unpuases the message and clears the authentication. > > I actually tried this, but it ended up adding more code than removing. Yes, indeed :-) > >> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1069 > >> // respond to the AuthenticationChallenge -- instead dealing with the Soup objects directly. > > > > Should we remove this comment now that we are passing the client? > > Yes. Thought I did. :) > > >> Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp:78 > >> + GRefPtr<SoupSession> m_session; > > > > This is unused, no? > > Yes. Left over from before the challenge carried a SoupSession.

(In reply to comment #6) > (In reply to comment #4) > > (From update of attachment 171098 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=171098&action=review > > > > This looks great in general, it would be great if Dan or Sergio could take a look at it too. I'm fine with splitting the patch and adding the password storing support in a follow up patch, but I would wait until the other patch is ready to land both patches together. This way there's only one revision with the regression and we can make sure we won't make a release with regressions. > > I'd prefer to do it in a way that the patches don't bitrot and sit on the bugs for a long time. I'll coordinate with you about the release and ensure that cross-session storage is in place for release-time. That's better for me than just letting the patches sit here. Don't you think you will have a patch for that soon? I don't like landing patches introducing regressions if there isn't a good reason. I know having patches pending in bugzilla is a pain, though, but I'm not sure it's enough reason to introduce regressions, tbh.

(In reply to comment #8) > Don't you think you will have a patch for that soon? I don't like landing patches introducing regressions if there isn't a good reason. I know having patches pending in bugzilla is a pain, though, but I'm not sure it's enough reason to introduce regressions, tbh. One thing that I can do is to hide this new work behind a flag. That way we can land the patches and then just flip the flag and remove the old code when it's complete.

Created attachment 171528 [details] Patch

Created attachment 171541 [details] Patch

Attachment 171541 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1 Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp:64: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 171528 [details] Patch Whoops. Wrong patch for this bug.

Comment on attachment 171528 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171528&action=review > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:112 > - SoupURI* uri = soup_message_get_uri(m_message.get()); > - GOwnPtr<char>description(g_strdup_printf(_("A username and password are being requested by the site %s"), uri->host)); > - GtkWidget* descriptionLabel = gtk_label_new(description.get()); > + String description = String::format(_("A username and password are being requested by the site %s"), > + m_challenge.protectionSpace().host().utf8().data()); > + GtkWidget* descriptionLabel = gtk_label_new(description.utf8().data()); This is causing encoding issues in the dialog label, I guess it's because String::format expects char* but not necessarily a utf8 encoded char*. I still think previous code was simpler and more efficient. > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:266 > dialog->destroy(); > + dialog->m_challenge.authenticationClient()->receivedCancellation(dialog->m_challenge); When the dialog is cancelled, ::destroy() deletes the object immediately, so you shouldn't use the object after calling destroy. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:875 > + if (challenge != d->m_currentWebChallenge) > + return; I think we should implement AuthenticationChallenge::platformCompare() to make sure we don't unpause a different soup message. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:903 > + if (client()) > + client()->receivedCancellation(this, challenge); > + soup_session_unpause_message(challenge.soupSession(), challenge.soupMessage()); This doesn't work, ResourceLoader::receivedCancellation() cancels the load operation, so when soup_session_unpause_message() is called the message is not in the queue anymore. I think we should not notify the client, and just unpause the message to continue with the load instead of cancelling it as we have always done.

(In reply to comment #14) > (From update of attachment 171528 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171528&action=review > > > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:112 > > - SoupURI* uri = soup_message_get_uri(m_message.get()); > > - GOwnPtr<char>description(g_strdup_printf(_("A username and password are being requested by the site %s"), uri->host)); > > - GtkWidget* descriptionLabel = gtk_label_new(description.get()); > > + String description = String::format(_("A username and password are being requested by the site %s"), > > + m_challenge.protectionSpace().host().utf8().data()); > > + GtkWidget* descriptionLabel = gtk_label_new(description.utf8().data()); > > This is causing encoding issues in the dialog label, I guess it's because String::format expects char* but not necessarily a utf8 encoded char*. I still think previous code was simpler and more efficient. Do you mind elaborating about what encoding issues this is causing?

(In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 171528 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=171528&action=review > > > > > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:112 > > > - SoupURI* uri = soup_message_get_uri(m_message.get()); > > > - GOwnPtr<char>description(g_strdup_printf(_("A username and password are being requested by the site %s"), uri->host)); > > > - GtkWidget* descriptionLabel = gtk_label_new(description.get()); > > > + String description = String::format(_("A username and password are being requested by the site %s"), > > > + m_challenge.protectionSpace().host().utf8().data()); > > > + GtkWidget* descriptionLabel = gtk_label_new(description.utf8().data()); > > > > This is causing encoding issues in the dialog label, I guess it's because String::format expects char* but not necessarily a utf8 encoded char*. I still think previous code was simpler and more efficient. > > Do you mind elaborating about what encoding issues this is causing? With your patch: El sitio foobar.com está solicitando un nombre de usuario y una contraseña With the current code: El sitio foobar.com está solicitando un nombre de usuario y una contraseña

(In reply to comment #14) > (From update of attachment 171528 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171528&action=review > > > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:112 > > - SoupURI* uri = soup_message_get_uri(m_message.get()); > > - GOwnPtr<char>description(g_strdup_printf(_("A username and password are being requested by the site %s"), uri->host)); > > - GtkWidget* descriptionLabel = gtk_label_new(description.get()); > > + String description = String::format(_("A username and password are being requested by the site %s"), > > + m_challenge.protectionSpace().host().utf8().data()); > > + GtkWidget* descriptionLabel = gtk_label_new(description.utf8().data()); > > This is causing encoding issues in the dialog label, I guess it's because String::format expects char* but not necessarily a utf8 encoded char*. I still think previous code was simpler and more efficient. Okay. I'll use GOwnPtr<char*> again. > > > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:266 > > dialog->destroy(); > > + dialog->m_challenge.authenticationClient()->receivedCancellation(dialog->m_challenge); > > When the dialog is cancelled, ::destroy() deletes the object immediately, so you shouldn't use the object after calling destroy. Nice catch! Yeah, that's some particularly stupid code that I wrote there. > > > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:875 > > + if (challenge != d->m_currentWebChallenge) > > + return; > > I think we should implement AuthenticationChallenge::platformCompare() to make sure we don't unpause a different soup message. Okay. > > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:903 > > + if (client()) > > + client()->receivedCancellation(this, challenge); > > + soup_session_unpause_message(challenge.soupSession(), challenge.soupMessage()); > > This doesn't work, ResourceLoader::receivedCancellation() cancels the load operation, so when soup_session_unpause_message() is called the message is not in the queue anymore. I think we should not notify the client, and just unpause the message to continue with the load instead of cancelling it as we have always done. Your suggestion would make receivedCancellation equivalent to receivedRequestToContinueWithoutCredential, I think. Instead I'll reorder these statements to avoid the assertion failure and call receivedRequestToContinueWithoutCredential from GtkAuthenticationDialog to preserve the current behavior.

Created attachment 171866 [details] Patch

(In reply to comment #17) > (In reply to comment #14) > > (From update of attachment 171528 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=171528&action=review > > > > > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:112 > > > - SoupURI* uri = soup_message_get_uri(m_message.get()); > > > - GOwnPtr<char>description(g_strdup_printf(_("A username and password are being requested by the site %s"), uri->host)); > > > - GtkWidget* descriptionLabel = gtk_label_new(description.get()); > > > + String description = String::format(_("A username and password are being requested by the site %s"), > > > + m_challenge.protectionSpace().host().utf8().data()); > > > + GtkWidget* descriptionLabel = gtk_label_new(description.utf8().data()); > > > > This is causing encoding issues in the dialog label, I guess it's because String::format expects char* but not necessarily a utf8 encoded char*. I still think previous code was simpler and more efficient. > > Okay. I'll use GOwnPtr<char*> again. Thanks. > > > > > Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:266 > > > dialog->destroy(); > > > + dialog->m_challenge.authenticationClient()->receivedCancellation(dialog->m_challenge); > > > > When the dialog is cancelled, ::destroy() deletes the object immediately, so you shouldn't use the object after calling destroy. > > Nice catch! Yeah, that's some particularly stupid code that I wrote there. > > > > > > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:875 > > > + if (challenge != d->m_currentWebChallenge) > > > + return; > > > > I think we should implement AuthenticationChallenge::platformCompare() to make sure we don't unpause a different soup message. > > Okay. > > > > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:903 > > > + if (client()) > > > + client()->receivedCancellation(this, challenge); > > > + soup_session_unpause_message(challenge.soupSession(), challenge.soupMessage()); > > > > This doesn't work, ResourceLoader::receivedCancellation() cancels the load operation, so when soup_session_unpause_message() is called the message is not in the queue anymore. I think we should not notify the client, and just unpause the message to continue with the load instead of cancelling it as we have always done. > > Your suggestion would make receivedCancellation equivalent to receivedRequestToContinueWithoutCredential, I think. Instead I'll reorder these statements to avoid the assertion failure and call receivedRequestToContinueWithoutCredential from GtkAuthenticationDialog to preserve the current behavior. Indeed, it makes a lot of sense because we want to continue in the end. I wonder when receivedCancellation is supposed to be called, though. Note that CF backend doesn't call clearAuthentication(); in receivedCancellation.

> Indeed, it makes a lot of sense because we want to continue in the end. I wonder when receivedCancellation is supposed to be called, though. Note that CF backend doesn't call clearAuthentication(); in receivedCancellation. Yeah, I noticed that as well. When I was implementing, I wanted to be a bit more cautious and consistent. It's probably doesn't matter a great deal either way.

Comment on attachment 171866 [details] Patch Perfect! Thanks.

Comment on attachment 171866 [details] Patch Rejecting attachment 171866 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: unk #2 succeeded at 869 with fuzz 2 (offset 10 lines). Hunk #3 succeeded at 1121 with fuzz 2 (offset 56 lines). patching file Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp patching file Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp patching file Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebFrameLoaderClientGtk.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Carlos Gar..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/14692941

Committed r133389: <http://trac.webkit.org/changeset/133389>