Minimize login prompts

chrome/browser/ui/login/login_prompt.cc

 // Copyright (c) 2009 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/login/login_prompt.h"
 
 #include <vector>
 
 #include "app/l10n_util.h"
 #include "base/command_line.h"
@@ skipping 98 matching lines @@
 
 TabContents* LoginHandler::GetTabContentsForLogin() const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   return tab_util::GetTabContentsByID(render_process_host_id_,
                                       tab_contents_id_);
 }
 
 void LoginHandler::SetAuth(const std::wstring& username,
                            const std::wstring& password) {
-  if (WasAuthHandled(true))
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (TestAndSetAuthHandled())
     return;
 
   // Tell the password manager the credentials were submitted / accepted.
   if (password_manager_) {
     password_form_.username_value = WideToUTF16Hack(username);
     password_form_.password_value = WideToUTF16Hack(password);
     password_manager_->ProvisionallySavePassword(password_form_);
   }
 
+  // Calling NotifyAuthSupplied() directly instead of posting a task
+  // allows other LoginHandler instances to queue their
+  // CloseContentsDeferred() before ours.  Closing dialogs in the
+  // opposite order as they were created avoids races where remaining
+  // dialogs in the same tab may be briefly displayed to the user
+  // before they are removed.
+  NotifyAuthSupplied(username, password);
+
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(this, &LoginHandler::CloseContentsDeferred));
   BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      NewRunnableMethod(
-          this, &LoginHandler::NotifyAuthSupplied, username, password));
-  BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       NewRunnableMethod(
           this, &LoginHandler::SetAuthDeferred, username, password));
 }
 
 void LoginHandler::CancelAuth() {
-  if (WasAuthHandled(true))
+  if (TestAndSetAuthHandled())
     return;
 
+  // Similar to how we deal with notifications above in SetAuth()
+  if (BrowserThread::CurrentlyOn(BrowserThread::UI))
+    NotifyAuthCancelled();
+  else

[cbentzel, 2011/01/05 13:13:52]

Nit: use braces {} for multi-line else clauses [and you'll want it for the if
portion as well]. 

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

Done.

+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        NewRunnableMethod(this, &LoginHandler::NotifyAuthCancelled));
+
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(this, &LoginHandler::CloseContentsDeferred));
   BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      NewRunnableMethod(this, &LoginHandler::NotifyAuthCancelled));
-  BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       NewRunnableMethod(this, &LoginHandler::CancelAuthDeferred));
 }
 
 void LoginHandler::OnRequestCancelled() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)) <<
       "Why is OnRequestCancelled called from the UI thread?";
 
   // Reference is no longer valid.
   request_ = NULL;
@@ skipping 26 matching lines @@
                            const NotificationSource& source,
                            const NotificationDetails& details) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(type == NotificationType::AUTH_SUPPLIED ||
          type == NotificationType::AUTH_CANCELLED);
 
   TabContents* requesting_contents = GetTabContentsForLogin();
   if (!requesting_contents)
     return;
 
-  NavigationController* this_controller = &requesting_contents->controller();
-  NavigationController* that_controller =
-      Source<NavigationController>(source).ptr();
-
-  // Only handle notifications from other handlers.
-  if (this_controller == that_controller)
+  // Break out early if we aren't interested in the notification.
+  if (WasAuthHandled())
     return;
 
   LoginNotificationDetails* login_details =
       Details<LoginNotificationDetails>(details).ptr();
 
+  // WasAuthHandled() should always test positive before we publish
+  // AUTH_SUPPLIED or AUTH_CANCELLED notifications.
+  DCHECK(login_details->handler() != this);
+
   // Only handle notification for the identical auth info.
   if (*login_details->handler()->auth_info() != *auth_info())
     return;
 
   // Set or cancel the auth in this handler.
   if (type == NotificationType::AUTH_SUPPLIED) {
     AuthSuppliedLoginNotificationDetails* supplied_details =
         Details<AuthSuppliedLoginNotificationDetails>(details).ptr();
     SetAuth(supplied_details->username(), supplied_details->password());
   } else {
     DCHECK(type == NotificationType::AUTH_CANCELLED);
     CancelAuth();
   }
 }
 
 void LoginHandler::SetModel(LoginModel* model) {
   if (login_model_)
     login_model_->SetObserver(NULL);
   login_model_ = model;
   if (login_model_)
     login_model_->SetObserver(this);
 }
 
 void LoginHandler::SetDialog(ConstrainedWindow* dialog) {
   dialog_ = dialog;
 }
 
 void LoginHandler::NotifyAuthNeeded() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (WasAuthHandled(false))
+  if (WasAuthHandled())
     return;
 
   TabContents* requesting_contents = GetTabContentsForLogin();
   if (!requesting_contents)
     return;
 
   NotificationService* service = NotificationService::current();
   NavigationController* controller = &requesting_contents->controller();
   LoginNotificationDetails details(this);
 
   service->Notify(NotificationType::AUTH_NEEDED,
                   Source<NavigationController>(controller),
                   Details<LoginNotificationDetails>(&details));
 }
 
 void LoginHandler::NotifyAuthCancelled() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(WasAuthHandled(false));
+  DCHECK(WasAuthHandled());
 
   TabContents* requesting_contents = GetTabContentsForLogin();
   if (!requesting_contents)
     return;
 
   NotificationService* service = NotificationService::current();
   NavigationController* controller = &requesting_contents->controller();
   LoginNotificationDetails details(this);
 
   service->Notify(NotificationType::AUTH_CANCELLED,
                   Source<NavigationController>(controller),
                   Details<LoginNotificationDetails>(&details));
 }
 
 void LoginHandler::NotifyAuthSupplied(const std::wstring& username,
                                       const std::wstring& password) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(WasAuthHandled(false));
+  DCHECK(WasAuthHandled());
 
   TabContents* requesting_contents = GetTabContentsForLogin();
   if (!requesting_contents)
     return;
 
   NotificationService* service = NotificationService::current();
   NavigationController* controller = &requesting_contents->controller();
   AuthSuppliedLoginNotificationDetails details(this, username, password);
 
   service->Notify(NotificationType::AUTH_SUPPLIED,
                   Source<NavigationController>(controller),
                   Details<AuthSuppliedLoginNotificationDetails>(&details));
 }
 
 void LoginHandler::ReleaseSoon() {
-  if (!WasAuthHandled(true)) {
+  if (!TestAndSetAuthHandled()) {
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         NewRunnableMethod(this, &LoginHandler::CancelAuthDeferred));
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
         NewRunnableMethod(this, &LoginHandler::NotifyAuthCancelled));
   }
 
   BrowserThread::PostTask(
     BrowserThread::UI, FROM_HERE,
     NewRunnableMethod(this, &LoginHandler::RemoveObservers));
 
   // Delete this object once all InvokeLaters have been called.
   BrowserThread::ReleaseSoon(BrowserThread::IO, FROM_HERE, this);
 }
 
 // Returns whether authentication had been handled (SetAuth or CancelAuth).
-// If |set_handled| is true, it will mark authentication as handled.
-bool LoginHandler::WasAuthHandled(bool set_handled) {
+bool LoginHandler::WasAuthHandled() {
   AutoLock lock(handled_auth_lock_);
   bool was_handled = handled_auth_;
-  if (set_handled)
-    handled_auth_ = true;
   return was_handled;
 }
 
+// Marks authentication as handled and returns the previous handled state.
+bool LoginHandler::TestAndSetAuthHandled() {
+  AutoLock lock(handled_auth_lock_);
+  bool was_handled = handled_auth_;
+  handled_auth_ = true;
+  return was_handled;
+}
+
 // Calls SetAuth from the IO loop.
 void LoginHandler::SetAuthDeferred(const std::wstring& username,
                                    const std::wstring& password) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   if (request_) {
     request_->SetAuth(WideToUTF16Hack(username), WideToUTF16Hack(password));
     ResetLoginHandlerForRequest(request_);
   }
 }
@@ skipping 30 matching lines @@
   LoginDialogTask(const GURL& request_url,
                   net::AuthChallengeInfo* auth_info,
                   LoginHandler* handler)
       : request_url_(request_url), auth_info_(auth_info), handler_(handler) {
   }
   virtual ~LoginDialogTask() {
   }
 
   void Run() {
     TabContents* parent_contents = handler_->GetTabContentsForLogin();
-    if (!parent_contents) {
+    if (!parent_contents || handler_->WasAuthHandled()) {
       // The request may have been cancelled, or it may be for a renderer
       // not hosted by a tab (e.g. an extension). Cancel just in case
       // (cancelling twice is a no-op).
       handler_->CancelAuth();
       return;
     }
 
     // Tell the password manager to look for saved passwords.
     TabContentsWrapper** wrapper =
         TabContentsWrapper::property_accessor()->GetProperty(
@@ skipping 67 matching lines @@
 // Public API
 
 LoginHandler* CreateLoginPrompt(net::AuthChallengeInfo* auth_info,
                                 net::URLRequest* request) {
   LoginHandler* handler = LoginHandler::Create(auth_info, request);
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE, new LoginDialogTask(
           request->url(), auth_info, handler));
   return handler;
 }