Remove ChromeExtensionsRendererClient::ShouldFork.

After https://crrev.com/c/1776854 the extension-related browser-side
checks should be a superset of renderer-side checks and we should be
able to start removing extension-related renderer-side checks like
ShouldFork.

Bug: 1003957, 883549
Change-Id: I2954ab641b8234c2b3b18db69bf04caae437e989
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1809814Reviewed-by: Egor Pasko <pasko@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700364}

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

@@ -999,7 +999,6 @@ class NavigatingExtensionPopupBrowserTest : public BrowserActionApiTest {
   enum ExpectedNavigationStatus {
     EXPECTING_NAVIGATION_SUCCESS,
     EXPECTING_NAVIGATION_FAILURE,
-    EXPECTING_NO_NAVIGATION,
   };
 
   void TestPopupNavigationViaGet(
@@ -1061,11 +1060,8 @@ class NavigatingExtensionPopupBrowserTest : public BrowserActionApiTest {
     } 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.
-      if (expected_navigation_status != EXPECTING_NO_NAVIGATION) {
-        popup_navigation_observer.Wait();
-      } else {
-        EXPECT_FALSE(popup->IsLoading());
-      }
+      popup_navigation_observer.Wait();
+
       // The popup should still be alive.
       ASSERT_TRUE(popup_destruction_watcher.web_contents());
 
@@ -1109,12 +1105,7 @@ class NavigatingExtensionPopupBrowserTest : public BrowserActionApiTest {
 // Tests that an extension pop-up cannot be navigated to a web page.
 IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest, Webpage) {
   GURL web_url(embedded_test_server()->GetURL("foo.com", "/title1.html"));
-
-  // The GET request will be blocked in ExtensionViewHost::OpenURLFromTab
-  // (which silently drops navigations with CURRENT_TAB disposition).
-  TestPopupNavigationViaGet(web_url, EXPECTING_NO_NAVIGATION);
-
-  // POST requests don't go through ExtensionViewHost::OpenURLFromTab.
+  TestPopupNavigationViaGet(web_url, EXPECTING_NAVIGATION_FAILURE);
   TestPopupNavigationViaPost(web_url, EXPECTING_NAVIGATION_FAILURE);
 }
 
@@ -1136,22 +1127,13 @@ IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest,
 IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest,
                        PageInOtherExtension) {
   GURL other_extension_url = other_extension().GetResourceURL("other.html");
-  TestPopupNavigationViaGet(other_extension_url, EXPECTING_NO_NAVIGATION);
+  TestPopupNavigationViaGet(other_extension_url, EXPECTING_NAVIGATION_FAILURE);
   TestPopupNavigationViaPost(other_extension_url, EXPECTING_NAVIGATION_FAILURE);
 }
 
 // Tests that 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.
-// TODO(lukasza): https://crbug.com/650694: Add a "Get" flavour of the test once
-// the download works both for GET and POST requests.
 IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest, DownloadViaPost) {
   // Setup monitoring of the downloads.
   content::DownloadTestObserverTerminal downloads_observer(
@@ -1186,6 +1168,40 @@ IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest, DownloadViaPost) {
 #endif
 }
 
+IN_PROC_BROWSER_TEST_F(NavigatingExtensionPopupBrowserTest, DownloadViaGet) {
+  // Setup monitoring of the downloads.
+  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"));
+  TestPopupNavigationViaGet(download_url, EXPECTING_NAVIGATION_FAILURE);
+
+  // Verify that "download-test3.gif got downloaded.
+  downloads_observer.WaitForFinished();
+  EXPECT_EQ(0u, downloads_observer.NumDangerousDownloadsSeen());
+  EXPECT_EQ(1u, downloads_observer.NumDownloadsSeenInState(
+                    download::DownloadItem::COMPLETE));
+
+  base::ScopedAllowBlockingForTesting allow_blocking;
+  base::FilePath downloads_directory =
+      DownloadPrefs(browser()->profile()).DownloadPath();
+  EXPECT_TRUE(base::PathExists(
+      downloads_directory.AppendASCII("download-test3-attachment.gif")));
+
+  // The test verification below is applicable only to scenarios where the
+  // download shelf is supported - on ChromeOS, instead of the download shelf,
+  // there is a download notification in the right-bottom corner of the screen.
+#if !defined(OS_CHROMEOS)
+  EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+#endif
+}
+
 // Verify video can enter and exit Picture-in_Picture when browser action icon
 // is clicked.
 IN_PROC_BROWSER_TEST_F(BrowserActionApiTest,

chrome/browser/extensions/extension_resource_request_policy_apitest.cc

@@ -293,9 +293,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionResourceRequestPolicyTest,
   EXPECT_EQ(content::PAGE_TYPE_ERROR,
             controller.GetLastCommittedEntry()->GetPageType());
   EXPECT_EQ("chrome-error://chromewebdata/", result);
-  GURL nonaccessible_url = extension->GetResourceURL("/test2.png");
-  EXPECT_EQ(nonaccessible_url,
-            web_contents->GetMainFrame()->GetLastCommittedURL());
+  GURL invalid_url("chrome-extension://invalid/");
+  EXPECT_EQ(invalid_url, web_contents->GetMainFrame()->GetLastCommittedURL());
 
   // Redirects can sometimes occur before the load event, so use a
   // UrlLoadObserver instead of blocking waiting for two load events.
@@ -311,7 +310,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionResourceRequestPolicyTest,
   EXPECT_EQ(accessible_url, web_contents->GetLastCommittedURL());
 
   ui_test_utils::UrlLoadObserver nonaccessible_observer(
-      nonaccessible_url, content::NotificationService::AllSources());
+      invalid_url, content::NotificationService::AllSources());
   GURL nonaccessible_client_redirect_resource(embedded_test_server()->GetURL(
       "/extensions/api_test/extension_resource_request_policy/"
       "web_accessible/nonaccessible_redirect_resource.html"));
@@ -320,7 +319,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionResourceRequestPolicyTest,
   nonaccessible_observer.Wait();
   EXPECT_EQ(content::PAGE_TYPE_ERROR,
             controller.GetLastCommittedEntry()->GetPageType());
-  EXPECT_EQ(nonaccessible_url, web_contents->GetLastCommittedURL());
+  EXPECT_EQ(invalid_url, web_contents->GetLastCommittedURL());
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionResourceRequestPolicyTest,

chrome/browser/prerender/prerender_browsertest.cc

@@ -2063,19 +2063,6 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderWebAudioDevice) {
                    FINAL_STATUS_CREATING_AUDIO_STREAM, 0);
 }
 
-// Checks that prerenders are aborted on cross-process navigation from
-// a client redirect.
-IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
-                       PrerenderCrossProcessClientRedirect) {
-  // Cross-process navigation logic for renderer-initiated navigations
-  // is partially controlled by the renderer, namely
-  // ChromeContentRendererClient. This test instead relies on the Web
-  // Store triggering such navigations.
-  PrerenderTestURL(
-      CreateClientRedirect(extension_urls::GetWebstoreLaunchURL().spec()),
-      FINAL_STATUS_OPEN_URL, 1);
-}
-
 // Checks that deferred redirects in a synchronous XHR abort the prerender.
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderDeferredSynchronousXHR) {
   // Disable load event checks because they race with cancellation.

chrome/renderer/chrome_content_renderer_client.cc

@@ -1269,16 +1269,6 @@ bool ChromeContentRendererClient::ShouldFork(WebLocalFrame* frame,
   }
 #endif
 
-  // TODO(lukasza): https://crbug.com/650694: For now, we skip the rest for POST
-  // submissions.  This is because 1) in M54 there are some remaining issues
-  // with POST in OpenURL path (e.g. https://crbug.com/648648) and 2) OpenURL
-  // path regresses (blocks) navigations that result in downloads
-  // (https://crbug.com/646261).  In the long-term we should avoid forking for
-  // extensions (not hosted apps though) altogether and rely on transfers logic
-  // instead.
-  if (http_method != "GET")
-    return false;
-
   // If |url| matches one of the prerendered URLs, stop this navigation and try
   // to swap in the prerendered page on the browser process. If the prerendered
   // page no longer exists by the time the OpenURL IPC is handled, a normal
@@ -1288,13 +1278,6 @@ bool ChromeContentRendererClient::ShouldFork(WebLocalFrame* frame,
     return true;
   }
 
-#if BUILDFLAG(ENABLE_EXTENSIONS)
-  bool should_fork = ChromeExtensionsRendererClient::ShouldFork(
-      frame, url, is_initial_navigation, is_server_redirect);
-  if (should_fork)
-    return true;
-#endif  // BUILDFLAG(ENABLE_EXTENSIONS)
-
   return false;
 }
 

chrome/renderer/extensions/chrome_extensions_renderer_client.cc

@@ -53,11 +53,6 @@ using extensions::Extension;
 
 namespace {
 
-bool IsStandaloneExtensionProcess() {
-  return base::CommandLine::ForCurrentProcess()->HasSwitch(
-      extensions::switches::kExtensionProcess);
-}
-
 void IsGuestViewApiAvailableToScriptContext(
     bool* api_is_available,
     extensions::ScriptContext* context) {
@@ -66,54 +61,6 @@ void IsGuestViewApiAvailableToScriptContext(
   }
 }
 
-// Returns true if the frame is navigating to an URL either into or out of an
-// extension app's extent.
-bool CrossesExtensionExtents(blink::WebLocalFrame* frame,
-                             const GURL& new_url,
-                             bool is_extension_url,
-                             bool is_initial_navigation) {
-  DCHECK(!frame->Parent());
-  GURL old_url(frame->GetDocument().Url());
-
-  extensions::RendererExtensionRegistry* extension_registry =
-      extensions::RendererExtensionRegistry::Get();
-
-  // If old_url is still empty and this is an initial navigation, then this is
-  // a window.open operation.  We should look at the opener URL.  Note that the
-  // opener is a local frame in this case.
-  if (is_initial_navigation && old_url.is_empty() && frame->Opener()) {
-    blink::WebLocalFrame* opener_frame = frame->Opener()->ToWebLocalFrame();
-
-    // We want to compare against the URL that determines the type of
-    // process.  Use the URL of the opener's local frame root, which will
-    // correctly handle any site isolation modes (e.g. --site-per-process).
-    blink::WebLocalFrame* local_root = opener_frame->LocalRoot();
-    old_url = local_root->GetDocument().Url();
-
-    // If we're about to open a normal web page from a same-origin opener stuck
-    // in an extension process (other than the Chrome Web Store), we want to
-    // keep it in process to allow the opener to script it.
-    blink::WebDocument opener_document = opener_frame->GetDocument();
-    blink::WebSecurityOrigin opener_origin =
-        opener_document.GetSecurityOrigin();
-    bool opener_is_extension_url =
-        !opener_origin.IsUnique() && extension_registry->GetExtensionOrAppByURL(
-                                         opener_document.Url()) != nullptr;
-    const Extension* opener_top_extension =
-        extension_registry->GetExtensionOrAppByURL(old_url);
-    bool opener_is_web_store =
-        opener_top_extension &&
-        opener_top_extension->id() == extensions::kWebStoreAppId;
-    if (!is_extension_url && !opener_is_extension_url && !opener_is_web_store &&
-        IsStandaloneExtensionProcess() &&
-        opener_origin.CanRequest(blink::WebURL(new_url)))
-      return false;
-  }
-
-  return extensions::CrossesExtensionProcessBoundary(
-      *extension_registry->GetMainThreadExtensionSet(), old_url, new_url);
-}
-
 }  // namespace
 
 ChromeExtensionsRendererClient::ChromeExtensionsRendererClient() {}
@@ -301,39 +248,6 @@ ChromeExtensionsRendererClient::GetExtensionDispatcherForTest() {
   return extension_dispatcher();
 }
 
-// static
-bool ChromeExtensionsRendererClient::ShouldFork(blink::WebLocalFrame* frame,
-                                                const GURL& url,
-                                                bool is_initial_navigation,
-                                                bool is_server_redirect) {
-  const extensions::RendererExtensionRegistry* extension_registry =
-      extensions::RendererExtensionRegistry::Get();
-
-  // Determine if the new URL is an extension (excluding bookmark apps).
-  const Extension* new_url_extension = extensions::GetNonBookmarkAppExtension(
-      *extension_registry->GetMainThreadExtensionSet(), url);
-  bool is_extension_url = !!new_url_extension;
-
-  // If the navigation would cross an app extent boundary, we also need
-  // to defer to the browser to ensure process isolation.  This is not necessary
-  // for server redirects, which will be transferred to a new process by the
-  // browser process when they are ready to commit.  It is necessary for client
-  // redirects, which won't be transferred in the same way.
-  if (!is_server_redirect &&
-      CrossesExtensionExtents(frame, url, is_extension_url,
-                              is_initial_navigation)) {
-    const Extension* extension =
-        extension_registry->GetExtensionOrAppByURL(url);
-    if (extension && extension->is_app()) {
-      extensions::RecordAppLaunchType(
-          extension_misc::APP_LAUNCH_CONTENT_NAVIGATION, extension->GetType());
-    }
-    return true;
-  }
-
-  return false;
-}
-
 // static
 content::BrowserPluginDelegate*
 ChromeExtensionsRendererClient::CreateBrowserPluginDelegate(

chrome/renderer/extensions/chrome_extensions_renderer_client.h

@@ -88,10 +88,6 @@ class ChromeExtensionsRendererClient
       std::unique_ptr<extensions::Dispatcher> extension_dispatcher);
   extensions::Dispatcher* GetExtensionDispatcherForTest();
 
-  static bool ShouldFork(blink::WebLocalFrame* frame,
-                         const GURL& url,
-                         bool is_initial_navigation,
-                         bool is_server_redirect);
   static content::BrowserPluginDelegate* CreateBrowserPluginDelegate(
       content::RenderFrame* render_frame,
       const content::WebPluginInfo& info,

chrome/test/data/extensions/api_test/webnavigation/crossProcess/test_crossProcess.js

@@ -68,7 +68,7 @@ onload = async function() {
                      processId: 1,
                      tabId: 0,
                      timeStamp: 0,
-                     transitionQualifiers: [],
+                     transitionQualifiers: ["client_redirect"],
                      transitionType: "link",
                      url: URL_REGULAR }},
         { label: "b-onDOMContentLoaded",
@@ -144,7 +144,8 @@ onload = async function() {
                      processId: 1,
                      tabId: 0,
                      timeStamp: 0,
-                     transitionQualifiers: ["server_redirect"],
+                     transitionQualifiers: ["client_redirect",
+                                            "server_redirect"],
                      transitionType: "link",
                      url: URL_REGULAR }},
         { label: "c-onDOMContentLoaded",

chrome/test/data/extensions/api_test/webnavigation/crossProcessHistory/test_crossProcessHistory.js

@@ -72,7 +72,7 @@ onload = async function() {
                      processId: 1,
                      tabId: 0,
                      timeStamp: 0,
-                     transitionQualifiers: [],
+                     transitionQualifiers: ["client_redirect"],
                      transitionType: "link",
                      url: URL_TEST + "2" }},
         { label: "b-onDOMContentLoaded",
@@ -250,7 +250,7 @@ onload = async function() {
                      processId: 1,
                      tabId: 0,
                      timeStamp: 0,
-                     transitionQualifiers: [],
+                     transitionQualifiers: ["client_redirect"],
                      transitionType: "link",
                      url: URL_TEST + "6" }},
         { label: "b-onDOMContentLoaded",