Don't focus the location bar in a phishy situation.

chrome/browser/ui/browser_focus_uitest.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 <stddef.h>
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/format_macros.h"
 #include "base/macros.h"
@@ skipping 720 matching lines @@
     content::WindowedNotificationObserver forward_nav_observer(
         content::NOTIFICATION_NAV_ENTRY_COMMITTED,
         content::NotificationService::AllSources());
     chrome::GoForward(browser(), CURRENT_TAB);
     forward_nav_observer.Wait();
   }
 
   EXPECT_FALSE(IsViewFocused(VIEW_ID_OMNIBOX));
 }
 
+// Ensure that crbug.com/567445 does not regress.

[Peter Kasting, 2016/03/03 21:02:18]

Nit: Explain here what this is testing, e.g. "Ensure the omnibox does not get
focused when loading about:blank in a case where it's not the startup URL, e.g.
when a page opens a popup to about:blank and then navigates it.  This is a
potential security issue; see comments in
WebContentsImpl::FocusLocationBarByDefault()."

[palmer, 2016/03/04 22:15:56]

Done.

+IN_PROC_BROWSER_TEST_F(BrowserFocusTest, AboutBlankNavigationLocationTest) {
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/title1.html"));

[Peter Kasting, 2016/03/03 21:02:18]

Nit: Store URL in a temp since you use it twice:

  const std::string url1(embedded_test_server()->GetURL("/title1.html"));
  ui_test_utils::NavigateToURL(browser(), url1);
  ...
  EXPECT_EQ(url1, web_contents->GetVisibleURL());

[palmer, 2016/03/04 22:15:56]

Done.

+
+  const char kSpoofScript[] =
+      "var w = window.open('about:blank');"

[Peter Kasting, 2016/03/03 21:02:18]

Nit: Could go on previous line (and next two lines get indented even)

But even better would be this, which is shorter and simpler overall:

  const std::string url2(embedded_test_server()->GetURL("/title2.html").spec());
  const std::string spoof("var w = window.open('about:blank'); w.opener = null;"
                          "w.document.location = '" + url2 + "';");
  ...
  ASSERT_TRUE(content::ExecuteScript(web_contents, spoof));

(|url2| here is so named to parallel the suggestion above)

[palmer, 2016/03/04 22:15:56]

Done.

+      "w.opener = null;"
+      "w.document.location = '%s';";
+
+  TabStripModel* tab_strip = browser()->tab_strip_model();
+  WebContents* web_contents = tab_strip->GetActiveWebContents();
+
+  ASSERT_TRUE(content::ExecuteScript(
+      web_contents,
+      base::StringPrintf(
+          kSpoofScript,
+          embedded_test_server()->GetURL("/title2.html").spec().c_str())));
+  EXPECT_EQ(embedded_test_server()->GetURL("/title1.html"),
+            web_contents->GetVisibleURL());
+  ASSERT_NO_FATAL_FAILURE(content::WaitForLoadStop(
+      browser()->tab_strip_model()->GetActiveWebContents()));

[Peter Kasting, 2016/03/03 21:02:18]

Is this different than |web_contents|?  If so I'd add a comment about why, since
this seems subtle.

[palmer, 2016/03/04 22:15:56]

Yes, they are different; it's the newly-opened contents that has the
phishy-looking URL. I added a comment.

+  EXPECT_FALSE(IsViewFocused(VIEW_ID_OMNIBOX));

[Charlie Reis, 2016/02/25 00:48:40]

Sure, these lines look plausible to me, assuming the popup opens in the same
window.  (That's probably enough to repro the bug.)

[palmer, 2016/03/04 22:15:56]

Acknowledged.

+}
+
 }  // namespace