fix external protocol handling for OOPIFs

chrome/browser/chrome_site_per_process_browsertest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/command_line.h"
+#include "base/debug/stack_trace.h"

[alexmos, 2016/12/12 21:11:39]

Don't forget to remove.

[davidsac (gone - try alexmos), 2016/12/13 23:02:32]

Done.

 #include "base/files/file_path.h"
 #include "base/macros.h"
 #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"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/guest_view/browser/guest_view_manager_delegate.h"
 #include "components/guest_view/browser/test_guest_view_manager.h"
 #include "content/public/browser/interstitial_page.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_service.h"
@@ skipping 335 matching lines @@
 
   // Now detach the frame and observe that the guest is destroyed.
   content::WebContentsDestroyedWatcher observer(guest_web_contents);
   EXPECT_TRUE(ExecuteScript(
       active_web_contents,
       "document.body.removeChild(document.querySelector('iframe'));"));
   observer.Wait();
   EXPECT_EQ(0U, test_guest_view_manager()->GetNumGuestsActive());
 }
 
+// a helper class to verify that a "mailto:" external protocol

[alexmos, 2016/12/12 21:11:39]

nit: capitalize A and end sentence with period.

[davidsac (gone - try alexmos), 2016/12/13 23:02:32]

Done.

+// request succeeds during unit tests

[alexmos, 2016/12/12 21:11:40]

This is not a unit test but rather a browser test, so let's just drop "during
unit tests".

[davidsac (gone - try alexmos), 2016/12/13 23:02:32]

Done.

+class MailtoExternalProtocolHandlerDelegate
+    : public ExternalProtocolHandler::Delegate {
+ public:
+  bool has_triggered_external_protocol() {
+    return has_triggered_external_protocol_;
+  }
+
+  std::string getURL() { return url_path_; }

[alexmos, 2016/12/12 21:11:40]

For simple accessors like this, we typically don't use the "get" prefix and use
all lowercase and underscores.  See
https://google.github.io/styleguide/cppguide.html#Function_Names
(For more complex functions that are not simple getters/setters, you'd use Camel
Case)

Also, I'd clarify in the name which url this is.  I.e., name this
external_protocol_url().

[alexmos, 2016/12/12 21:11:40]

I'd just store the full GURL and return this from here, since this is what your
method name implies.  Then your call site could validate the full URL, which is
more useful as it also validates the scheme. (See also my comment there.) 

[davidsac (gone - try alexmos), 2016/12/13 23:02:32]

Done.

[davidsac (gone - try alexmos), 2016/12/13 23:02:32]

Done.

+
+  content::WebContents* getWebContents() { return web_contents_; }

[alexmos, 2016/12/12 21:11:40]

Similarly, you can rename this to web_contents().

[davidsac (gone - try alexmos), 2016/12/13 23:02:32]

Done.

+
+  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::GetBlockState(scheme);
+  }
+
+  void BlockRequest() override {}
+
+  void LaunchUrlWithoutSecurityCheck(
+      const GURL& url,
+      content::WebContents* web_contents) override {
+    if (message_loop_runner_) {
+      LOG(INFO) << "message_loop_runner_ is not null!";

[alexmos, 2016/12/12 21:11:39]

Don't forget to remove the LOGs once you're done debugging.

[davidsac (gone - try alexmos), 2016/12/13 23:02:32]

Done.

+    } else {
+      LOG(INFO) << "message_loop_runner_ is null!";
+    }
+    url_path_ = url.path();
+    web_contents_ = web_contents;
+    if (web_contents) {

[alexmos, 2016/12/12 21:11:39]

nit: no need for { here and below.

[alexmos, 2016/12/12 21:11:40]

I don't think you need to check if (web_contents) anymore.  If the web_contents
is null, your test will catch it when comparing the WebContents.

[davidsac (gone - try alexmos), 2016/12/13 23:02:32]

Done.

[davidsac (gone - try alexmos), 2016/12/13 23:02:32]

Done.

+      has_triggered_external_protocol_ = true;
+    }
+    if (message_loop_runner_) {
+      message_loop_runner_->Quit();
+    }
+  }
+
+  void Wait() {
+    if (has_triggered_external_protocol_) {
+      LOG(INFO) << "was going to start waiting, but it wasn't necessary!";
+    } else {
+      LOG(INFO) << "starting wait";
+      message_loop_runner_ = new content::MessageLoopRunner();
+      message_loop_runner_->Run();
+    }
+  }
+
+  void FinishedProcessingCheck() override {}
+
+ private:
+  bool has_triggered_external_protocol_ = false;
+  std::string url_path_ = "";

[alexmos, 2016/12/12 21:11:40]

no need for this assignment, std::string is going to be empty by default.  But
also see my earlier comment; I think you can just make this "GURL url_;"

[davidsac (gone - try alexmos), 2016/12/13 23:02:32]

Done.

+  content::WebContents* web_contents_ = nullptr;
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+};
+
+// This test verifies that external protocol requests succeed when made from a
+// OOPIF (https://crbug.com/668289).
+IN_PROC_BROWSER_TEST_F(ChromeSitePerProcessTest,
+                       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.
+

[alexmos, 2016/12/12 21:11:39]

nit: no blank line.

[davidsac (gone - try alexmos), 2016/12/13 23:02:32]

Done.

+  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"));
+

[alexmos, 2016/12/12 21:11:39]

nit: no blank line (this whole block is about navigating the subframe)

[davidsac (gone - try alexmos), 2016/12/13 23:02:32]

Done.

+  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.getURL(), "mail@example.org");

[alexmos, 2016/12/12 21:11:39]

Let's also validate that the scheme of the URL is in fact "mailto".  You could
do that just by checking the whole URL against GURL("mailto:mail@example.org").

[davidsac (gone - try alexmos), 2016/12/13 23:02:32]

Done.

+  EXPECT_EQ(active_web_contents, delegate.getWebContents());
+  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.
 IN_PROC_BROWSER_TEST_F(ChromeSitePerProcessTest,
                        NavigateRemoteFrameAndOpenPopup) {
   // Start on a page with an <iframe>.
   GURL main_url(embedded_test_server()->GetURL("a.com", "/iframe.html"));
   ui_test_utils::NavigateToURL(browser(), main_url);
 
   // Navigate the iframe cross-site.
@@ skipping 15 matching lines @@
       "document.querySelector('iframe').src = '" + frame_url.spec() + "';\n"
       "var w = window.open('about:blank');\n"
       "window.domAutomationController.send(!!w);\n",
       &popup_handle_is_valid));
   popup_observer.Wait();
 
   // The popup shouldn't be blocked.
   EXPECT_TRUE(popup_handle_is_valid);
   ASSERT_EQ(2, browser()->tab_strip_model()->count());
 }