Convert some NetError[Tab]Helper messages to mojom

Converts ChromeViewHostMsg_RunNetworkDiagnostics and
ChromeViewMsg_SetCanShowNetworkDiagnosticsDialg to mojom without
impacting message ordering.

The browser-side interface is exposed via a
WebContentsFrameBindingSet<T>.

The renderer-side (client) interface is exposed via
RenderFrameImpl's AssociatedInterfaceRegistry.

This CL serves as a simple example of converting routed IPC
messages which are handled by WebContentsObservers or other
WebContents-bound things.

BUG=612500

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

chrome/browser/net/net_error_tab_helper.cc

@@ -21,6 +21,7 @@
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/common/associated_interface_provider.h"
 #include "ipc/ipc_message_macros.h"
 #include "net/base/net_errors.h"
 #include "url/gurl.h"
@@ -98,10 +99,10 @@ void NetErrorTabHelper::RenderFrameCreated(
   // platform's network diagnostics dialog.
   if (render_frame_host->GetParent())
     return;
-  render_frame_host->Send(
-      new ChromeViewMsg_SetCanShowNetworkDiagnosticsDialog(
-          render_frame_host->GetRoutingID(),
-          CanShowNetworkDiagnosticsDialog()));
+
+  mojom::NetworkDiagnosticsClientAssociatedPtr client;
+  render_frame_host->GetRemoteAssociatedInterfaces()->GetInterface(&client);
+  client->SetCanShowNetworkDiagnosticsDialog(CanShowNetworkDiagnosticsDialog());
 }
 
 void NetErrorTabHelper::DidStartNavigationToPendingEntry(
@@ -181,21 +182,22 @@ bool NetErrorTabHelper::OnMessageReceived(
     content::RenderFrameHost* render_frame_host) {
   if (render_frame_host != web_contents()->GetMainFrame())
     return false;
+#if BUILDFLAG(ANDROID_JAVA_UI)
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(NetErrorTabHelper, message)
-    IPC_MESSAGE_HANDLER(ChromeViewHostMsg_RunNetworkDiagnostics,
-                        RunNetworkDiagnostics)
-#if BUILDFLAG(ANDROID_JAVA_UI)
     IPC_MESSAGE_HANDLER(ChromeViewHostMsg_ShowOfflinePages, ShowOfflinePages)
-#endif  // BUILDFLAG(ANDROID_JAVA_UI)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
 
   return handled;
+#else
+  return false;
+#endif  // BUILDFLAG(ANDROID_JAVA_UI)
 }
 
 NetErrorTabHelper::NetErrorTabHelper(WebContents* contents)
     : WebContentsObserver(contents),
+      network_diagnostics_bindings_(contents, this),
       is_error_page_(false),
       dns_error_active_(false),
       dns_error_page_committed_(false),
@@ -286,12 +288,18 @@ void NetErrorTabHelper::RunNetworkDiagnostics(const GURL& url) {
   // any other schemes, but the renderer is not trusted.
   if (!url.is_valid() || !url.SchemeIsHTTPOrHTTPS())
     return;
+
   // Sanitize URL prior to running diagnostics on it.
   RunNetworkDiagnosticsHelper(url.GetOrigin().spec());
 }
 
 void NetErrorTabHelper::RunNetworkDiagnosticsHelper(
     const std::string& sanitized_url) {
+  if (network_diagnostics_bindings_.GetCurrentTargetFrame()
+          != web_contents()->GetMainFrame()) {
+    return;
+  }
+
   ShowNetworkDiagnosticsDialog(web_contents(), sanitized_url);
 }
 

chrome/browser/net/net_error_tab_helper.h

@@ -13,9 +13,11 @@
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/net/dns_probe_service.h"
 #include "chrome/common/features.h"
+#include "chrome/common/network_diagnostics.mojom.h"
 #include "components/error_page/common/net_error_info.h"
 #include "components/prefs/pref_member.h"
 #include "content/public/browser/reload_type.h"
+#include "content/public/browser/web_contents_binding_set.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 
@@ -26,7 +28,8 @@ namespace chrome_browser_net {
 // DnsProbeService whenever a page fails to load with a DNS-related error.
 class NetErrorTabHelper
     : public content::WebContentsObserver,
-      public content::WebContentsUserData<NetErrorTabHelper> {
+      public content::WebContentsUserData<NetErrorTabHelper>,
+      public mojom::NetworkDiagnostics {
  public:
   enum TestingState {
     TESTING_DEFAULT,
@@ -76,7 +79,6 @@ class NetErrorTabHelper
   bool OnMessageReceived(const IPC::Message& message,
                          content::RenderFrameHost* render_frame_host) override;
 
-
  protected:
   // |contents| is the WebContents of the tab this NetErrorTabHelper is
   // attached to.
@@ -89,6 +91,11 @@ class NetErrorTabHelper
     return dns_probe_status_;
   }
 
+  content::WebContentsFrameBindingSet<mojom::NetworkDiagnostics>&
+  network_diagnostics_bindings_for_testing() {
+    return network_diagnostics_bindings_;
+  }
+
  private:
   friend class content::WebContentsUserData<NetErrorTabHelper>;
 
@@ -97,8 +104,8 @@ class NetErrorTabHelper
   void InitializePref(content::WebContents* contents);
   bool ProbesAllowed() const;
 
-  // Sanitizes |url| and shows a dialog for it.
-  void RunNetworkDiagnostics(const GURL& url);
+  // mojom::NetworkDiagnostics:
+  void RunNetworkDiagnostics(const GURL& url) override;
 
   // Shows the diagnostics dialog after its been sanitized, virtual for
   // testing.
@@ -112,6 +119,9 @@ class NetErrorTabHelper
   bool IsFromErrorPage() const;
 #endif  // BUILDFLAG(ANDROID_JAVA_UI)
 
+  content::WebContentsFrameBindingSet<mojom::NetworkDiagnostics>
+      network_diagnostics_bindings_;
+
   // True if the last provisional load that started was for an error page.
   bool is_error_page_;
 

chrome/browser/net/net_error_tab_helper_unittest.cc

@@ -43,6 +43,13 @@ class TestNetErrorTabHelper : public NetErrorTabHelper {
     return times_diagnostics_dialog_invoked_;
   }
 
+  void SetCurrentTargetFrame(content::RenderFrameHost* render_frame_host) {
+    network_diagnostics_bindings_for_testing().SetCurrentTargetFrameForTesting(
+        render_frame_host);
+  }
+
+  mojom::NetworkDiagnostics* network_diagnostics_interface() { return this; }
+
  private:
   // NetErrorTabHelper implementation:
 
@@ -420,10 +427,9 @@ TEST_F(NetErrorTabHelperTest, CoalesceFailures) {
 // Makes sure that URLs are sanitized before running the platform network
 // diagnostics tool.
 TEST_F(NetErrorTabHelperTest, SanitizeDiagnosticsUrl) {
-  content::RenderFrameHost* rfh = web_contents()->GetMainFrame();
-  rfh->OnMessageReceived(ChromeViewHostMsg_RunNetworkDiagnostics(
-      rfh->GetRoutingID(),
-      GURL("http://foo:bar@somewhere:123/hats?for#goats")));
+  tab_helper()->SetCurrentTargetFrame(web_contents()->GetMainFrame());
+  tab_helper()->network_diagnostics_interface()->RunNetworkDiagnostics(
+      GURL("http://foo:bar@somewhere:123/hats?for#goats"));
   EXPECT_EQ("http://somewhere:123/",
             tab_helper()->network_diagnostics_url());
   EXPECT_EQ(1, tab_helper()->times_diagnostics_dialog_invoked());
@@ -442,9 +448,9 @@ TEST_F(NetErrorTabHelperTest, NoDiagnosticsForNonHttpSchemes) {
   };
 
   for (const char* url : kUrls) {
-    content::RenderFrameHost* rfh = web_contents()->GetMainFrame();
-    rfh->OnMessageReceived(ChromeViewHostMsg_RunNetworkDiagnostics(
-        rfh->GetRoutingID(), GURL(url)));
+    tab_helper()->SetCurrentTargetFrame(web_contents()->GetMainFrame());
+    tab_helper()->network_diagnostics_interface()
+        ->RunNetworkDiagnostics(GURL(url));
     EXPECT_EQ(0, tab_helper()->times_diagnostics_dialog_invoked());
   }
 }

chrome/common/BUILD.gn

@@ -639,12 +639,14 @@ source_set("app_mode_app_support") {
 mojom("mojo_bindings") {
   sources = [
     "image_decoder.mojom",
+    "network_diagnostics.mojom",
     "resource_usage_reporter.mojom",
     "shell_handler_win.mojom",
   ]
 
   public_deps = [
     "//skia/public/interfaces",
+    "//url/mojo:url_mojom_gurl",
   ]
 
   use_new_wrapper_types = false

chrome/common/network_diagnostics.mojom

+// Copyright 2016 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.
+
+module mojom;
+
+import "url/mojo/url.mojom";
+
+interface NetworkDiagnostics {
+  RunNetworkDiagnostics(url.mojom.Url failed_url);
+};
+
+interface NetworkDiagnosticsClient {
+  // Tells the renderer whether or not there is a local diagnostics service that
+  // can be run via calls to NetworkDiagnostics::RunNetworkDiagnostics.
+  SetCanShowNetworkDiagnosticsDialog(bool can_show);
+};

chrome/common/render_messages.h

@@ -306,11 +306,6 @@ IPC_MESSAGE_CONTROL0(ChromeViewHostMsg_ShowBrowserAccountManagementUI)
 IPC_MESSAGE_ROUTED1(ChromeViewMsg_NetErrorInfo,
                     int /* DNS probe status */)
 
-// Tells the renderer whether or not there is a local diagnostics service that
-// can be run via ChromeViewHostMsg_RunNetworkDiagnostics messages.
-IPC_MESSAGE_ROUTED1(ChromeViewMsg_SetCanShowNetworkDiagnosticsDialog,
-                    bool /* can_show_network_diagnostics_dialog */)
-
 #if defined(OS_ANDROID)
 // Tells the renderer whether or not offline pages exist. This is used to
 // decide if offline related button will be provided on certain error page.
@@ -327,9 +322,6 @@ IPC_MESSAGE_ROUTED5(ChromeViewMsg_SetNavigationCorrectionInfo,
                     std::string /* API key to use */,
                     GURL /* Google Search URL to use */)
 
-IPC_MESSAGE_ROUTED1(ChromeViewHostMsg_RunNetworkDiagnostics,
-                    GURL /* failed_url */)
-
 #if defined(OS_ANDROID)
 // Message sent from the renderer to the browser to show the UI for offline
 // pages.

chrome/renderer/net/net_error_helper.cc

@@ -21,6 +21,8 @@
 #include "components/error_page/common/localized_error.h"
 #include "components/error_page/common/net_error_info.h"
 #include "components/grit/components_resources.h"
+#include "content/public/common/associated_interface_provider.h"
+#include "content/public/common/associated_interface_registry.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/renderer/content_renderer_client.h"
@@ -82,6 +84,7 @@ NetErrorHelperCore::FrameType GetFrameType(RenderFrame* render_frame) {
 NetErrorHelper::NetErrorHelper(RenderFrame* render_frame)
     : RenderFrameObserver(render_frame),
       content::RenderFrameObserverTracker<NetErrorHelper>(render_frame),
+      network_diagnostics_client_binding_(this),
       weak_controller_delegate_factory_(this) {
   RenderThread::Get()->AddObserver(this);
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
@@ -95,6 +98,10 @@ NetErrorHelper::NetErrorHelper(RenderFrame* render_frame)
                                      auto_reload_enabled,
                                      auto_reload_visible_only,
                                      !render_frame->IsHidden()));
+
+  render_frame->GetAssociatedInterfaceRegistry()->AddInterface(
+      base::Bind(&NetErrorHelper::OnNetworkDiagnosticsClientRequest,
+                 base::Unretained(this)));
 }
 
 NetErrorHelper::~NetErrorHelper() {
@@ -152,8 +159,6 @@ bool NetErrorHelper::OnMessageReceived(const IPC::Message& message) {
 
   IPC_BEGIN_MESSAGE_MAP(NetErrorHelper, message)
     IPC_MESSAGE_HANDLER(ChromeViewMsg_NetErrorInfo, OnNetErrorInfo)
-    IPC_MESSAGE_HANDLER(ChromeViewMsg_SetCanShowNetworkDiagnosticsDialog,
-                        OnSetCanShowNetworkDiagnosticsDialog);
     IPC_MESSAGE_HANDLER(ChromeViewMsg_SetNavigationCorrectionInfo,
                         OnSetNavigationCorrectionInfo);
 #if defined(OS_ANDROID)
@@ -185,6 +190,14 @@ bool NetErrorHelper::ShouldSuppressErrorPage(const GURL& url) {
   return core_->ShouldSuppressErrorPage(GetFrameType(render_frame()), url);
 }
 
+mojom::NetworkDiagnostics* NetErrorHelper::GetRemoteNetworkDiagnostics() {
+  if (!remote_network_diagnostics_) {
+    render_frame()->GetRemoteAssociatedInterfaces()
+        ->GetInterface(&remote_network_diagnostics_);
+  }
+  return remote_network_diagnostics_.get();
+}
+
 void NetErrorHelper::GenerateLocalizedErrorPage(
     const blink::WebURLError& error,
     bool is_failed_post,
@@ -322,8 +335,7 @@ void NetErrorHelper::LoadPageFromCache(const GURL& page_url) {
 }
 
 void NetErrorHelper::DiagnoseError(const GURL& page_url) {
-  render_frame()->Send(new ChromeViewHostMsg_RunNetworkDiagnostics(
-      render_frame()->GetRoutingID(), page_url));
+  GetRemoteNetworkDiagnostics()->RunNetworkDiagnostics(page_url);
 }
 
 void NetErrorHelper::ShowOfflinePages() {
@@ -341,12 +353,6 @@ void NetErrorHelper::OnNetErrorInfo(int status_num) {
   core_->OnNetErrorInfo(static_cast<DnsProbeStatus>(status_num));
 }
 
-void NetErrorHelper::OnSetCanShowNetworkDiagnosticsDialog(
-    bool can_use_local_diagnostics_service) {
-  core_->OnSetCanShowNetworkDiagnosticsDialog(
-      can_use_local_diagnostics_service);
-}
-
 void NetErrorHelper::OnSetNavigationCorrectionInfo(
     const GURL& navigation_correction_url,
     const std::string& language,
@@ -375,6 +381,16 @@ void NetErrorHelper::OnTrackingRequestComplete(
   tracking_fetcher_.reset();
 }
 
+void NetErrorHelper::OnNetworkDiagnosticsClientRequest(
+    mojom::NetworkDiagnosticsClientAssociatedRequest request) {
+  DCHECK(!network_diagnostics_client_binding_.is_bound());
+  network_diagnostics_client_binding_.Bind(std::move(request));
+}
+
+void NetErrorHelper::SetCanShowNetworkDiagnosticsDialog(bool can_show) {
+  core_->OnSetCanShowNetworkDiagnosticsDialog(can_show);
+}
+
 #if defined(OS_ANDROID)
 void NetErrorHelper::OnSetHasOfflinePages(bool has_offline_pages) {
   core_->OnSetHasOfflinePages(has_offline_pages);

chrome/renderer/net/net_error_helper.h

@@ -11,12 +11,14 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "build/build_config.h"
+#include "chrome/common/network_diagnostics.mojom.h"
 #include "chrome/renderer/net/net_error_page_controller.h"
 #include "components/error_page/common/net_error_info.h"
 #include "components/error_page/renderer/net_error_helper_core.h"
 #include "content/public/renderer/render_frame_observer.h"
 #include "content/public/renderer/render_frame_observer_tracker.h"
 #include "content/public/renderer/render_thread_observer.h"
+#include "mojo/public/cpp/bindings/associated_binding.h"
 
 class GURL;
 
@@ -44,7 +46,8 @@ class NetErrorHelper
       public content::RenderFrameObserverTracker<NetErrorHelper>,
       public content::RenderThreadObserver,
       public error_page::NetErrorHelperCore::Delegate,
-      public NetErrorPageController::Delegate {
+      public NetErrorPageController::Delegate,
+      public mojom::NetworkDiagnosticsClient {
  public:
   explicit NetErrorHelper(content::RenderFrame* render_frame);
   ~NetErrorHelper() override;
@@ -82,6 +85,8 @@ class NetErrorHelper
   bool ShouldSuppressErrorPage(const GURL& url);
 
  private:
+  mojom::NetworkDiagnostics* GetRemoteNetworkDiagnostics();
+
   // NetErrorHelperCore::Delegate implementation:
   void GenerateLocalizedErrorPage(
       const blink::WebURLError& error,
@@ -112,8 +117,6 @@ class NetErrorHelper
   void ShowOfflinePages() override;
 
   void OnNetErrorInfo(int status);
-  void OnSetCanShowNetworkDiagnosticsDialog(
-      bool can_use_local_diagnostics_service);
   void OnSetNavigationCorrectionInfo(const GURL& navigation_correction_url,
                                      const std::string& language,
                                      const std::string& country_code,
@@ -126,6 +129,12 @@ class NetErrorHelper
   void OnTrackingRequestComplete(const blink::WebURLResponse& response,
                                  const std::string& data);
 
+  void OnNetworkDiagnosticsClientRequest(
+      mojom::NetworkDiagnosticsClientAssociatedRequest request);
+
+  // mojom::NetworkDiagnosticsClient:
+  void SetCanShowNetworkDiagnosticsDialog(bool can_show) override;
+
 #if defined(OS_ANDROID)
   // Called to set whether offline pages exists, which will be used to decide
   // if offline related button will be provided in the error page.
@@ -137,6 +146,10 @@ class NetErrorHelper
 
   std::unique_ptr<error_page::NetErrorHelperCore> core_;
 
+  mojo::AssociatedBinding<mojom::NetworkDiagnosticsClient>
+      network_diagnostics_client_binding_;
+  mojom::NetworkDiagnosticsAssociatedPtr remote_network_diagnostics_;
+
   // Weak factory for vending a weak pointer to a NetErrorPageController. Weak
   // pointers are invalidated on each commit, to prevent getting messages from
   // Controllers used for the previous commit that haven't yet been cleaned up.