Make chrome.proxy.onProxyError extension API work under the Network Service.

This adds an optional "ProxyErrorClient" client to the NetworkContext which
receives notifications of errors in the PAC script, as well as notifications
of failed URL loads that may have been proxy related.

(Reland of d576f34a).

Bug: 876568,851609,879491
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I08987d658879913d94c9b13b6fdc1de2a32a6d54
Reviewed-on: https://chromium-review.googlesource.com/1200076
Commit-Queue: Eric Roman <eroman@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588259}

chrome/browser/net/chrome_extensions_network_delegate.cc

@@ -100,7 +100,6 @@ class ChromeExtensionsNetworkDelegateImpl
 
  private:
   // ChromeExtensionsNetworkDelegate implementation.
-  void ForwardProxyErrors(net::URLRequest* request, int net_error) override;
   void ForwardStartRequestStatus(net::URLRequest* request) override;
   void ForwardDoneRequestStatus(net::URLRequest* request) override;
   int OnBeforeURLRequest(net::URLRequest* request,
@@ -124,7 +123,6 @@ class ChromeExtensionsNetworkDelegateImpl
                    bool started,
                    int net_error) override;
   void OnURLRequestDestroyed(net::URLRequest* request) override;
-  void OnPACScriptError(int line_number, const base::string16& error) override;
   net::NetworkDelegate::AuthRequiredResponse OnAuthRequired(
       net::URLRequest* request,
       const net::AuthChallengeInfo& auth_info,
@@ -159,19 +157,6 @@ ChromeExtensionsNetworkDelegateImpl::~ChromeExtensionsNetworkDelegateImpl() {
   DCHECK(active_requests_.empty());
 }
 
-void ChromeExtensionsNetworkDelegateImpl::ForwardProxyErrors(
-    net::URLRequest* request,
-    int net_error) {
-  if (net_error != net::OK) {
-    switch (net_error) {
-      case net::ERR_PROXY_AUTH_UNSUPPORTED:
-      case net::ERR_PROXY_CONNECTION_FAILED:
-      case net::ERR_TUNNEL_CONNECTION_FAILED:
-        extensions::ProxyEventRouter::GetInstance()->OnProxyError(
-            event_router_.get(), profile_, net_error);
-    }
-  }
-}
 
 void ChromeExtensionsNetworkDelegateImpl::ForwardStartRequestStatus(
     net::URLRequest* request) {
@@ -253,7 +238,6 @@ void ChromeExtensionsNetworkDelegateImpl::OnResponseStarted(
   info->AddResponseInfoFromURLRequest(request);
   ExtensionWebRequestEventRouter::GetInstance()->OnResponseStarted(
       profile_, extension_info_map_.get(), info, net_error);
-  ForwardProxyErrors(request, net_error);
 }
 
 void ChromeExtensionsNetworkDelegateImpl::OnCompleted(net::URLRequest* request,
@@ -286,13 +270,6 @@ void ChromeExtensionsNetworkDelegateImpl::OnURLRequestDestroyed(
   active_requests_.erase(it);
 }
 
-void ChromeExtensionsNetworkDelegateImpl::OnPACScriptError(
-    int line_number,
-    const base::string16& error) {
-  extensions::ProxyEventRouter::GetInstance()->OnPACScriptError(
-      event_router_.get(), profile_, line_number, error);
-}
-
 net::NetworkDelegate::AuthRequiredResponse
 ChromeExtensionsNetworkDelegateImpl::OnAuthRequired(
     net::URLRequest* request,
@@ -333,10 +310,6 @@ void ChromeExtensionsNetworkDelegate::set_extension_info_map(
 #endif
 }
 
-void ChromeExtensionsNetworkDelegate::ForwardProxyErrors(
-    net::URLRequest* request,
-    int net_error) {}
-
 void ChromeExtensionsNetworkDelegate::ForwardStartRequestStatus(
     net::URLRequest* request) {
 }

chrome/browser/net/chrome_extensions_network_delegate.h

@@ -35,10 +35,6 @@ class ChromeExtensionsNetworkDelegate : public net::NetworkDelegateImpl {
     profile_ = profile;
   }
 
-  // If the |request| failed due to problems with a proxy, forward the error to
-  // the proxy extension API.
-  virtual void ForwardProxyErrors(net::URLRequest* request, int net_error);
-
   // Notifies the extensions::ProcessManager for the associated RenderFrame, if
   // any, that a request has started or stopped.
   virtual void ForwardStartRequestStatus(net::URLRequest* request);

chrome/browser/net/chrome_network_delegate.cc

@@ -245,7 +245,6 @@ void ChromeNetworkDelegate::OnCompleted(net::URLRequest* request,
   extensions_delegate_->NotifyCompleted(request, started, net_error);
   if (domain_reliability_monitor_)
     domain_reliability_monitor_->OnCompleted(request, started);
-  extensions_delegate_->ForwardProxyErrors(request, net_error);
   extensions_delegate_->ForwardDoneRequestStatus(request);
 }
 
@@ -253,11 +252,6 @@ void ChromeNetworkDelegate::OnURLRequestDestroyed(net::URLRequest* request) {
   extensions_delegate_->NotifyURLRequestDestroyed(request);
 }
 
-void ChromeNetworkDelegate::OnPACScriptError(int line_number,
-                                             const base::string16& error) {
-  extensions_delegate_->NotifyPACScriptError(line_number, error);
-}
-
 net::NetworkDelegate::AuthRequiredResponse
 ChromeNetworkDelegate::OnAuthRequired(net::URLRequest* request,
                                       const net::AuthChallengeInfo& auth_info,

chrome/browser/net/chrome_network_delegate.h

@@ -131,7 +131,6 @@ class ChromeNetworkDelegate : public net::NetworkDelegateImpl {
                    bool started,
                    int net_error) override;
   void OnURLRequestDestroyed(net::URLRequest* request) override;
-  void OnPACScriptError(int line_number, const base::string16& error) override;
   net::NetworkDelegate::AuthRequiredResponse OnAuthRequired(
       net::URLRequest* request,
       const net::AuthChallengeInfo& auth_info,

chrome/browser/net/proxy_config_monitor.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/net/proxy_config_monitor.h"
 
+#include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/net/proxy_service_factory.h"
@@ -16,10 +17,18 @@
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #endif  // defined(OS_CHROMEOS)
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+#include "chrome/browser/extensions/api/proxy/proxy_api.h"
+#endif
+
 ProxyConfigMonitor::ProxyConfigMonitor(Profile* profile) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(profile);
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+  profile_ = profile;
+#endif
+
 // If this is the ChromeOS sign-in profile, just create the tracker from global
 // state.
 #if defined(OS_CHROMEOS)
@@ -68,9 +77,15 @@ void ProxyConfigMonitor::AddToNetworkContextParams(
       mojo::MakeRequest(&proxy_config_client);
   proxy_config_client_set_.AddPtr(std::move(proxy_config_client));
 
-  binding_set_.AddBinding(
+  poller_binding_set_.AddBinding(
       this,
       mojo::MakeRequest(&network_context_params->proxy_config_poller_client));
+
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+  error_binding_set_.AddBinding(
+      this, mojo::MakeRequest(&network_context_params->proxy_error_client));
+#endif
+
   net::ProxyConfigWithAnnotation proxy_config;
   net::ProxyConfigService::ConfigAvailability availability =
       proxy_config_service_->GetLatestProxyConfig(&proxy_config);
@@ -107,3 +122,29 @@ void ProxyConfigMonitor::OnProxyConfigChanged(
 void ProxyConfigMonitor::OnLazyProxyConfigPoll() {
   proxy_config_service_->OnLazyPoll();
 }
+
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+void ProxyConfigMonitor::OnPACScriptError(int32_t line_number,
+                                          const std::string& details) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  extensions::ProxyEventRouter::GetInstance()->OnPACScriptError(
+      g_browser_process->extension_event_router_forwarder(), profile_,
+      line_number, base::UTF8ToUTF16(details));
+}
+
+void ProxyConfigMonitor::OnRequestMaybeFailedDueToProxySettings(
+    int32_t net_error) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  if (net_error >= 0) {
+    // If the error is obviously wrong, don't dispatch it to extensions. If the
+    // PAC executor process is compromised, then |net_error| could be attacker
+    // controlled.
+    return;
+  }
+
+  extensions::ProxyEventRouter::GetInstance()->OnProxyError(
+      g_browser_process->extension_event_router_forwarder(), profile_,
+      net_error);
+}
+#endif

chrome/browser/net/proxy_config_monitor.h

@@ -8,6 +8,8 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "build/buildflag.h"
+#include "extensions/buildflags/buildflags.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "mojo/public/cpp/bindings/interface_ptr_set.h"
 #include "net/proxy_resolution/proxy_config_service.h"
@@ -22,9 +24,17 @@ class Profile;
 class PrefProxyConfigTracker;
 
 // Tracks the ProxyConfig to use, and passes any updates to a NetworkContext's
-// ProxyConfigClient.
+// ProxyConfigClient. This also responds to errors related to proxy settings
+// from the NetworkContext, and forwards them any listening Chrome extensions
+// associated with the profile.
 class ProxyConfigMonitor : public net::ProxyConfigService::Observer,
-                           public network::mojom::ProxyConfigPollerClient {
+                           public network::mojom::ProxyConfigPollerClient
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+    ,
+                           public network::mojom::ProxyErrorClient
+#endif
+
+{
  public:
   // Creates a ProxyConfigMonitor that gets proxy settings from |profile| and
   // watches for changes. The created ProxyConfigMonitor must be destroyed
@@ -60,15 +70,27 @@ class ProxyConfigMonitor : public net::ProxyConfigService::Observer,
   // network::mojom::ProxyConfigPollerClient implementation:
   void OnLazyProxyConfigPoll() override;
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+  // network::mojom::ProxyErrorClient implementation:
+  void OnPACScriptError(int32_t line_number,
+                        const std::string& details) override;
+  void OnRequestMaybeFailedDueToProxySettings(int32_t net_error) override;
+#endif
+
   std::unique_ptr<net::ProxyConfigService> proxy_config_service_;
   // Monitors global and Profile prefs related to proxy configuration.
   std::unique_ptr<PrefProxyConfigTracker> pref_proxy_config_tracker_;
 
-  mojo::BindingSet<network::mojom::ProxyConfigPollerClient> binding_set_;
+  mojo::BindingSet<network::mojom::ProxyConfigPollerClient> poller_binding_set_;
 
   mojo::InterfacePtrSet<network::mojom::ProxyConfigClient>
       proxy_config_client_set_;
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+  mojo::BindingSet<network::mojom::ProxyErrorClient> error_binding_set_;
+  Profile* profile_ = nullptr;
+#endif
+
   DISALLOW_COPY_AND_ASSIGN(ProxyConfigMonitor);
 };
 

net/base/layered_network_delegate.cc

@@ -109,12 +109,12 @@ void LayeredNetworkDelegate::OnBeforeRedirectInternal(
 
 void LayeredNetworkDelegate::OnResponseStarted(URLRequest* request,
                                                int net_error) {
-  OnResponseStartedInternal(request);
+  OnResponseStartedInternal(request, net_error);
   nested_network_delegate_->NotifyResponseStarted(request, net_error);
 }
 
-void LayeredNetworkDelegate::OnResponseStartedInternal(URLRequest* request) {
-}
+void LayeredNetworkDelegate::OnResponseStartedInternal(URLRequest* request,
+                                                       int net_error) {}
 
 void LayeredNetworkDelegate::OnNetworkBytesReceived(URLRequest* request,
                                                     int64_t bytes_received) {

net/base/layered_network_delegate.h

@@ -134,7 +134,7 @@ class NET_EXPORT LayeredNetworkDelegate : public NetworkDelegate {
   virtual void OnBeforeRedirectInternal(URLRequest* request,
                                         const GURL& new_location);
 
-  virtual void OnResponseStartedInternal(URLRequest* request);
+  virtual void OnResponseStartedInternal(URLRequest* request, int net_error);
 
   virtual void OnNetworkBytesReceivedInternal(URLRequest* request,
                                               int64_t bytes_received);

net/base/layered_network_delegate_unittest.cc

@@ -256,7 +256,7 @@ class TestLayeredNetworkDelegate : public LayeredNetworkDelegate {
     EXPECT_EQ(1, (*counters_)["on_before_redirect_count"]);
   }
 
-  void OnResponseStartedInternal(URLRequest* request) override {
+  void OnResponseStartedInternal(URLRequest* request, int net_error) override {
     ++(*counters_)["on_response_started_count"];
     EXPECT_EQ(1, (*counters_)["on_response_started_count"]);
   }

services/network/network_context.cc

@@ -18,6 +18,7 @@
 #include "base/sequenced_task_runner.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
 #include "build/build_config.h"
@@ -242,12 +243,16 @@ class NetworkContext::ContextNetworkDelegate
       std::unique_ptr<net::NetworkDelegate> nested_network_delegate,
       bool enable_referrers,
       bool validate_referrer_policy_on_initial_request,
+      mojom::ProxyErrorClientPtrInfo proxy_error_client_info,
       NetworkContext* network_context)
       : LayeredNetworkDelegate(std::move(nested_network_delegate)),
         enable_referrers_(enable_referrers),
         validate_referrer_policy_on_initial_request_(
             validate_referrer_policy_on_initial_request),
-        network_context_(network_context) {}
+        network_context_(network_context) {
+    if (proxy_error_client_info)
+      proxy_error_client_.Bind(std::move(proxy_error_client_info));
+  }
 
   ~ContextNetworkDelegate() override {}
 
@@ -284,6 +289,8 @@ class NetworkContext::ContextNetworkDelegate
             "Net.HttpRequestCompletionErrorCodes.MainFrame", -net_error);
       }
     }
+
+    ForwardProxyErrors(net_error);
   }
 
   bool OnCancelURLRequestWithPolicyViolatingReferrerHeaderInternal(
@@ -338,13 +345,43 @@ class NetworkContext::ContextNetworkDelegate
                 .IsCookieAccessAllowed(url, site_for_cookies);
   }
 
+  void OnResponseStartedInternal(net::URLRequest* request,
+                                 int net_error) override {
+    ForwardProxyErrors(net_error);
+  }
+
+  void OnPACScriptErrorInternal(int line_number,
+                                const base::string16& error) override {
+    if (!proxy_error_client_)
+      return;
+
+    proxy_error_client_->OnPACScriptError(line_number,
+                                          base::UTF16ToUTF8(error));
+  }
+
   void set_enable_referrers(bool enable_referrers) {
     enable_referrers_ = enable_referrers;
   }
 
  private:
+  void ForwardProxyErrors(int net_error) {
+    if (!proxy_error_client_)
+      return;
+
+    // TODO(https://crbug.com/876848): Provide justification for the currently
+    // enumerated errors.
+    switch (net_error) {
+      case net::ERR_PROXY_AUTH_UNSUPPORTED:
+      case net::ERR_PROXY_CONNECTION_FAILED:
+      case net::ERR_TUNNEL_CONNECTION_FAILED:
+        proxy_error_client_->OnRequestMaybeFailedDueToProxySettings(net_error);
+        break;
+    }
+  }
+
   bool enable_referrers_;
   bool validate_referrer_policy_on_initial_request_;
+  mojom::ProxyErrorClientPtr proxy_error_client_;
   NetworkContext* network_context_;
 
   DISALLOW_COPY_AND_ASSIGN(ContextNetworkDelegate);
@@ -1202,6 +1239,7 @@ URLRequestContextOwner NetworkContext::ApplyContextParamsToBuilder(
                 network_context_params->enable_referrers,
                 network_context_params
                     ->validate_referrer_policy_on_initial_request,
+                std::move(network_context_params->proxy_error_client),
                 network_context);
         if (out_context_network_delegate)
           *out_context_network_delegate = context_network_delegate.get();

services/network/public/mojom/network_context.mojom

@@ -149,6 +149,10 @@ struct NetworkContextParams {
   // ProxyConfigServices be modified not to need this notification?
   ProxyConfigPollerClient? proxy_config_poller_client;
 
+  // Optional client that will be notified of errors related to the proxy
+  // settings.
+  ProxyErrorClient? proxy_error_client;
+
   // When PAC quick checking is enabled, DNS lookups for PAC script's host are
   // timed out aggressively. This prevents hanging all network request on DNS
   // lookups that are slow or are blockholed, at the cost of making it more

services/network/public/mojom/proxy_config_with_annotation.mojom

@@ -22,4 +22,27 @@ interface ProxyConfigClient {
 // it might be a good time to double-check the proxy configuration.
 interface ProxyConfigPollerClient {
   OnLazyProxyConfigPoll();
-};
\ No newline at end of file
+};
+
+// Called to notify error related to the configured proxy settings.
+interface ProxyErrorClient {
+  // Called when the PAC script being used by the NetworkContext throws a
+  // JavaScript error or fails to execute. This error is not necessarily
+  // fatal for URL loading, since by default errors in a PAC script
+  // result in a fallback to DIRECT connections.
+  OnPACScriptError(int32 line_number, string details);
+
+  // This is a best effort notification that a URL request failed due to
+  // a problem with the proxy settings. |net_error| is the error code that the
+  // request failed with.
+  //
+  // This only surfaces failures for an entire URL load, and not from
+  // individual proxy servers. For instance if a PAC script returned 4 proxy
+  // servers, and sending the request through the first three failed before
+  // successfulyl sending through the fourth, this method is NOT called.
+  //
+  // There is some ambiguity with how errors are classified as being a
+  // "proxy error". The current implementation includes a mix of
+  // connection and protocol errors.
+  OnRequestMaybeFailedDueToProxySettings(int32 net_error);
+};

testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

@@ -91,15 +91,6 @@
 -ResourceLoadingHintsBrowserTest.ResourceLoadingHintsHttp
 -ResourceLoadingHintsBrowserTest.ResourceLoadingHintsHttpsNoWhitelisted
 
-# https://crbug.com/851609
-# Extension listeners for "chrome.proxy.onProxyError" are not called for errors
-# in PAC scripts.
--ProxySettingsApiTest.ProxyEventsParseError
-# https://crbug.com/876568
-# Extension listeners for "chrome.proxy.onProxyError" are not called for
-# failures connecting to proxy.
--ProxySettingsApiTest.ProxyEventsInvalidProxy
-
 # https://crbug.com/827582
 # Editing response cookies through headers with webRequest is not supported with
 # the network service.