Fix cross origin check when deciding to show the HTTP auth interstitial.

chrome/browser/ui/login/login_prompt.cc

 // Copyright (c) 2012 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 "base/bind.h"
 #include "base/command_line.h"
@@ skipping 492 matching lines @@
   WebContents* parent_contents = handler->GetWebContentsForLogin();
   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;
   }
 
   if (is_main_frame &&
-      parent_contents->GetVisibleURL().GetOrigin() != request_url.GetOrigin()) {
+      (parent_contents->GetVisibleURL().GetOrigin() !=
+          request_url.GetOrigin() ||
+       parent_contents->GetLastCommittedURL().GetOrigin() !=
+          request_url.GetOrigin())) {

[Peter Kasting, 2014/08/05 20:18:10]

It seems a bit weird that we check both of these.

If the two are the same, we should only need to check one.  If the two aren't
the same, then it seems like one must be the one we want to check, and the other
not.  In either case, I'd think we'd only ever want to check one.

Clearly always checking just the visible URL isn't correct or you wouldn't be
making a change.  So does always checking the last committed URL work?  If not,
why not, and how can we write the code so that we know when we should check one
and when the other?

[meacer, 2014/08/05 22:29:19]

The two different cases are:

1- The user navigates directly by entering a url in the omnibox. In this case:

resource_url!=GetLastCommittedURL and resource_url==GetVisibleURL

because the navigation hasn't committed yet (which is the oddity of requests
with HTTP Auth headers).

2- The page redirects to a cross origin resource. In this case:

resource_url!=GetLastCommitedURL and resource_url!=GetVisibleURL

So you are right, it looks like we don't need to check for GetVisibleURL. I'm
trying to think of a situation where resource_url isn't equal to
GetLastCommittedURL but is is equal to GetVisibleURL, but I can't find any.

[Peter Kasting, 2014/08/05 23:15:34]

I would probably change to just checking against GetLastCommittedURL() then. 
You might want to take the comment that's inside the conditional here and hoist
it above, and expand it to note how it catches both scenarios you describe.

[meacer, 2014/08/05 23:32:23]

Done.

     // Show a blank interstitial for main-frame, cross origin requests
     // so that the correct URL is shown in the omnibox.
     base::Closure callback = base::Bind(&ShowLoginPrompt,
                                         request_url,
                                         make_scoped_refptr(auth_info),
                                         make_scoped_refptr(handler));
     // This is owned by the interstitial it creates.
     new LoginInterstitialDelegate(parent_contents,
                                   request_url,
                                   callback);
@@ skipping 11 matching lines @@
                                 net::URLRequest* request) {
   bool is_main_frame = (request->load_flags() & net::LOAD_MAIN_FRAME) != 0;
   LoginHandler* handler = LoginHandler::Create(auth_info, request);
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&LoginDialogCallback, request->url(),
                  make_scoped_refptr(auth_info), make_scoped_refptr(handler),
                  is_main_frame));
   return handler;
 }