Fix possible null WebContents access in ExternalProtocolDialog.

The dialog may outlive the WebContents, but
ExternalProtocolDialogDelegate::DoAccept() is missing a check on the
result of tab_util::GetWebContentsByID() before dereferencing the
WebContents. Add a check and a regression test.

In order to achieve coverage of the crashing lines, update the test
harness and the static method involved to use a
ExternalProtocolHandler::Delegate. This interface already exists and is
used for testing, but wasn't in the codepaths invoked from the crashing
code in ExternalProtocolDialogDelegate::DoAccept().

Bug: 835216
Change-Id: If7d4593090a1db9a96bc829d1e1c253860d0cf20
Reviewed-on: https://chromium-review.googlesource.com/1049774Reviewed-by: Ben Wells <benwells@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557478}

chrome/browser/external_protocol/external_protocol_handler.cc

@@ -90,16 +90,20 @@ void RunExternalProtocolDialogWithDelegate(
 
 void LaunchUrlWithoutSecurityCheckWithDelegate(
     const GURL& url,
-    int render_process_host_id,
-    int render_view_routing_id,
+    content::WebContents* web_contents,
     ExternalProtocolHandler::Delegate* delegate) {
-  content::WebContents* web_contents = tab_util::GetWebContentsByID(
-      render_process_host_id, render_view_routing_id);
   if (delegate) {
     delegate->LaunchUrlWithoutSecurityCheck(url, web_contents);
     return;
   }
-  ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(url, 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;
+
+  platform_util::OpenExternal(
+      Profile::FromBrowserContext(web_contents->GetBrowserContext()), url);
 }
 
 // When we are about to launch a URL with the default OS level application, we
@@ -137,8 +141,10 @@ void OnDefaultProtocolClientWorkerFinished(
     return;
   }
 
-  LaunchUrlWithoutSecurityCheckWithDelegate(escaped_url, render_process_host_id,
-                                            render_view_routing_id, delegate);
+  content::WebContents* web_contents = tab_util::GetWebContentsByID(
+      render_process_host_id, render_view_routing_id);
+  LaunchUrlWithoutSecurityCheckWithDelegate(escaped_url, web_contents,
+                                            delegate);
 }
 
 }  // namespace
@@ -267,13 +273,8 @@ void ExternalProtocolHandler::LaunchUrl(const GURL& url,
 void ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(
     const GURL& url,
     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;
-
-  platform_util::OpenExternal(
-      Profile::FromBrowserContext(web_contents->GetBrowserContext()), url);
+  LaunchUrlWithoutSecurityCheckWithDelegate(
+      url, web_contents, g_external_protocol_handler_delegate);
 }
 
 // static

chrome/browser/ui/external_protocol_dialog_delegate.cc

@@ -63,6 +63,9 @@ void ExternalProtocolDialogDelegate::DoAccept(const GURL& url,
   content::WebContents* web_contents = tab_util::GetWebContentsByID(
       render_process_host_id_, render_view_routing_id_);
 
+  if (!web_contents)
+    return;  // The dialog may outlast the WebContents.
+
   if (remember) {
     Profile* profile =
         Profile::FromBrowserContext(web_contents->GetBrowserContext());

chrome/browser/ui/views/external_protocol_dialog_browsertest.cc

@@ -58,10 +58,10 @@ class TestExternalProtocolDialogDelegate
 
   // ExternalProtocolDialogDelegate:
   void DoAccept(const GURL& url, bool remember) const override {
-    // Don't call the base impl because it will actually launch |url|.
     *called_ = true;
     *accept_ = true;
     *remember_ = remember;
+    ExternalProtocolDialogDelegate::DoAccept(url, remember);
   }
 
  private:
@@ -72,9 +72,17 @@ class TestExternalProtocolDialogDelegate
   DISALLOW_COPY_AND_ASSIGN(TestExternalProtocolDialogDelegate);
 };
 
-class ExternalProtocolDialogBrowserTest : public DialogBrowserTest {
+class ExternalProtocolDialogBrowserTest
+    : public DialogBrowserTest,
+      public ExternalProtocolHandler::Delegate {
  public:
-  ExternalProtocolDialogBrowserTest() {}
+  ExternalProtocolDialogBrowserTest() {
+    ExternalProtocolHandler::SetDelegateForTesting(this);
+  }
+
+  ~ExternalProtocolDialogBrowserTest() override {
+    ExternalProtocolHandler::SetDelegateForTesting(nullptr);
+  }
 
   // DialogBrowserTest:
   void ShowUi(const std::string& name) override {
@@ -95,6 +103,30 @@ class ExternalProtocolDialogBrowserTest : public DialogBrowserTest {
     test::ExternalProtocolDialogTestApi(dialog_).SetCheckBoxSelected(checked);
   }
 
+  // ExternalProtocolHander::Delegate:
+  scoped_refptr<shell_integration::DefaultProtocolClientWorker>
+  CreateShellWorker(
+      const shell_integration::DefaultWebClientWorkerCallback& callback,
+      const std::string& protocol) override {
+    return nullptr;
+  }
+  ExternalProtocolHandler::BlockState GetBlockState(const std::string& scheme,
+                                                    Profile* profile) override {
+    return ExternalProtocolHandler::DONT_BLOCK;
+  }
+  void BlockRequest() override {}
+  void RunExternalProtocolDialog(const GURL& url,
+                                 int render_process_host_id,
+                                 int routing_id,
+                                 ui::PageTransition page_transition,
+                                 bool has_user_gesture) override {}
+  void LaunchUrlWithoutSecurityCheck(
+      const GURL& url,
+      content::WebContents* web_contents) override {
+    url_did_launch_ = true;
+  }
+  void FinishedProcessingCheck() override {}
+
   base::HistogramTester histogram_tester_;
 
  protected:
@@ -102,6 +134,7 @@ class ExternalProtocolDialogBrowserTest : public DialogBrowserTest {
   bool called_ = false;
   bool accept_ = false;
   bool remember_ = false;
+  bool url_did_launch_ = false;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ExternalProtocolDialogBrowserTest);
@@ -113,6 +146,7 @@ IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest, TestAccept) {
   EXPECT_TRUE(called_);
   EXPECT_TRUE(accept_);
   EXPECT_FALSE(remember_);
+  EXPECT_TRUE(url_did_launch_);
   histogram_tester_.ExpectBucketCount(
       ExternalProtocolHandler::kHandleStateMetric,
       ExternalProtocolHandler::LAUNCH, 1);
@@ -126,17 +160,36 @@ IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest,
   EXPECT_TRUE(called_);
   EXPECT_TRUE(accept_);
   EXPECT_TRUE(remember_);
+  EXPECT_TRUE(url_did_launch_);
   histogram_tester_.ExpectBucketCount(
       ExternalProtocolHandler::kHandleStateMetric,
       ExternalProtocolHandler::CHECKED_LAUNCH, 1);
 }
 
+// Regression test for http://crbug.com/835216. The OS owns the dialog, so it
+// may may outlive the WebContents it is attached to.
+IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest,
+                       TestAcceptAfterCloseTab) {
+  ShowUi(std::string());
+  SetChecked(true);  // |remember_| must be true for the segfault to occur.
+  browser()->tab_strip_model()->CloseAllTabs();
+  EXPECT_TRUE(dialog_->Accept());
+  EXPECT_TRUE(called_);
+  EXPECT_TRUE(accept_);
+  EXPECT_TRUE(remember_);
+  EXPECT_FALSE(url_did_launch_);
+  histogram_tester_.ExpectBucketCount(
+      ExternalProtocolHandler::kHandleStateMetric,
+      ExternalProtocolHandler::DONT_LAUNCH, 1);
+}
+
 IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest, TestCancel) {
   ShowUi(std::string());
   EXPECT_TRUE(dialog_->Cancel());
   EXPECT_FALSE(called_);
   EXPECT_FALSE(accept_);
   EXPECT_FALSE(remember_);
+  EXPECT_FALSE(url_did_launch_);
   histogram_tester_.ExpectBucketCount(
       ExternalProtocolHandler::kHandleStateMetric,
       ExternalProtocolHandler::DONT_LAUNCH, 1);
@@ -150,6 +203,7 @@ IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest,
   EXPECT_FALSE(called_);
   EXPECT_FALSE(accept_);
   EXPECT_FALSE(remember_);
+  EXPECT_FALSE(url_did_launch_);
   histogram_tester_.ExpectBucketCount(
       ExternalProtocolHandler::kHandleStateMetric,
       ExternalProtocolHandler::DONT_LAUNCH, 1);
@@ -162,6 +216,7 @@ IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest, TestClose) {
   EXPECT_FALSE(called_);
   EXPECT_FALSE(accept_);
   EXPECT_FALSE(remember_);
+  EXPECT_FALSE(url_did_launch_);
   histogram_tester_.ExpectBucketCount(
       ExternalProtocolHandler::kHandleStateMetric,
       ExternalProtocolHandler::DONT_LAUNCH, 1);
@@ -176,6 +231,7 @@ IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest,
   EXPECT_FALSE(called_);
   EXPECT_FALSE(accept_);
   EXPECT_FALSE(remember_);
+  EXPECT_FALSE(url_did_launch_);
   histogram_tester_.ExpectBucketCount(
       ExternalProtocolHandler::kHandleStateMetric,
       ExternalProtocolHandler::DONT_LAUNCH, 1);