Allow top-level navigation in extension pop-ups if it only triggers a download.

chrome/browser/extensions/api/extension_action/browser_action_apitest.cc

Index: chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
diff --git a/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc b/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
index a8f61c9e8e30fa7545fcc3c368392fdcb7fdc905..99f3aec4527dfc91bd037b0014c489819b2ef67c 100644
--- a/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
+++ b/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
@@ -24,10 +24,14 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/content_browser_test_utils.h"
+#include "content/public/test/download_test_observer.h"
+#include "content/public/test/test_utils.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/notification_types.h"
@@ -38,6 +42,7 @@
 #include "extensions/test/result_catcher.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
@@ -105,7 +110,7 @@ class BrowserActionApiTest : public ExtensionApiTest {
     return browser_action_test_util_.get();
   }
 
-  bool OpenPopup(int index) {
+  WebContents* OpenPopup(int index) {
     ResultCatcher catcher;
     content::WindowedNotificationObserver popup_observer(
         content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
@@ -113,7 +118,13 @@ class BrowserActionApiTest : public ExtensionApiTest {
     GetBrowserActionsBar()->Press(index);
     popup_observer.Wait();
     EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
-    return GetBrowserActionsBar()->HasPopup();
+
+    if (!GetBrowserActionsBar()->HasPopup())
+      return nullptr;
+
+    const auto& source = static_cast<const content::Source<WebContents>&>(
+        popup_observer.source());
+    return source.ptr();
   }
 
   ExtensionAction* GetBrowserAction(const Extension& extension) {
@@ -794,5 +805,219 @@ IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, BrowserActionOpenPopupOnPopup) {
   EXPECT_TRUE(catcher.GetNextResult()) << message_;
 }
 
+class NavigatingExtensionPopupBrowserTest : public BrowserActionApiTest {
+ public:
+  const Extension& popup_extension() { return *popup_extension_; }
+  const Extension& other_extension() { return *other_extension_; }
+
+  void SetUpOnMainThread() override {
+    BrowserActionApiTest::SetUpOnMainThread();
+
+    host_resolver()->AddRule("*", "127.0.0.1");
+    ASSERT_TRUE(embedded_test_server()->Start());
+
+    // Load an extension with a pop-up.
+    ASSERT_TRUE(popup_extension_ = LoadExtension(test_data_dir_.AppendASCII(
+                    "browser_action/popup_with_form")));
+
+    // Load another extension (that we can try navigating to).
+    ASSERT_TRUE(other_extension_ = LoadExtension(test_data_dir_.AppendASCII(
+                    "browser_action/popup_with_iframe")));
+  }
+
+  void VerifyNavigationResult(WebContents* popup,

[Devlin, 2016/09/26 20:57:05]

Could this be private?

[Łukasz Anforowicz, 2016/09/26 22:05:11]

Actually, after addressing Charlie's feedback there is only one called of
VerifyNavigationResult, so I've decided to just inline this method.

+                              GURL old_popup_url,
+                              GURL target_url,
+                              bool expecting_navigation_success) {
+    if (!popup) {
+      // If navigation ends up in a tab, then the tab will be focused and
+      // therefore the popup will be closed, destroying associated WebContents -
+      // don't do any verification in this case.
+      ADD_FAILURE() << "Navigation should not close extension pop-up";
+    } else {
+      // If the extension popup is still opened, then wait until there is no
+      // load in progress, and verify whether the navigation succeeded or not.
+      LOG(INFO) << "Waiting until popup navigation stops...";

[Devlin, 2016/09/26 20:57:05]

nit: remove debugging logs

[Łukasz Anforowicz, 2016/09/26 22:05:11]

Done.

+      WaitForLoadStop(popup);
+      if (expecting_navigation_success) {
+        EXPECT_EQ(target_url, popup->GetLastCommittedURL())
+            << "Navigation to " << target_url.spec()
+            << " should succeed in an extension pop-up";
+      } else {
+        EXPECT_NE(target_url, popup->GetLastCommittedURL())
+            << "Navigation to " << target_url.spec()

[Devlin, 2016/09/26 20:57:05]

I think you can avoid the .spec() calls here - log streams can handle printing
GURLs.

[Łukasz Anforowicz, 2016/09/26 22:05:11]

Done.

+            << " should fail in an extension pop-up";
+        EXPECT_THAT(
+            popup->GetLastCommittedURL(),
+            ::testing::AnyOf(::testing::Eq(old_popup_url),
+                             ::testing::Eq(GURL("chrome-extension://invalid")),
+                             ::testing::Eq(GURL("about:blank"))));
+      }
+    }
+
+    // Make sure that the web navigation did not succeed somewhere outside of
+    // the extension popup (as it might if ExtensionViewHost::OpenURLFromTab
+    // forwards the navigation to Browser::OpenURL [which doesn't specify a
+    // source WebContents]).
+    LOG(INFO) << "Verifying contents of tabs...";
+    TabStripModel* tabs = browser()->tab_strip_model();
+    for (int i = 0; i < tabs->count(); i++) {
+      WebContents* tab_contents = tabs->GetWebContentsAt(i);
+      WaitForLoadStop(tab_contents);
+      EXPECT_NE(target_url, tab_contents->GetLastCommittedURL())
+          << "Navigating an extension pop-up should not affect tabs.";
+    }
+  }
+
+  void TestPopupNavigationViaGet(GURL target_url,

[Devlin, 2016/09/26 20:57:05]

const GURL&s

[Łukasz Anforowicz, 2016/09/26 22:05:11]

Done.

+                                 bool expecting_navigation_success) {
+    std::string navigation_starting_script =
+        "window.location = '" + target_url.spec() + "';";
+    TestPopupNavigation(target_url, expecting_navigation_success,
+                        navigation_starting_script);
+  }
+
+  void TestPopupNavigationViaPost(GURL target_url,
+                                  bool expecting_navigation_success) {
+    std::string navigation_starting_script =
+        "var form = document.getElementById('form');"
+        "form.action = '" + target_url.spec() + "';"
+        "form.submit();";
+    TestPopupNavigation(target_url, expecting_navigation_success,
+                        navigation_starting_script);
+  }
+
+ private:
+  void TestPopupNavigation(
+      GURL target_url,
+      bool expecting_navigation_success,
+      std::string navigation_starting_script) {
+    // Were there any failures so far (e.g. in SetUpOnMainThread)?
+    ASSERT_FALSE(HasFailure());
+
+    // Simulate a click on the browser action to open the popup.
+    WebContents* popup = OpenPopup(0);
+    ASSERT_TRUE(popup);
+    GURL popup_url = popup_extension().GetResourceURL("popup.html");
+    EXPECT_EQ(popup_url, popup->GetLastCommittedURL());
+
+    // Note that the |setTimeout| call below is needed to make sure
+    // ExecuteScriptAndExtractBool returns *after* a scheduled navigation has
+    // already started.
+    std::string script_to_execute =
+        navigation_starting_script +
+        "setTimeout("

[Devlin, 2016/09/26 20:57:05]

\n

[Łukasz Anforowicz, 2016/09/26 22:05:11]

Done.

+        "  function(){ window.domAutomationController.send(true); },"

[Devlin, 2016/09/26 20:57:05]

space after function()
\n

[Łukasz Anforowicz, 2016/09/26 22:05:11]

Done.

+        "  0);";

[Devlin, 2016/09/26 20:57:05]

no indentation

[Łukasz Anforowicz, 2016/09/26 22:05:11]

I don't understand.  The arguments of setTimeout should be aligned, so that they
begin in the same column right?  In other words, "0" has to be in the same
column as "f" from "function", right?

[Devlin, 2016/09/27 15:57:28]

Whoops, misread this for some reason.  The 0 should line with the 'f', but both
should be indented 4, rather than 2.  (Also for the process_manager_browsertest
one.)

[Łukasz Anforowicz, 2016/09/27 17:00:50]

Got it - done.

+
+    // Try to navigate the pop-up.
+    bool ignored;

[Devlin, 2016/09/26 20:57:05]

init ignored

[Łukasz Anforowicz, 2016/09/26 22:05:11]

Done (+ renamed the variable to be more descriptive).

+    content::WebContentsDestroyedWatcher popup_destruction_watcher(popup);
+    LOG(INFO) << "Navigating extension popup to " << target_url;
+    EXPECT_TRUE(ExecuteScriptAndExtractBool(popup, script_to_execute,
+                                            &ignored));
+
+    // Verify whether navigation succeeded or failed.
+    VerifyNavigationResult(popup_destruction_watcher.web_contents(), popup_url,
+                           target_url, expecting_navigation_success);
+
+    // Close the pop-up.
+    BrowserActionTestUtil* actions_bar = GetBrowserActionsBar();
+    EXPECT_TRUE(actions_bar->HidePopup());
+  }
+
+  const Extension* popup_extension_;
+  const Extension* other_extension_;
+};
+
+// Test verifies if an extension pop-up can be navigated to a web page.

[Devlin, 2016/09/26 20:57:05]

s/Test verifies if/Tests that (and add the expected result, so here "that an
extension popup can't be navigated to a web page").

[Łukasz Anforowicz, 2016/09/26 22:05:11]

Done.

+IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest, WebpageViaGet) {
+  GURL web_url(embedded_test_server()->GetURL("foo.com", "/title1.html"));
+  TestPopupNavigationViaGet(web_url,
+                            false /* expecting_navigation_success */);

[Devlin, 2016/09/26 20:57:05]

with how frequent this bool is used, maybe make an enum for readability?

[Łukasz Anforowicz, 2016/09/26 22:05:11]

Done.

+}
+
+// Test verifies if an extension pop-up can be navigated to a web page.
+IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest, WebpageViaPost) {

[Devlin, 2016/09/26 20:57:05]

Browsertests are expensive.  Can we combine the ViaGet/ViaPosts into one test
per scenario?

[Łukasz Anforowicz, 2016/09/26 22:05:11]

Done.

+  GURL web_url(embedded_test_server()->GetURL("foo.com", "/title1.html"));
+  TestPopupNavigationViaPost(web_url,
+                             false /* expecting_navigation_success */);
+}
+
+// Test verifies if an extension pop-up can be navigated to another page
+// in the same extension.
+IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest,
+                       PageInSameExtensionViaGet) {
+  GURL other_page_in_same_extension =
+      popup_extension().GetResourceURL("other_page.html");
+  TestPopupNavigationViaGet(other_page_in_same_extension,
+                            true /* expecting_navigation_success */);
+}
+
+// Test verifies if an extension pop-up can be navigated to another page
+// in the same extension.
+IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest,
+                       PageInSameExtensionViaPost) {
+  GURL other_page_in_same_extension =
+      popup_extension().GetResourceURL("other_page.html");
+  TestPopupNavigationViaPost(other_page_in_same_extension,
+                             true /* expecting_navigation_success */);
+}
+
+// Test verifies if an extension pop-up can be navigated to a page
+// in another extension.
+IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest,
+                       PageInOtherExtensionViaGet) {
+  GURL other_extension_url = other_extension().GetResourceURL("other.html");
+  TestPopupNavigationViaGet(other_extension_url,
+                            false /* expecting_navigation_success */);
+}
+
+// Test verifies if an extension pop-up can be navigated to a page
+// in another extension.
+IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest,
+                       PageInOtherExtensionViaPost) {
+  GURL other_extension_url = other_extension().GetResourceURL("other.html");
+  TestPopupNavigationViaPost(other_extension_url,
+                             false /* expecting_navigation_success */);
+}
+
+// Test verifies if navigating an extension pop-up to a http URI that returns
+// Content-Disposition: attachment; filename=...
+// works: No navigation, but download shelf visible + download goes through.
+//
+// Note - there is no "...ViaGet" flavour of this test, because we don't care
+// (yet) if GET succeeds with the download or not (it probably should succeed
+// for consistency with POST, but it always failed in M54 and before).  After
+// abandoing ShouldFork/OpenURL for all methods (not just for POST) [see comment
+// about https://crbug.com/646261 in ChromeContentRendererClient::ShouldFork]
+// GET should automagically start working for downloads.
+IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest, DownloadViaPost) {
+  content::DownloadTestObserverTerminal downloads_observer(
+      content::BrowserContext::GetDownloadManager(browser()->profile()),
+      1,  // == wait_count (only waiting for "download-test3.gif").
+      content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL);
+
+  // Navigate to a URL that replies with
+  // Content-Disposition: attachment; filename=...
+  // header.
+  GURL download_url(
+      embedded_test_server()->GetURL("foo.com", "/download-test3.gif"));
+  TestPopupNavigationViaPost(download_url,
+                             false /* expecting_navigation_success */);
+
+  // Verify that "download-test3.gif got downloaded.
+  LOG(INFO) << "Waiting for downloads to finish...";
+  downloads_observer.WaitForFinished();
+  EXPECT_EQ(0u, downloads_observer.NumDangerousDownloadsSeen());
+  EXPECT_EQ(1u, downloads_observer.NumDownloadsSeenInState(
+                    content::DownloadItem::COMPLETE));
+
+  // The test verification below is applicable only to scenarios where the
+  // download shelf is supported (e.g. there is no download shelf on Chrome OS).
+  if (browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF))
+    EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+}
+
 }  // namespace
 }  // namespace extensions