Poll PAC scripts for content changes.

net/proxy/proxy_service.cc

Index: net/proxy/proxy_service.cc
diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index c8193b4dabdb3a79ce65f1059910c8eaf556b596..af0d44148da76349822b27031553e3daf6653b28 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -65,29 +65,10 @@ const size_t kDefaultNumPacThreads = 4;
 // we were to run proxy auto-config right away, it could fail due to spurious
 // DNS failures. (see http://crbug.com/50779 for more details.)
 //
-// By adding the wait window, we give things a chance to get properly set up.
-// Now by the time we run the proxy-autoconfig there is a lower chance of
-// getting transient DNS / connect failures.
-//
-// Admittedly this is a hack. Ideally we would have NetworkChangeNotifier
-// deliver a reliable signal indicating that the network has changed AND is
-// ready for action... But until then, we can reduce the likelihood of users
-// getting wedged because of proxy detection failures on network switch.
-//
-// The obvious downside to this strategy is it introduces an additional
-// latency when switching networks. This delay shouldn't be too disruptive
-// assuming network switches are infrequent and user initiated. However if
-// NetworkChangeNotifier delivers network changes more frequently this could
-// cause jankiness. (NetworkChangeNotifier broadcasts a change event when ANY
-// interface goes up/down. So in theory if the non-primary interface were
-// hopping on and off wireless networks our constant delayed reconfiguration
-// could add noticeable jank.)
-//
-// The specific hard-coded wait time below is arbitrary.
-// Basically I ran some experiments switching between wireless networks on
-// a Linux Ubuntu (Lucid) laptop, and experimentally found this timeout fixes
-// things. It is entirely possible that the value is insufficient for other
-// setups.
+// By adding the wait window, we give things a better chance to get properly
+// set up. Network failures can happen at any time though, so we additionally
+// poll the PAC script for changes, which will allow us to recover from these
+// sorts of problems.
 const int64 kNumMillisToStallAfterNetworkChanges = 2000;
 
 // Config getter that always returns direct settings.
@@ -323,18 +304,16 @@ class BadProxyListNetLogParam : public NetLog::EventParameters {
 //       figure out what we should configure against.
 //   (2) Feed the fetched PAC script into the ProxyResolver.
 //
-// TODO(eroman): This is something of a temporary shim while refactoring to keep
-// things similar. It will probably end up changing while solving bug 90581.
+// InitProxyResolver is a single-use class which encapsulates cancellation as
+// part of its destructor. Start() or StartSkipDecider() should be called just
+// once. The instance can be destroyed at any time, and the request will be
+// cancelled.
 
 class ProxyService::InitProxyResolver {
  public:
-  InitProxyResolver(ProxyResolver* proxy_resolver,
-                    ProxyScriptFetcher* proxy_script_fetcher,
-                    DhcpProxyScriptFetcher* dhcp_proxy_script_fetcher,
-                    NetLog* net_log)
-      : decider_(proxy_script_fetcher, dhcp_proxy_script_fetcher, net_log),
-        effective_config_(NULL),
-        proxy_resolver_(proxy_resolver) {
+  InitProxyResolver()
+      : proxy_resolver_(NULL),
+        next_state_(STATE_NONE) {
   }
 
   ~InitProxyResolver() {
@@ -345,19 +324,63 @@ class ProxyService::InitProxyResolver {
     }
   }
 
-  int Init(const ProxyConfig& config,
-           base::TimeDelta wait_delay,
-           ProxyConfig* effective_config,
-           const CompletionCallback& callback) {
+  // Begins initializing the proxy resolver; calls |callback| when done.
+  int Start(ProxyResolver* proxy_resolver,
+            ProxyScriptFetcher* proxy_script_fetcher,
+            DhcpProxyScriptFetcher* dhcp_proxy_script_fetcher,
+            NetLog* net_log,
+            const ProxyConfig& config,
+            base::TimeDelta wait_delay,
+            const CompletionCallback& callback) {
+    DCHECK_EQ(STATE_NONE, next_state_);
+    proxy_resolver_ = proxy_resolver;
+
+    decider_.reset(new ProxyScriptDecider(
+        proxy_script_fetcher, dhcp_proxy_script_fetcher, net_log));
     config_ = config;
     wait_delay_ = wait_delay;
-    effective_config_ = effective_config;
     callback_ = callback;
 
     next_state_ = STATE_DECIDE_PROXY_SCRIPT;
     return DoLoop(OK);
   }
 
+  // Similar to Start(), however it skips the ProxyScriptDecider stage. Instead
+  // |effective_config|, |decider_result|, |script_data| will be used as the

[wtc, 2012/01/03 23:54:45]

Nit: add "and" before |script_data|.

[eroman, 2012/01/04 03:41:47]

Done.

+  // inputs for initializing the ProxyResolver.
+  int StartSkipDecider(ProxyResolver* proxy_resolver,
+                       const ProxyConfig& effective_config,
+                       int decider_result,
+                       ProxyResolverScriptData* script_data,
+                       const CompletionCallback& callback) {
+    DCHECK_EQ(STATE_NONE, next_state_);
+    proxy_resolver_ = proxy_resolver;
+
+    effective_config_ = effective_config;
+    script_data_ = script_data;
+    callback_ = callback;
+
+    if (decider_result != OK)
+      return decider_result;

[wtc, 2012/01/03 23:54:45]

IMPORTANT: do we need to call callback in this case?

[eroman, 2012/01/04 00:11:24]

|callback| should not be called in this case. The caller should check the return
value of Start() to see if it completed synchronously.

+
+    next_state_ = STATE_SET_PAC_SCRIPT;
+    return DoLoop(OK);
+  }
+
+  // Returns the proxy configuration that was selected by ProxyScriptDecider.
+  // Should only be called upon completion of the initialization.
+  const ProxyConfig& effective_config() const {
+    DCHECK_EQ(STATE_NONE, next_state_);
+    return effective_config_;
+  }
+
+  // Returns the PAC script data that was selected by ProxyScriptDecider.
+  // Should only be called upon completion of the initialization.
+  ProxyResolverScriptData* script_data() {
+    DCHECK_EQ(STATE_NONE, next_state_);
+    return script_data_.get();
+  }
+
  private:
   enum State {
     STATE_NONE,
@@ -400,7 +423,7 @@ class ProxyService::InitProxyResolver {
   int DoDecideProxyScript() {
     next_state_ = STATE_DECIDE_PROXY_SCRIPT_COMPLETE;
 
-    return decider_.Start(
+    return decider_->Start(
         config_, wait_delay_, proxy_resolver_->expects_pac_bytes(),
         base::Bind(&InitProxyResolver::OnIOCompletion, base::Unretained(this)));
   }
@@ -409,17 +432,19 @@ class ProxyService::InitProxyResolver {
     if (result != OK)
       return result;
 
-    *effective_config_ = decider_.effective_config();
+    effective_config_ = decider_->effective_config();
+    script_data_ = decider_->script_data();
+
     next_state_ = STATE_SET_PAC_SCRIPT;
     return OK;
   }
 
   int DoSetPacScript() {
-    DCHECK(decider_.script_data());
+    DCHECK(script_data_);
     // TODO(eroman): Should log this latency to the NetLog.
     next_state_ = STATE_SET_PAC_SCRIPT_COMPLETE;
     return proxy_resolver_->SetPacScript(
-        decider_.script_data(),
+        script_data_,
         base::Bind(&InitProxyResolver::OnIOCompletion, base::Unretained(this)));
   }
 
@@ -440,9 +465,10 @@ class ProxyService::InitProxyResolver {
   }
 
   ProxyConfig config_;
+  ProxyConfig effective_config_;
+  scoped_refptr<ProxyResolverScriptData> script_data_;
   base::TimeDelta wait_delay_;
-  ProxyScriptDecider decider_;
-  ProxyConfig* effective_config_;
+  scoped_ptr<ProxyScriptDecider> decider_;
   ProxyResolver* proxy_resolver_;
   CompletionCallback callback_;
   State next_state_;
@@ -450,6 +476,183 @@ class ProxyService::InitProxyResolver {
   DISALLOW_COPY_AND_ASSIGN(InitProxyResolver);
 };
 
+// ProxyService::ProxyScriptDeciderPoller -------------------------------------
+
+// This helper class encapsulates the logic to schedule and run periodic
+// background checks to see if the PAC script (or effective proxy configuration)
+// has changed. If a change is detected, then the caller will be notified via
+// the ChangeCallback.
+class ProxyService::ProxyScriptDeciderPoller {
+ public:
+  typedef base::Callback<void(int, ProxyResolverScriptData*,
+                              const ProxyConfig& )> ChangeCallback;

[wtc, 2012/01/03 23:54:45]

Nit: is the space before the closing parenthesis ')' necessary?

[eroman, 2012/01/04 03:41:47]

Done.

+
+  ProxyScriptDeciderPoller(ChangeCallback callback,
+                           const ProxyConfig& config,
+                           bool proxy_resolver_expects_pac_bytes,
+                           ProxyScriptFetcher* proxy_script_fetcher,
+                           DhcpProxyScriptFetcher* dhcp_proxy_script_fetcher,
+                           int init_net_error,
+                           ProxyResolverScriptData* init_script_data,
+                           NetLog* net_log)

[wtc, 2012/01/03 23:54:45]

Nit: document at least the more important parameters.  For example,
I could not figure out what init_net_error means.

Nit: the callback parameter is usually listed pretty late.

[eroman, 2012/01/04 03:41:47]

Done.

+      : ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)),
+        change_callback_(callback),
+        config_(config),
+        proxy_resolver_expects_pac_bytes_(proxy_resolver_expects_pac_bytes),
+        proxy_script_fetcher_(proxy_script_fetcher),
+        dhcp_proxy_script_fetcher_(dhcp_proxy_script_fetcher),
+        last_error_(init_net_error),
+        last_script_data_(init_script_data) {
+    // Set the initial poll delay -- we check more aggressively right after a
+    // failure in case it was due to a spurious network error.
+    current_poll_delay_ms_ = GetInitialWaitDelayMs(init_net_error);
+
+    StartPollTimer();
+  }
+
+  // ----------------------------------
+  // Policy for the retry scheduling.
+  // ----------------------------------
+
+  static int64 GetInitialWaitDelayMs(int error) {
+    switch (poll_policy_) {
+      case POLL_POLICY_REGULAR:
+        // Wait 4 seconds before trying after an error, and 16 seconds after
+        // success.
+        return (error == OK) ? 4000 : 16000;

[wtc, 2012/01/03 23:54:45]

BUG(?): if we check more aggressively right after a failure,
then this check should be error != OK, right?

[eroman, 2012/01/04 00:11:24]

You are correct, this is a bug!

I'll have to think harder how to test this, since currently my tests are timing
independent (in fact the regular policy code is not exercised in the unit-test
code).

[eroman, 2012/01/04 03:41:47]

Done.

+      case POLL_POLICY_IMMEDIATE:
+        // Wait a mere 1 millisecond.
+        return 1;
+      case POLL_POLICY_NEVER:
+        // Unreasonably large wait delay, will never fire the check.
+        return 0xFFFFFFFF;
+    }
+
+    // Shouldn't be reached
+    return -1;

[wtc, 2012/01/03 23:54:45]

Nit: it seems better to do this as a default case.
Similarly for the switch statement below.

[eroman, 2012/01/04 00:11:24]

I prefer not to use default cases, since if I do that I don't get compiler
warnings when new unhandled enum values are added (with our current compiler
warnings, things don't build if I were to introduce a new PollPolicy value
without adding it to the switch, and I think that is useful.

+  }
+
+  static int64 GetNextWaitDelayMs(int64 current_delay_ms) {
+    switch (poll_policy_) {
+      case POLL_POLICY_REGULAR:
+        // Increase the delay exponentially up to 2 minutes.
+        return std::min(current_delay_ms * 2, static_cast<int64>(120000));

[wtc, 2012/01/03 23:54:45]

IMPORTANT: it would be nice to define constants for 4000,
16000, and 120000 at the top of this file, so that we can
easily locate them if we ever need to change them.

[eroman, 2012/01/04 03:41:47]

Done.

+      case POLL_POLICY_IMMEDIATE:
+      case POLL_POLICY_NEVER:
+        return current_delay_ms;
+    }
+
+    // Shouldn't be reached
+    return -1;
+  }
+
+  static PollPolicy set_policy(PollPolicy policy) {
+    PollPolicy prev = poll_policy_;
+    poll_policy_ = policy;
+    return prev;
+  }
+
+ private:
+  void StartPollTimer() {
+    DCHECK(!decider_.get());
+
+    MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        method_factory_.NewRunnableMethod(
+            &ProxyScriptDeciderPoller::OnPollTimerFired),
+        current_poll_delay_ms_);
+  }
+
+  void OnPollTimerFired() {
+    // Start the proxy script decider to see if anything has changed.
+    // TODO(eroman): Pass a proper NetLog rather than NULL.
+    decider_.reset(new ProxyScriptDecider(
+        proxy_script_fetcher_, dhcp_proxy_script_fetcher_, NULL));
+    int result = decider_->Start(
+        config_, base::TimeDelta(), proxy_resolver_expects_pac_bytes_,
+        base::Bind(&ProxyScriptDeciderPoller::OnProxyScriptDeciderCompleted,
+                   base::Unretained(this)));
+
+    if (result != ERR_IO_PENDING)
+      OnProxyScriptDeciderCompleted(result);
+  }
+
+  void OnProxyScriptDeciderCompleted(int result) {
+    if (HasScriptDataChanged(result, decider_->script_data())) {
+      // Something has changed, we must notify the ProxyService so it can
+      // re-initialize its ProxyResolver. Note that we post a notification task
+      // rather than calling it directly -- this is done to avoid an ugly
+      // destruction sequence, since |this| might be destroyed as a result of
+      // the notification.
+      MessageLoop::current()->PostTask(
+        FROM_HERE,
+        method_factory_.NewRunnableMethod(

[wtc, 2012/01/03 23:54:45]

IMPORTANT: I think the new way is to use a WeakPtrFactory
instead.

[eroman, 2012/01/04 00:11:24]

Thanks, I will look into this and update.

[eroman, 2012/01/04 03:41:47]

Done.

+            &ProxyScriptDeciderPoller::NotifyProxyServiceOfChange,
+            result,
+            make_scoped_refptr(decider_->script_data()),
+            decider_->effective_config()));
+      return;
+    }
+
+    decider_.reset();
+
+    // Schedule the next poll check.
+    current_poll_delay_ms_ = GetNextWaitDelayMs(current_poll_delay_ms_);
+    StartPollTimer();
+  }
+
+  bool HasScriptDataChanged(int result, ProxyResolverScriptData* script_data) {
+    if (result != last_error_) {
+      // Something changed -- it was failing before and now it succeeded, or
+      // conversely it succeeded before and now it failed.

[wtc, 2012/01/03 23:54:45]

IMPORTANT: A third case is that it was failing before and now it failed
with a different error code.  How should we handle that case?

The next check seems to handle that case.  If so, it is wrong...

[eroman, 2012/01/04 00:11:24]

Correct -- if the error code changes (but it is still a failure) we will notify
the ProxyService of the change.

I believe this is the behavior we want. For extensions there is a mechanism
called "mandatory PAC scripts", which means if the PAC script fails to be
fetched requests should error rather than silently fall-back to DIRECT settings.
When that happens, the error for the fetch is relevant, so having the most up to
date error is a good idea.

That said, I don't think getting different failures is going to be a common
occurrence, and it shouldn't have performance impact (since the re-configuration
will simply be falling back to DIRECT or error-mode, which doesn't involve much
work).

+      return true;
+    }
+
+    if (result != OK) {
+      // If it failed last time and failed again this time, then nothing has
+      // actually changed (even though the specific error code might be
+      // different).
+      return false;
+    }
+
+    // Otherwise if it succeeded both this time and last time, we need to look
+    // closer and see if we ended up downloading different content for the PAC
+    // script.
+    if (proxy_resolver_expects_pac_bytes_)
+      return script_data->utf16() != last_script_data_->utf16();
+    return script_data->url() != last_script_data_->url();
+  }
+
+  void NotifyProxyServiceOfChange(
+      int result,
+      const scoped_refptr<ProxyResolverScriptData>& script_data,
+      const ProxyConfig& effective_config) {
+    // Note that |this| may be deleted after calling into the ProxyService.
+    change_callback_.Run(result, script_data, effective_config);
+  }
+
+  ScopedRunnableMethodFactory<ProxyScriptDeciderPoller> method_factory_;
+
+  ChangeCallback change_callback_;
+  ProxyConfig config_;
+  bool proxy_resolver_expects_pac_bytes_;
+  ProxyScriptFetcher* proxy_script_fetcher_;
+  DhcpProxyScriptFetcher* dhcp_proxy_script_fetcher_;
+
+  int last_error_;
+  scoped_refptr<ProxyResolverScriptData> last_script_data_;
+
+  scoped_ptr<ProxyScriptDecider> decider_;
+  int64 current_poll_delay_ms_;
+
+  static PollPolicy poll_policy_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProxyScriptDeciderPoller);
+};
+
+ProxyService::PollPolicy ProxyService::ProxyScriptDeciderPoller::poll_policy_ =

[wtc, 2012/01/03 23:54:45]

Nit: add a
  // static
comment?  Not sure if it's recommended for static member initializers,
which are obviously static members.

[eroman, 2012/01/04 03:41:47]

Done.

+    ProxyService::POLL_POLICY_REGULAR;
+
 // ProxyService::PacRequest ---------------------------------------------------
 
 class ProxyService::PacRequest
@@ -858,6 +1061,24 @@ void ProxyService::OnInitProxyResolverComplete(int result) {
   DCHECK_EQ(STATE_WAITING_FOR_INIT_PROXY_RESOLVER, current_state_);
   DCHECK(init_proxy_resolver_.get());
   DCHECK(fetched_config_.HasAutomaticSettings());
+  config_ = init_proxy_resolver_->effective_config();
+
+  // At this point we have decided which proxy settings to use (i.e. which PAC
+  // script if any). We start up a background poller to periodically revisit
+  // this decision. If the contents of the PAC script change, or if the
+  // result of proxy auto-discovery changes, this instance will notice it and

[wtc, 2012/01/03 23:54:45]

Nit: instance => poller

"instance" is vague.

+  // will trigger a re-initialization using the newly discovered PAC.
+  script_poller_.reset(new ProxyScriptDeciderPoller(
+      base::Bind(&ProxyService::InitializeUsingDecidedConfig,
+                 base::Unretained(this)),
+      fetched_config_,
+      resolver_->expects_pac_bytes(),
+      proxy_script_fetcher_.get(),
+      dhcp_proxy_script_fetcher_.get(),
+      result,
+      init_proxy_resolver_->script_data(),
+      NULL));
+
   init_proxy_resolver_.reset();
 
   if (result != OK) {
@@ -876,6 +1097,8 @@ void ProxyService::OnInitProxyResolverComplete(int result) {
   }
   permanent_error_ = result;
 
+  // TODO(eroman): Make this ID unique in the case where configuration changed
+  //               due to ProxyScriptDeciderPoller.
   config_.set_id(fetched_config_.id());
 
   // Resume any requests which we had to defer until the PAC script was
@@ -1022,6 +1245,7 @@ ProxyService::State ProxyService::ResetProxyConfig(bool reset_fetched_config) {
 
   permanent_error_ = OK;
   proxy_retry_info_.clear();
+  script_poller_.reset();
   init_proxy_resolver_.reset();
   SuspendAllPendingRequests();
   config_ = ProxyConfig();
@@ -1099,6 +1323,12 @@ ProxyConfigService* ProxyService::CreateSystemProxyConfigService(
 #endif
 }
 
+// static
+ProxyService::PollPolicy ProxyService::set_pac_script_poll_policy(
+    PollPolicy policy) {
+  return ProxyScriptDeciderPoller::set_policy(policy);
+}
+
 void ProxyService::OnProxyConfigChanged(
     const ProxyConfig& config,
     ProxyConfigService::ConfigAvailability availability) {
@@ -1154,18 +1384,42 @@ void ProxyService::InitializeUsingLastFetchedConfig() {
   // Start downloading + testing the PAC scripts for this new configuration.
   current_state_ = STATE_WAITING_FOR_INIT_PROXY_RESOLVER;
 
-  init_proxy_resolver_.reset(
-      new InitProxyResolver(resolver_.get(),
-                            proxy_script_fetcher_.get(),
-                            dhcp_proxy_script_fetcher_.get(),
-                            net_log_));
-
   // If we changed networks recently, we should delay running proxy auto-config.
   base::TimeDelta wait_delay =
       stall_proxy_autoconfig_until_ - base::TimeTicks::Now();
 
-  int rv = init_proxy_resolver_->Init(
-      fetched_config_, wait_delay, &config_,
+  init_proxy_resolver_.reset(new InitProxyResolver());
+  int rv = init_proxy_resolver_->Start(
+      resolver_.get(),
+      proxy_script_fetcher_.get(),
+      dhcp_proxy_script_fetcher_.get(),
+      net_log_,
+      fetched_config_,
+      wait_delay,
+      base::Bind(&ProxyService::OnInitProxyResolverComplete,
+                 base::Unretained(this)));
+
+  if (rv != ERR_IO_PENDING)
+    OnInitProxyResolverComplete(rv);
+}
+
+void ProxyService::InitializeUsingDecidedConfig(
+    int decider_result,
+    ProxyResolverScriptData* script_data,
+    const ProxyConfig& effective_config) {
+  DCHECK(fetched_config_.is_valid());
+  DCHECK(fetched_config_.HasAutomaticSettings());
+
+  ResetProxyConfig(false);
+
+  current_state_ = STATE_WAITING_FOR_INIT_PROXY_RESOLVER;
+
+  init_proxy_resolver_.reset(new InitProxyResolver());
+  int rv = init_proxy_resolver_->StartSkipDecider(
+      resolver_.get(),
+      effective_config,
+      decider_result,
+      script_data,
       base::Bind(&ProxyService::OnInitProxyResolverComplete,
                  base::Unretained(this)));