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..40b32fd13962e0b7bb6658c3e3beafc5a8ee40c2 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,237 @@ 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,
+                              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...";

[Charlie Reis, 2016/09/26 17:27:50]

Did you mean to leave these LOG(INFO) statements in?  I'm ok with it if you
think it will help diagnose test failures, though I'm not sure if we need all of
them.

[Łukasz Anforowicz, 2016/09/26 18:30:23]

When I was working on the tests, I had them hang (i.e. when navigation is
blocked, then downloads never complete).  Having the extra logging helped to
figure out which part of the test was hanging - I think this will also be true
for cases where the tests time out on a try bot.

That said, maybe some of the logging statements are not really needed - for
example I've removed the one talking about opening the pop-up.

+      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()
+            << " 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,
+                                 bool expecting_navigation_success) {
+    // Were there any failures so far (e.g. in SetUpOnMainThread)?
+    ASSERT_FALSE(HasFailure());
+
+    // Simulate a click on the browser action to open the popup.
+    LOG(INFO) << "Opening an extension 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 navigation_starting_script =
+        "window.location = '" + target_url.spec() + "';"
+        "setTimeout("
+        "  function(){ window.domAutomationController.send(true); },"
+        "  0);";
+
+    // Try to navigate the pop-up.
+    bool ignored;
+    content::WebContentsDestroyedWatcher popup_destruction_watcher(popup);
+    LOG(INFO) << "Navigating the popup to " << target_url;
+    EXPECT_TRUE(ExecuteScriptAndExtractBool(popup, navigation_starting_script,
+                                            &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());
+  }
+
+  void TestPopupNavigationViaPost(GURL target_url,
+                                  bool expecting_navigation_success) {
+    // Were there any failures so far (e.g. in SetUpOnMainThread)?
+    ASSERT_FALSE(HasFailure());
+
+    // Simulate a click on the browser action to open the popup.
+    LOG(INFO) << "Opening an extension 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 navigation_starting_script =
+        "var form = document.getElementById('form');"
+        "form.action = '" + target_url.spec() + "';"
+        "form.submit();"

[Charlie Reis, 2016/09/26 17:27:50]

This function looks the same as TestPopupNavigationViaGet except for these 3
lines of script.  Maybe let the rest share code, given that they're already
written as helper functions?

[Łukasz Anforowicz, 2016/09/26 18:30:23]

Done.

+        "setTimeout("
+        "  function(){ window.domAutomationController.send(true); },"
+        "  0);";
+
+    // Try to navigate the pop-up.
+    bool ignored;
+    content::WebContentsDestroyedWatcher popup_destruction_watcher(popup);
+    LOG(INFO) << "Navigating the popup to " << target_url;
+    EXPECT_TRUE(ExecuteScriptAndExtractBool(popup, navigation_starting_script,
+                                            &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());
+  }
+
+ private:
+  const Extension* popup_extension_;
+  const Extension* other_extension_;
+};
+
+// Test verifies if an extension pop-up can be navigated to a web page.
+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 */);

[Charlie Reis, 2016/09/26 17:27:50]

nit: No need for the ==

[Łukasz Anforowicz, 2016/09/26 18:30:23]

Done.

+}
+
+// Test verifies if an extension pop-up can be navigated to a web page.
+IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest, WebpageViaPost) {
+  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));
+#if !defined(OS_CHROMEOS)

[Charlie Reis, 2016/09/26 17:27:50]

nit: Add a comment explaining why this is skipped on ChromeOS.

[Łukasz Anforowicz, 2016/09/26 18:30:23]

Done.

I've also felt that maybe !defined(OS_CHROMEOS) is a bit too fragile - I was
able to replace this condition with a more direct test for download shelf
feature support (I am keeping my fingers crossed that this will also pass on
ChromeOS trybots).

[Charlie Reis, 2016/09/26 19:00:48]

Cool, I like it.

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

Actually, this didn't work...  I've built CrOS browser_tests and the test indeed
doesn't trigger the download shelf on this OS - there is a download notification
in bottom right screen corner instead.  Surprisingly,
browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF) returns true,
so I think I need to go back to !defined(OS_CHROMEOS) as the condition here.

Visibility of the shelf is not critical for this test, but it still feels fairly
important / integral to the expected behavior for the extension described in
https://crbug.com/646261.  I am therefore hesitant to remove this verification
just because it doesn't work for ChromeOS.  I hope the ifdef approach here is
okay.

+  EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+#endif
+}
+
 }  // namespace
 }  // namespace extensions