Minimize login prompts

Minimize login prompts

LoginHandler can receive useful notifications from the same
NavigationController.  Excluding them can trigger spurious login dialogs
for the same target.

Also, re-order authentication dialog cancellations to prevent unnecessary
dialogs from being shown to the user.

BUG=51570
TEST=LoginPromptBrowserTest.*

Contributed by: asanka@google.com

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70509

[asanka (google), 2010-12-14 19:46:39]

Please review.

[cbentzel, 2010-12-14 19:54:02]

On 2010/12/14 19:46:39, asanka wrote:
> Please review.

Please add a login_prompt_uitest for this case.

I expect the uitest to be a fair amount more work than the actual fix, but it
would be nice to minimize regressions going into the future. 

There are a number of tests which already exist, but it may be tricky to rig one
up for this case [something like a main page which has two image subresources on
the same authenticated server].

[asanka (google), 2010-12-17 16:34:22]

Added new browsertest.

[cbentzel, 2010-12-17 19:22:56]

tonyg: I added you for the LoginPrompt change.
phajdan.jr: I added you for test-fu, mainly to make sure this doesn't seem like
it will timeout.

Asanka - please don't be scared by the number of comments. For your first "real"
patch to Chrome, this is impressive, both to get the tests working correctly and
that you stayed pretty true to the Chrome style.

It definitely seems like using browser tests instead of redoing the automation
framework to handle these cases worked well.

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
File chrome/browser/ui/login/login_prompt.cc (right):

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt.cc:200: if (WasAuthHandled(false))
I know the code already existed like this, but I'm wondering if you should
change this class to have

bool WasAuthHandled() const;
bool TestAndSetAuthHandled();

Passing in the boolean makes it pretty unclear what is going on.

It's fine to defer this cleanup to another CL.

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
File chrome/browser/ui/login/login_prompt.h (right):

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt.h:74: net::AuthChallengeInfo* auth_info()
const { return auth_info_.get(); }
I know that you just moved this from protected to public, but this should
probably not be a const method if it returns a non-const AuthChallengeInfo*
which it uses internally [the assumption being that mutations can be done on it
from outside].

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:8: #include
"net/base/auth.h"
Includes need to be alphabetically ordered.

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:21:
LoginPromptBrowserTest():
Nit: the first initializer list argument should be on the same line as the ":". 

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:30: 
Nit: extra newline

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:47: void
SetAuthFor(LoginHandler* handler);
Nit: SetAuthFor needs to be declared before the member variables.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:51: const char*
kSingleRealmTestPage = "files/login/single_realm.html";
Might make sense to move these right above the TEST_F's below, since that's the
first time they are used.

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:55: std::map<std::wstring,
AuthInfo>::iterator i;
Don't use default constructor for i, assign with
auth_map_.find(challenge->realm) from below.

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:61: AuthInfo& info =
i->second;
const AuthInfo

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:62: 
Nit: extra newline

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:71: 
Nit: extra new line

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:72:
LoginPromptBrowserTestObserver():
Nit: first initializer list argument on same line as :

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:73: n_auth_needed(0),
n_auth_supplied(0), n_auth_cancelled(0) {}
Use auth_needed_count rather than n_auth_needed [etc].

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:79: void
AddSelf(NotificationRegistrar* registrar,
Register and Unregister may be clearer than AddSelf/RemoveSelf.

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:98: 
Nit: extra new line

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:99:
LoginNotificationDetails* login_details =
The Details cast here is typically done after checking the type - since it's
effectively a reinterpret cast. 

I recommend rewriting as 

switch (type) {
  case NotificationType::AUTH_NEEDED:
    // do the cast and other things.
    break;
  case NotificationType::AUTH_SUPPLIED:
  case NotificationType::AUTH_CANCELLED:
    // Do the other stuff
    break;
  default:
    ADD_FAILURE();
    break;
  }
}

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:108: std::set<LoginHandler
*>::iterator i;
Nit: do the handlers_.find on the same line as i declaration.

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:159: // Test handling of
resource that require authentication even though
Nit: s/resource/resources

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:170: ASSERT_TRUE(contents !=
NULL);
ASSERT_TRUE(contents) is sufficient.

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:177:
WindowedLoadStopObserver load_stop_waiter(controller);
Does the OpenURL trigger LOAD_STOP on the first auth prompt? When does that
notification event get sent?

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:179: {
It doesn't seem like scoping the WindowedAuthNeededObserver is necessary here
[although it isn't incorrect] - for example, I don't think there will be an
AUTH_NEEDED if before this scope.

I would just keep it in the top level function scope so future readers don't
have to wonder why.

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:187: while
(observer.n_auth_cancelled < 4) {
You should make 4 a constant so it's clear what it represent.

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:192: LoginHandler * handler
= *observer.handlers_.begin();
Nit: LoginHandler* handler

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:194: ASSERT_TRUE(handler !=
NULL);
Nit: ASSERT_TRUE(handler);

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:206: EXPECT_EQ(2,
n_handlers);
Add a constant for 2 and indicate that there are only two realms [essentially,
why this is 2 compared to the 4 for number of auth_needed/auth_cancelled].

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:211:
observer.RemoveSelf(&registrar_, Source<NavigationController>(controller));
Should this be done in the destructor for LoginPromptBrowserTestObserver?

http://codereview.chromium.org/5814005/diff/7001/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/5814005/diff/7001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:915: set_cookie_if_challenged =
self.path.find('?set-cookie-if-challenged') > 0
Could you shift this to urlparse and cgi.parse_qs rather than doing regexps
directly? That would make this clearer, and probably work on some weird corner
cases.

http://codereview.chromium.org/5814005/diff/7001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:959: self.send_header('Content-type',
'text/html')
It seems a bit weird to return text/html when your test uses a.gif,b.gif, etc.

[asanka (google), 2010-12-17 21:35:35]

Coding convention fixes for the browsertest and update testserver.py to use
urlparse.

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
File chrome/browser/ui/login/login_prompt.cc (right):

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt.cc:200: if (WasAuthHandled(false))
I'll defer this for another CL.  I agree with the comment.

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
File chrome/browser/ui/login/login_prompt.h (right):

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt.h:74: net::AuthChallengeInfo* auth_info()
const { return auth_info_.get(); }
On 2010/12/17 19:22:56, cbentzel wrote:
> I know that you just moved this from protected to public, but this should
> probably not be a const method if it returns a non-const AuthChallengeInfo*
> which it uses internally [the assumption being that mutations can be done on
it
> from outside]. 

I'll defer this to another CL since there are other consumers of auth_info().

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:177:
WindowedLoadStopObserver load_stop_waiter(controller);
LOAD_STOP is received after the page is done loading (after all the login
prompts have been resolved).  We are waiting on LOAD_STOP so that we can have a
clean teardown after the test run.

http://codereview.chromium.org/5814005/diff/7001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:179: {
On 2010/12/17 19:22:56, cbentzel wrote:
> It doesn't seem like scoping the WindowedAuthNeededObserver is necessary here
> [although it isn't incorrect] - for example, I don't think there will be an
> AUTH_NEEDED if before this scope.

I expect the behavior to be identical if the WindowedAuthNeededObserver was at
the top level.  But I scoped it here so that the next auth_needed_waiter a few
lines down doesn't shadow it.

[cbentzel, 2010-12-17 22:02:55]

LGTM - with the exception of the testserver.py bug.

Since I'm on vacation next week, you should try to find someone else in our
office to run the trybots for you and land your change. 

Please give Pawel and/or Tony a day or two to respond - they may have some
additional insights.

http://codereview.chromium.org/5814005/diff/14001/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/5814005/diff/14001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:6: #include <map>
Nit: <map> before <set>

http://codereview.chromium.org/5814005/diff/14001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:17: class
LoginPromptBrowserTest : public InProcessBrowserTest {
Can these go in an anonymous namespace?

http://codereview.chromium.org/5814005/diff/14001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:64: : auth_needed_count_(0),
auth_supplied_count_(0),
Nit: probably move auth_supplied_count_ right under auth_needed_count_

http://codereview.chromium.org/5814005/diff/14001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:139:
WindowedLoadStopObserver;
Nit: I think it would be clearer if you indented this in 4 chars, even if emacs
or other auto-formatters don't.

http://codereview.chromium.org/5814005/diff/14001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/5814005/diff/14001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:919: for (name, value) in
urlparse.parse_qsl(query, True):
Use cgi.parse_qs instead of urlparse.parse_qs [and you have a typo which must
have slipped in].

urlparse.parse_qs was only introduced in python 2.6 [I think] and many of the
scripts including this one still expect 2.4

http://codereview.chromium.org/5814005/diff/14001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:964: self.send_header('Content-type',
'text/html')
Ah, I remember the missing comment. I thought it was a bad idea to set
Content-Type: text/html on the image/gif resources you recently added in the
multiauth/singleauth test pages.

[asanka (google), 2010-12-20 17:44:08]

Changed testserver to write out a .gif file for the placeholder test images.

http://codereview.chromium.org/5814005/diff/14001/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/5814005/diff/14001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:6: #include <map>
On 2010/12/17 22:02:55, cbentzel wrote:
> Nit: <map> before <set>

Done.

http://codereview.chromium.org/5814005/diff/14001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:17: class
LoginPromptBrowserTest : public InProcessBrowserTest {
On 2010/12/17 22:02:55, cbentzel wrote:
> Can these go in an anonymous namespace? 

Done.

http://codereview.chromium.org/5814005/diff/14001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:64: : auth_needed_count_(0),
auth_supplied_count_(0),
On 2010/12/17 22:02:55, cbentzel wrote:
> Nit: probably move auth_supplied_count_ right under auth_needed_count_

Done.

http://codereview.chromium.org/5814005/diff/14001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:139:
WindowedLoadStopObserver;
On 2010/12/17 22:02:55, cbentzel wrote:
> Nit: I think it would be clearer if you indented this in 4 chars, even if
emacs
> or other auto-formatters don't.

Done.

http://codereview.chromium.org/5814005/diff/14001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/5814005/diff/14001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:919: for (name, value) in
urlparse.parse_qsl(query, True):
On 2010/12/17 22:02:55, cbentzel wrote:
> Use cgi.parse_qs instead of urlparse.parse_qs [and you have a typo which must
> have slipped in].

parse_qsl() returns the query parameters as a list.  I replaced it with
cgi.parse_qs().

> 
> urlparse.parse_qs was only introduced in python 2.6 [I think] and many of the
> scripts including this one still expect 2.4

http://codereview.chromium.org/5814005/diff/14001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:964: self.send_header('Content-type',
'text/html')
On 2010/12/17 22:02:55, cbentzel wrote:
> Ah, I remember the missing comment. I thought it was a bad idea to set
> Content-Type: text/html on the image/gif resources you recently added in the
> multiauth/singleauth test pages.

Done.

[asanka (google), 2010-12-20 20:21:37]

Adding ahendrickson.

[cbentzel, 2010-12-21 02:53:53]

LGTM

[ahendrickson, 2010-12-21 16:40:38]

LGTM

[ahendrickson, 2010-12-21 17:46:23]

Working on committing this CL for Asanka.

[asanka (google), 2010-12-23 21:48:09]

This last set of changes were made to address a race that caused unnecessary
dialogs to be shown on Macs.

The root cause is that we are creating multiple dialogs, even if they are for
the same target and then dismissing them when we have a single response.  The
order of the dismissal matters in this case.  If there are multiple dialogs and
we dismiss the first one, the next dialog will be shown to the user before
control is returned to Chrome.  This is very apparent on Mac due to transition
animations.  It is also very time consuming.

Arguably, the ultimate fix would be to rework how login prompting works so that
we only have one set of prompts per target and we don't issue any more requests
than necessary until we have confirmed credentials (see http://crbug.com/19298
).

[cbentzel, 2011-01-04 16:09:54]

http://codereview.chromium.org/5814005/diff/34001/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt.cc (right):

http://codereview.chromium.org/5814005/diff/34001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt.cc:135: if
(BrowserThread::CurrentlyOn(BrowserThread::UI))
I think this is always called from the UI thread. It appears to only be called
by LoginHandler::Observe, which in turn checks that it's on the UI thread.

That being said, I'm glad that you added this comment but ensuring that
CloseContentsDeferred are triggered in reverse order [or at least prior to the
foreground LoginHandler] seems very fragile.

[cbentzel, 2011-01-04 19:46:49]

Also, this is an LGTM provided you change SetAuth/CancelAuth to expect to be
called on the UI thread.

[asanka (google), 2011-01-04 22:34:49]

Changed SetAuth() to expect being called from the UI thread and updated the
description.

http://codereview.chromium.org/5814005/diff/34001/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt.cc (right):

http://codereview.chromium.org/5814005/diff/34001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt.cc:135: if
(BrowserThread::CurrentlyOn(BrowserThread::UI))
On 2011/01/04 16:09:54, cbentzel wrote:
> I think this is always called from the UI thread. It appears to only be called
> by LoginHandler::Observe, which in turn checks that it's on the UI thread.

Fixed.  I didn't change the CancelAuth() case since it can be called from
OnRequestCancelled(), which is on the IO thread.

> That being said, I'm glad that you added this comment but ensuring that
> CloseContentsDeferred are triggered in reverse order [or at least prior to the
> foreground LoginHandler] seems very fragile.

[cbentzel, 2011-01-05 13:13:51]

LGTM

Let me know when you make these small tweaks and I will patch and run on try
bots.

http://codereview.chromium.org/5814005/diff/39001/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt.cc (right):

http://codereview.chromium.org/5814005/diff/39001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt.cc:155: else
Nit: use braces {} for multi-line else clauses [and you'll want it for the if
portion as well].

http://codereview.chromium.org/5814005/diff/39001/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt.h (right):

http://codereview.chromium.org/5814005/diff/39001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt.h:74: net::AuthChallengeInfo* auth_info()
const { return auth_info_.get(); }
This should return a const net::AuthChallengeInfo* since it's labeled const -
otherwise the underlying data could be mutated.

http://codereview.chromium.org/5814005/diff/39001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt.h:77: bool WasAuthHandled();
This should probably be const.

[asanka (google), 2011-01-05 14:29:11]

Added const for getters and added braces for multi-line if-else.

http://codereview.chromium.org/5814005/diff/39001/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt.cc (right):

http://codereview.chromium.org/5814005/diff/39001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt.cc:155: else
On 2011/01/05 13:13:52, cbentzel wrote:
> Nit: use braces {} for multi-line else clauses [and you'll want it for the if
> portion as well]. 

Done.

http://codereview.chromium.org/5814005/diff/39001/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt.h (right):

http://codereview.chromium.org/5814005/diff/39001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt.h:74: net::AuthChallengeInfo* auth_info()
const { return auth_info_.get(); }
On 2011/01/05 13:13:52, cbentzel wrote:
> This should return a const net::AuthChallengeInfo* since it's labeled const -
> otherwise the underlying data could be mutated. 

Done.

http://codereview.chromium.org/5814005/diff/39001/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt.h:77: bool WasAuthHandled();
On 2011/01/05 13:13:52, cbentzel wrote:
> This should probably be const.

Done.

[cbentzel, 2011-01-05 14:32:04]

LGTM.

I'll take care of landing this.