Track sources of proxy settings.

net/proxy/proxy_service.cc

 // Copyright (c) 2012 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 "net/proxy/proxy_service.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 146 matching lines @@
 
 // Config getter that always returns direct settings.
 class ProxyConfigServiceDirect : public ProxyConfigService {
  public:
   // ProxyConfigService implementation:
   virtual void AddObserver(Observer* observer) OVERRIDE {}
   virtual void RemoveObserver(Observer* observer) OVERRIDE {}
   virtual ConfigAvailability GetLatestProxyConfig(ProxyConfig* config)
       OVERRIDE {
     *config = ProxyConfig::CreateDirect();
+    config->set_source(PROXY_CONFIG_SOURCE_NONE);
     return CONFIG_VALID;
   }
 };
 
 // Proxy resolver that fails every time.
 class ProxyResolverNull : public ProxyResolver {
  public:
   ProxyResolverNull() : ProxyResolver(false /*expects_pac_bytes*/) {}
 
   // ProxyResolver implementation.
@@ skipping 615 matching lines @@
              const GURL& url,
              ProxyInfo* results,
              const net::CompletionCallback& user_callback,
              const BoundNetLog& net_log)
       : service_(service),
         user_callback_(user_callback),
         results_(results),
         url_(url),
         resolve_job_(NULL),
         config_id_(ProxyConfig::kInvalidConfigID),
+        config_source_(PROXY_CONFIG_SOURCE_UNKNOWN),
         net_log_(net_log) {
     DCHECK(!user_callback.is_null());
   }
 
   // Starts the resolve proxy request.
   int Start() {
     DCHECK(!was_cancelled());
     DCHECK(!is_started());
 
     DCHECK(service_->config_.is_valid());
 
     config_id_ = service_->config_.id();
+    config_source_ = service_->config_.source();
 
     return resolver()->GetProxyForURL(
         url_, results_,
         base::Bind(&PacRequest::QueryComplete, base::Unretained(this)),
         &resolve_job_, net_log_);
   }
 
   bool is_started() const {
     // Note that !! casts to bool. (VS gives a warning otherwise).
     return !!resolve_job_;
@@ skipping 35 matching lines @@
   }
 
   // Helper to call after ProxyResolver completion (both synchronous and
   // asynchronous). Fixes up the result that is to be returned to user.
   int QueryDidComplete(int result_code) {
     DCHECK(!was_cancelled());
 
     // Make a note in the results which configuration was in use at the
     // time of the resolve.
     results_->config_id_ = config_id_;
+    results_->config_source_ = config_source_;
 
     // Reset the state associated with in-progress-resolve.
     resolve_job_ = NULL;
     config_id_ = ProxyConfig::kInvalidConfigID;
+    config_source_ = PROXY_CONFIG_SOURCE_UNKNOWN;
 
     return service_->DidFinishResolvingProxy(results_, result_code, net_log_);
   }
 
   BoundNetLog* net_log() { return &net_log_; }
 
   LoadState GetLoadState() const {
     if (is_started())
       return resolver()->GetLoadState(resolve_job_);
     return LOAD_STATE_RESOLVING_PROXY_FOR_URL;
@@ skipping 21 matching lines @@
 
   // Note that we don't hold a reference to the ProxyService. Outstanding
   // requests are cancelled during ~ProxyService, so this is guaranteed
   // to be valid throughout our lifetime.
   ProxyService* service_;
   net::CompletionCallback user_callback_;
   ProxyInfo* results_;
   GURL url_;
   ProxyResolver::RequestHandle resolve_job_;
   ProxyConfig::ID config_id_;  // The config id when the resolve was started.
+  ProxyConfigSource config_source_;  // The source of proxy settings.
   BoundNetLog net_log_;
 };
 
 // ProxyService ---------------------------------------------------------------
 
 ProxyService::ProxyService(ProxyConfigService* config_service,
                            ProxyResolver* resolver,
                            NetLog* net_log)
     : resolver_(resolver),
       next_config_id_(1),
@@ skipping 181 matching lines @@
 
   // If it was impossible to fetch or parse the PAC script, we cannot complete
   // the request here and bail out.
   if (permanent_error_ != OK)
     return permanent_error_;
 
   if (config_.HasAutomaticSettings())
     return ERR_IO_PENDING;  // Must submit the request to the proxy resolver.
 
   // Use the manual proxy settings.
-  config_.proxy_rules().Apply(url, result);
+  bool did_use_proxy_rules = config_.proxy_rules().Apply(url, result);
+  result->config_source_ =
+      (did_use_proxy_rules ? config_.source() : PROXY_CONFIG_SOURCE_NONE);

[eroman, 2012/06/02 02:06:10]

I still don't agree with this layering.

This makes it seem like somehow Apply failed to apply the rules and we are
falling back. In doing this we lose track of the source where these setting
originated from.

However the fact that we ended up using direct was itself a "rule", not a
failure.

I think there are two concepts trying to be merged into one here.

  (1) There is the concept of the "source", which identifies where the
underlying settings originated from. This basically tracks the internals of the
ProxyConfigService.

  (2) There is the concept of whether we arrived at a DIRECT connection by way
of a bypass rule.


Basically what I am asking for here is to keep these two separate. How about
mirroring config_.source() directly to result->config_source_ so it preserves
the origin of the settings.

The second bit of information could be whether we applied a bypass rule. I think
the bypass case in ProxyConfig is the only one you actually need to care about,
since the higher level check would be listsize == 1 && isdirect &&
proxyconfig.was_from_bypass.

Lastly, if you keep the "was from bypass" bit as an output from Apply(), I think
it should either be a named out-parameter instead of return value to make it
clear that it does not reflect the success of the operation. Or it should be
directly set into |result| as part of Apply(), and not be an extra parameter.

Sorry for taking so long on this review, I have had a hard time understanding
this and expressing what my concerns are :) But I think we are pretty close now.

Feel free to grab me on IM or on the phone if we can't get this finalized
quickly!

   result->config_id_ = config_.id();
   return OK;
 }
 
 ProxyService::~ProxyService() {
   NetworkChangeNotifier::RemoveIPAddressObserver(this);
   config_service_->RemoveObserver(this);
 
   // Cancel any inprogress requests.
   for (PendingRequests::iterator it = pending_requests_.begin();
@@ skipping 101 matching lines @@
       config_ = fetched_config_;
       config_.ClearAutomaticSettings();
       result = OK;
     }
   }
   permanent_error_ = result;
 
   // TODO(eroman): Make this ID unique in the case where configuration changed
   //               due to ProxyScriptDeciderPoller.
   config_.set_id(fetched_config_.id());
+  config_.set_source(fetched_config_.source());
 
   // Resume any requests which we had to defer until the PAC script was
   // downloaded.
   SetReady();
 }
 
 int ProxyService::ReconsiderProxyAfterError(const GURL& url,
                                             ProxyInfo* result,
                                             const CompletionCallback& callback,
                                             PacRequest** pac_request,
@@ skipping 393 matching lines @@
     OnCompletion(result_);
   }
 }
 
 void SyncProxyServiceHelper::OnCompletion(int rv) {
   result_ = rv;
   event_.Signal();
 }
 
 }  // namespace net