fix external protocol handling for OOPIFs

Replace using the wrong render_process_id for launching external protocol
requests in chrome_resource_dispatcher_host_delegate.cc.

Previously, handlers for external protocols like 'mailto:' weren't working for
OOPIFs because the render_process_id used to retrieve the content::WebContents
object was incorrect.

BUG=668289
TEST=See bug for repro steps

Review-Url: https://codereview.chromium.org/2538353002
Cr-Commit-Position: refs/heads/master@{#439654}

chrome/browser/chrome_site_per_process_browsertest.cc

@@ -8,6 +8,8 @@
 #include "base/path_service.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/external_protocol/external_protocol_handler.h"
+#include "chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -363,6 +365,115 @@ IN_PROC_BROWSER_TEST_F(ChromeSitePerProcessPDFTest,
   EXPECT_EQ(0U, test_guest_view_manager()->GetNumGuestsActive());
 }
 
+// A helper class to verify that a "mailto:" external protocol request succeeds.
+class MailtoExternalProtocolHandlerDelegate
+    : public ExternalProtocolHandler::Delegate {
+ public:
+  bool has_triggered_external_protocol() {
+    return has_triggered_external_protocol_;
+  }
+
+  const GURL& external_protocol_url() { return external_protocol_url_; }
+
+  content::WebContents* web_contents() { return web_contents_; }
+
+  void RunExternalProtocolDialog(const GURL& url,
+                                 int render_process_host_id,
+                                 int routing_id,
+                                 ui::PageTransition page_transition,
+                                 bool has_user_gesture) override {}
+
+  scoped_refptr<shell_integration::DefaultProtocolClientWorker>
+  CreateShellWorker(
+      const shell_integration::DefaultWebClientWorkerCallback& callback,
+      const std::string& protocol) override {
+    return new shell_integration::DefaultProtocolClientWorker(callback,
+                                                              protocol);
+  }
+
+  ExternalProtocolHandler::BlockState GetBlockState(
+      const std::string& scheme) override {
+    return ExternalProtocolHandler::DONT_BLOCK;
+  }
+
+  void BlockRequest() override {}
+
+  void LaunchUrlWithoutSecurityCheck(
+      const GURL& url,
+      content::WebContents* web_contents) override {
+    external_protocol_url_ = url;
+    web_contents_ = web_contents;
+    has_triggered_external_protocol_ = true;
+    if (message_loop_runner_)
+      message_loop_runner_->Quit();
+  }
+
+  void Wait() {
+    if (!has_triggered_external_protocol_) {
+      message_loop_runner_ = new content::MessageLoopRunner();
+      message_loop_runner_->Run();
+    }
+  }
+
+  void FinishedProcessingCheck() override {}
+
+ private:
+  bool has_triggered_external_protocol_ = false;
+  GURL external_protocol_url_;
+  content::WebContents* web_contents_ = nullptr;
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+};
+
+// This test is not run on ChromeOS because it registers a custom handler (see
+// ProtocolHandlerRegistry::InstallDefaultsForChromeOS), and handles mailto:
+// navigations before getting to external protocol code.
+#if defined(OS_CHROMEOS)
+#define MAYBE_LaunchExternalProtocolFromSubframe \
+  DISABLED_LaunchExternalProtocolFromSubframe
+#else
+#define MAYBE_LaunchExternalProtocolFromSubframe \
+  LaunchExternalProtocolFromSubframe
+#endif
+// This test verifies that external protocol requests succeed when made from an
+// OOPIF (https://crbug.com/668289).
+IN_PROC_BROWSER_TEST_F(ChromeSitePerProcessTest,
+                       MAYBE_LaunchExternalProtocolFromSubframe) {
+  GURL start_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+
+  ui_test_utils::NavigateToURL(browser(), start_url);
+
+  // Navigate to a page with a cross-site iframe that triggers a mailto:
+  // external protocol request.
+  // The test did not start by navigating to this URL because that would mask
+  // the bug.  Instead, navigating the main frame to another page will cause a
+  // cross-process transfer, which will avoid a situation where the OOPIF's
+  // swapped-out RenderViewHost and the main frame's active RenderViewHost get
+  // the same routing IDs, causing an accidental success.
+  GURL mailto_main_frame_url(
+      embedded_test_server()->GetURL("b.com", "/iframe.html"));
+
+  ui_test_utils::NavigateToURL(browser(), mailto_main_frame_url);
+
+  MailtoExternalProtocolHandlerDelegate delegate;
+  ChromeResourceDispatcherHostDelegate::
+      SetExternalProtocolHandlerDelegateForTesting(&delegate);
+
+  GURL mailto_subframe_url(
+      embedded_test_server()->GetURL("c.com", "/page_with_mailto.html"));
+  content::WebContents* active_web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_TRUE(
+      NavigateIframeToURL(active_web_contents, "test", mailto_subframe_url));
+
+  delegate.Wait();
+
+  EXPECT_TRUE(delegate.has_triggered_external_protocol());
+  EXPECT_EQ(delegate.external_protocol_url(), GURL("mailto:mail@example.org"));
+  EXPECT_EQ(active_web_contents, delegate.web_contents());
+  ChromeResourceDispatcherHostDelegate::
+      SetExternalProtocolHandlerDelegateForTesting(nullptr);
+}
+
 // Verify that a popup can be opened after navigating a remote frame.  This has
 // to be a chrome/ test to ensure that the popup blocker doesn't block the
 // popup.  See https://crbug.com/670770.

chrome/browser/external_protocol/external_protocol_handler.cc

@@ -82,11 +82,13 @@ void LaunchUrlWithoutSecurityCheckWithDelegate(
     int render_process_host_id,
     int tab_contents_id,
     ExternalProtocolHandler::Delegate* delegate) {
+  content::WebContents* web_contents =
+      tab_util::GetWebContentsByID(render_process_host_id, tab_contents_id);
+
   if (!delegate) {
-    ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(
-        url, render_process_host_id, tab_contents_id);
+    ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(url, web_contents);
   } else {
-    delegate->LaunchUrlWithoutSecurityCheck(url);
+    delegate->LaunchUrlWithoutSecurityCheck(url, web_contents);
   }
 }
 
@@ -222,10 +224,9 @@ void ExternalProtocolHandler::LaunchUrlWithDelegate(
 // static
 void ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(
     const GURL& url,
-    int render_process_host_id,
-    int tab_contents_id) {
-  content::WebContents* web_contents = tab_util::GetWebContentsByID(
-      render_process_host_id, tab_contents_id);
+    content::WebContents* web_contents) {
+  // |web_contents| is only passed in to find browser context. Do not assume
+  // that the external protocol request came from the main frame.
   if (!web_contents)
     return;
 

chrome/browser/external_protocol/external_protocol_handler.h

@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "chrome/browser/shell_integration.h"
+#include "content/public/browser/web_contents.h"
 #include "ui/base/page_transition_types.h"
 
 class GURL;
@@ -41,7 +42,9 @@ class ExternalProtocolHandler {
         int routing_id,
         ui::PageTransition page_transition,
         bool has_user_gesture) = 0;
-    virtual void LaunchUrlWithoutSecurityCheck(const GURL& url) = 0;
+    virtual void LaunchUrlWithoutSecurityCheck(
+        const GURL& url,
+        content::WebContents* web_contents) = 0;
     virtual void FinishedProcessingCheck() = 0;
     virtual ~Delegate() {}
   };
@@ -76,8 +79,7 @@ class ExternalProtocolHandler {
   // url you have has been checked against the blacklist, and has been escaped.
   // All calls to this function should originate in some way from LaunchUrl.
   static void LaunchUrlWithoutSecurityCheck(const GURL& url,
-                                            int render_process_host_id,
-                                            int tab_contents_id);
+                                            content::WebContents* web_contents);
 
   // Allows LaunchUrl to proceed with launching an external protocol handler.
   // This is typically triggered by a user gesture, but is also called for
@@ -106,6 +108,8 @@ class ExternalProtocolHandler {
   //       to change the command line by itself, we do not do anything special
   //       to protect against this scenario.
   // This is implemented separately on each platform.
+  // TODO(davidsac): Consider refactoring this to take a WebContents directly.
+  // crbug.com/668289
   static void RunExternalProtocolDialog(const GURL& url,
                                         int render_process_host_id,
                                         int routing_id,

chrome/browser/external_protocol/external_protocol_handler_unittest.cc

@@ -73,7 +73,9 @@ class FakeExternalProtocolHandlerDelegate
     has_prompted_ = true;
   }
 
-  void LaunchUrlWithoutSecurityCheck(const GURL& url) override {
+  void LaunchUrlWithoutSecurityCheck(
+      const GURL& url,
+      content::WebContents* web_contents) override {
     ASSERT_EQ(block_state_, ExternalProtocolHandler::DONT_BLOCK);
     ASSERT_NE(os_state_, shell_integration::IS_DEFAULT);
     has_launched_ = true;

chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc

@@ -224,7 +224,6 @@ void SendExecuteMimeTypeHandlerEvent(
 
 void LaunchURL(
     const GURL& url,
-    int render_process_id,
     const content::ResourceRequestInfo::WebContentsGetter& web_contents_getter,
     ui::PageTransition page_transition,
     bool has_user_gesture,
@@ -247,15 +246,11 @@ void LaunchURL(
   // If the URL is in whitelist, we launch it without asking the user and
   // without any additional security checks. Since the URL is whitelisted,
   // we assume it can be executed.
-  // TODO(davidsac): External protocol handling needs to be
-  // fixed for OOPIFs.  See https://crbug.com/668289.
   if (is_whitelisted) {
-    ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(
-        url, render_process_id,
-        web_contents->GetRenderViewHost()->GetRoutingID());
+    ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(url, web_contents);
   } else {
     ExternalProtocolHandler::LaunchUrlWithDelegate(
-        url, render_process_id,
+        url, web_contents->GetRenderViewHost()->GetProcess()->GetID(),
         web_contents->GetRenderViewHost()->GetRoutingID(), page_transition,
         has_user_gesture, g_external_protocol_handler_delegate);
   }
@@ -578,11 +573,12 @@ bool ChromeResourceDispatcherHostDelegate::HandleExternalProtocol(
     // content page.
     return false;
   }
-  int child_id = info->GetChildID();
+
 #if BUILDFLAG(ENABLE_EXTENSIONS)
   // External protocols are disabled for guests. An exception is made for the
   // "mailto" protocol, so that pages that utilize it work properly in a
   // WebView.
+  int child_id = info->GetChildID();
   ChromeNavigationUIData* navigation_data =
       static_cast<ChromeNavigationUIData*>(info->GetNavigationUIData());
   if ((extensions::WebViewRendererState::GetInstance()->IsGuest(child_id) ||
@@ -604,8 +600,7 @@ bool ChromeResourceDispatcherHostDelegate::HandleExternalProtocol(
       url_state == policy::URLBlacklist::URLBlacklistState::URL_IN_WHITELIST;
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
-      base::Bind(&LaunchURL, url, child_id,
-                 info->GetWebContentsGetterForRequest(),
+      base::Bind(&LaunchURL, url, info->GetWebContentsGetterForRequest(),
                  info->GetPageTransition(), info->HasUserGesture(),
                  is_whitelisted));
   return true;

chrome/browser/prerender/prerender_test_utils.cc

@@ -240,7 +240,11 @@ class NeverRunsExternalProtocolHandlerDelegate
     NOTREACHED();
   }
 
-  void LaunchUrlWithoutSecurityCheck(const GURL& url) override { NOTREACHED(); }
+  void LaunchUrlWithoutSecurityCheck(
+      const GURL& url,
+      content::WebContents* web_contents) override {
+    NOTREACHED();
+  }
 
   void FinishedProcessingCheck() override { NOTREACHED(); }
 };

chrome/browser/ui/cocoa/external_protocol_dialog.mm

@@ -8,6 +8,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "chrome/browser/external_protocol/external_protocol_handler.h"
 #include "chrome/browser/shell_integration.h"
+#include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/strings/grit/components_strings.h"
@@ -117,8 +118,10 @@ void ExternalProtocolHandler::RunExternalProtocolDialog(
     UMA_HISTOGRAM_LONG_TIMES("clickjacking.launch_url",
                              base::Time::Now() - creation_time_);
 
-    ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(
-        url_, render_process_host_id_, routing_id_);
+    content::WebContents* web_contents =
+        tab_util::GetWebContentsByID(render_process_host_id_, routing_id_);
+
+    ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(url_, web_contents);
   }
 
   [self autorelease];

chrome/browser/ui/external_protocol_dialog_delegate.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/external_protocol_dialog_delegate.h"
 
 #include "chrome/browser/external_protocol/external_protocol_handler.h"
+#include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/strings/grit/components_strings.h"
@@ -65,8 +66,10 @@ void ExternalProtocolDialogDelegate::DoAccept(const GURL& url,
                                            ExternalProtocolHandler::DONT_BLOCK);
   }
 
-  ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(
-      url, render_process_host_id_, tab_contents_id_);
+  content::WebContents* web_contents =
+      tab_util::GetWebContentsByID(render_process_host_id_, tab_contents_id_);
+
+  ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(url, web_contents);
 }
 
 void ExternalProtocolDialogDelegate::DoCancel(const GURL& url,

chrome/test/data/page_with_mailto.html

+<!DOCTYPE html>
+<html>
+<body onload="window.location.href='mailto:mail@example.org';">
+  This page automatically opens a mailto: link.
+</body>
+</html>