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

This reverts commit d576f34a.

Reason for revert: Likely culprit of https://crbug.com/879491.

Original change's description:
> 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.
> 
> Bug: 876568,851609
> Cq-Include-Trybots: luci.chromium.try:linux_mojo
> Change-Id: I645ef91875135fcc81a7bd113442a0b65c661e65
> Reviewed-on: https://chromium-review.googlesource.com/1192123
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Reviewed-by: Will Harris <wfh@chromium.org>
> Commit-Queue: Eric Roman <eroman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#587812}

TBR=eroman@chromium.org,mmenke@chromium.org,wfh@chromium.org

Change-Id: I286c7f768b04b6e47f1836abc459a33c7b394498
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 876568, 851609
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1199043Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587966}

chrome/browser/net/chrome_extensions_network_delegate.cc

@@ -100,6 +100,7 @@ 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,
@@ -123,6 +124,7 @@ 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,
@@ -157,6 +159,19 @@ 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) {
@@ -238,6 +253,7 @@ 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,
@@ -270,6 +286,13 @@ 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,
@@ -310,6 +333,10 @@ 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,6 +35,10 @@ 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,6 +245,7 @@ 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);
 }
 
@@ -252,6 +253,11 @@ 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,6 +131,7 @@ 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,7 +4,6 @@
 
 #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"
@@ -17,18 +16,10 @@
 #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)
@@ -77,15 +68,9 @@ void ProxyConfigMonitor::AddToNetworkContextParams(
       mojo::MakeRequest(&proxy_config_client);
   proxy_config_client_set_.AddPtr(std::move(proxy_config_client));
 
-  poller_binding_set_.AddBinding(
+  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);
@@ -122,29 +107,3 @@ 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,8 +8,6 @@
 #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"
@@ -24,17 +22,9 @@ class Profile;
 class PrefProxyConfigTracker;
 
 // Tracks the ProxyConfig to use, and passes any updates to a NetworkContext's
-// ProxyConfigClient. This also responds to errors related to proxy settings
-// from the NetworkContext, and forwards them any listening Chrome extensions
-// associated with the profile.
+// ProxyConfigClient.
 class ProxyConfigMonitor : public net::ProxyConfigService::Observer,
-                           public network::mojom::ProxyConfigPollerClient
-#if BUILDFLAG(ENABLE_EXTENSIONS)
-    ,
-                           public network::mojom::ProxyErrorClient
-#endif
-
-{
+                           public network::mojom::ProxyConfigPollerClient {
  public:
   // Creates a ProxyConfigMonitor that gets proxy settings from |profile| and
   // watches for changes. The created ProxyConfigMonitor must be destroyed
@@ -70,27 +60,15 @@ 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> poller_binding_set_;
+  mojo::BindingSet<network::mojom::ProxyConfigPollerClient> 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_;
-#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, net_error);
+  OnResponseStartedInternal(request);
   nested_network_delegate_->NotifyResponseStarted(request, net_error);
 }
 
-void LayeredNetworkDelegate::OnResponseStartedInternal(URLRequest* request,
-                                                       int net_error) {}
+void LayeredNetworkDelegate::OnResponseStartedInternal(URLRequest* request) {
+}
 
 void LayeredNetworkDelegate::OnNetworkBytesReceived(URLRequest* request,
                                                     int64_t bytes_received) {

net/base/layered_network_delegate.h

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

net/base/layered_network_delegate_unittest.cc

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

services/network/network_context.cc

@@ -18,7 +18,6 @@
 #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"
@@ -243,16 +242,12 @@ 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) {
-    if (proxy_error_client_info)
-      proxy_error_client_.Bind(std::move(proxy_error_client_info));
-  }
+        network_context_(network_context) {}
 
   ~ContextNetworkDelegate() override {}
 
@@ -289,8 +284,6 @@ class NetworkContext::ContextNetworkDelegate
             "Net.HttpRequestCompletionErrorCodes.MainFrame", -net_error);
       }
     }
-
-    ForwardProxyErrors(net_error);
   }
 
   bool OnCancelURLRequestWithPolicyViolatingReferrerHeaderInternal(
@@ -316,43 +309,13 @@ class NetworkContext::ContextNetworkDelegate
     return true;
   }
 
-  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);
@@ -1210,7 +1173,6 @@ 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,10 +149,6 @@ 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,27 +22,4 @@ interface ProxyConfigClient {
 // it might be a good time to double-check the proxy configuration.
 interface ProxyConfigPollerClient {
   OnLazyProxyConfigPoll();
-};
-
-// 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);
-};
+};
\ No newline at end of file

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

@@ -94,6 +94,15 @@
 -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.