Poll PAC scripts for content changes.

Poll PAC scripts for content changes.

An exponential poll delay is used which doubles the delay after each attempt (for instance polls might occur at 4 seconds, then 8 seconds, then 16 seconds, up to a maximum of 2 minutes).

This same mechanism also applies to the resolution of "auto-detect" settings.

The intent of this change is to make it possible to
  (1) Notice server-side changes made to the PAC scripts
  (2) Recover from spurious network errors when initially downloading PAC scripts.

BUG=90581
TEST=Configure Chrome to use a PAC script which fails to load (i.e. a non-existent file:// url, or remap the host's DNS record). Start Chrome and verify using about:net-internals that it has failed to fetch the PAC script and has fallen back to DIRECT settings. Fix the PAC URL to work and wait for chrome to notice the change and reconfigure itself (will take a maximum of 2 minutes).


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=116462

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

Patch Set 1 LGTM.

Please check the comment marked with BUG(?).  Also, some of
my comments marked with IMPORTANT may be potential bugs.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:349: // |effective_config|, |decider_result|,
|script_data| will be used as the

Nit: add "and" before |script_data|.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:364: return decider_result;

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

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:488: const ProxyConfig& )> ChangeCallback;

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

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:497: NetLog* net_log)

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.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:522: return (error == OK) ? 4000 : 16000;

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

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:532: return -1;

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

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:539: return std::min(current_delay_ms * 2,
static_cast<int64>(120000));

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.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:589: method_factory_.NewRunnableMethod(

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

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:607: // conversely it succeeded before and now it
failed.

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...

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:653: ProxyService::PollPolicy
ProxyService::ProxyScriptDeciderPoller::poll_policy_ =

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

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:1069: // result of proxy auto-discovery changes, this
instance will notice it and

Nit: instance => poller

"instance" is vague.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.h#newco...
net/proxy/proxy_service.h:248: // active policy.

Nit: currently active => previously active ?

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.h#newco...
net/proxy/proxy_service.h:369: scoped_ptr<ProxyScriptDeciderPoller>
script_poller_;

Nit: should we document this member?

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service_unittes...
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service_unittes...
net/proxy/proxy_service_unittest.cc:41: // polling, then it is possible it might
start a background re-try in the middle

Nit: remove "it is possible"

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service_unittes...
net/proxy/proxy_service_unittest.cc:60: ProxyService::PollPolicy
previous_policy_;

Nit: initialize this member in a constructor?

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service_unittes...
net/proxy/proxy_service_unittest.cc:1956: TEST_F(ProxyServiceTest,
PACScriptRefetchAfterFailure) {

I didn't review the unit tests carefully.  I merely verified
that you have covered four important cases.

It may be a good idea to add a fifth case: two consecutive
failures with different error codes.

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

Thanks for the review!

I will respond to your other comments after updating the patch, some initial
responses:

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:364: return decider_result;
On 2012/01/03 23:54:45, wtc wrote:
> 
> IMPORTANT: do we need to call callback in this case?

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

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:522: return (error == OK) ? 4000 : 16000;
On 2012/01/03 23:54:45, wtc wrote:
> 
> BUG(?): if we check more aggressively right after a failure,
> then this check should be error != OK, right?

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).

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:532: return -1;
On 2012/01/03 23:54:45, wtc wrote:
> 
> Nit: it seems better to do this as a default case.
> Similarly for the switch statement below.

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.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:589: method_factory_.NewRunnableMethod(
On 2012/01/03 23:54:45, wtc wrote:
> 
> IMPORTANT: I think the new way is to use a WeakPtrFactory
> instead.

Thanks, I will look into this and update.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:607: // conversely it succeeded before and now it
failed.
On 2012/01/03 23:54:45, wtc wrote:
> 
> 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...

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).

[eroman, 2012-01-04 03:41:46]

All comments addressed:

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:349: // |effective_config|, |decider_result|,
|script_data| will be used as the
On 2012/01/03 23:54:45, wtc wrote:
> 
> Nit: add "and" before |script_data|.

Done.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:488: const ProxyConfig& )> ChangeCallback;
On 2012/01/03 23:54:45, wtc wrote:
> 
> Nit: is the space before the closing parenthesis ')' necessary?

Done.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:497: NetLog* net_log)
On 2012/01/03 23:54:45, wtc wrote:
> 
> 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.

Done.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:522: return (error == OK) ? 4000 : 16000;
On 2012/01/03 23:54:45, wtc wrote:
> 
> BUG(?): if we check more aggressively right after a failure,
> then this check should be error != OK, right?

Done.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:539: return std::min(current_delay_ms * 2,
static_cast<int64>(120000));
On 2012/01/03 23:54:45, wtc wrote:
> 
> 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.

Done.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:589: method_factory_.NewRunnableMethod(
On 2012/01/03 23:54:45, wtc wrote:
> 
> IMPORTANT: I think the new way is to use a WeakPtrFactory
> instead.

Done.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.cc#newc...
net/proxy/proxy_service.cc:653: ProxyService::PollPolicy
ProxyService::ProxyScriptDeciderPoller::poll_policy_ =
On 2012/01/03 23:54:45, wtc wrote:
> 
> Nit: add a
>   // static
> comment?  Not sure if it's recommended for static member initializers,
> which are obviously static members.

Done.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.h#newco...
net/proxy/proxy_service.h:248: // active policy.
On 2012/01/03 23:54:45, wtc wrote:
> 
> Nit: currently active => previously active ?

Done.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service.h#newco...
net/proxy/proxy_service.h:369: scoped_ptr<ProxyScriptDeciderPoller>
script_poller_;
On 2012/01/03 23:54:45, wtc wrote:
> 
> Nit: should we document this member?

Done.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service_unittes...
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service_unittes...
net/proxy/proxy_service_unittest.cc:41: // polling, then it is possible it might
start a background re-try in the middle
On 2012/01/03 23:54:45, wtc wrote:
> 
> Nit: remove "it is possible"

Done.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service_unittes...
net/proxy/proxy_service_unittest.cc:60: ProxyService::PollPolicy
previous_policy_;
On 2012/01/03 23:54:45, wtc wrote:
> 
> Nit: initialize this member in a constructor?

leaving as-is, since there isn't a good value to initialize it to.

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service_unittes...
net/proxy/proxy_service_unittest.cc:1956: TEST_F(ProxyServiceTest,
PACScriptRefetchAfterFailure) {
On 2012/01/03 23:54:45, wtc wrote:
> 
> I didn't review the unit tests carefully.  I merely verified
> that you have covered four important cases.
> 
> It may be a good idea to add a fifth case: two consecutive
> failures with different error codes.

Sounds good, I'll leave that to a follow-up change.

[wtc, 2012-01-04 22:14:15]

Patch Set 2 LGTM.  Please fix an inaccurate comment (see my
last review comment below).

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service_unittes...
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/9078003/diff/1/net/proxy/proxy_service_unittes...
net/proxy/proxy_service_unittest.cc:60: ProxyService::PollPolicy
previous_policy_;

On 2012/01/04 03:41:47, eroman wrote:
>
> leaving as-is, since there isn't a good value to initialize it to.

OK, I think this is fine for the following reasons.

Coverity warns about members not initialized in a
constructor.  Since we do not define a constructor for
this class, I guess Coverity has no constructor to warn
about.  Also we are not scanning unit tests with Coverity
right now (to save time).

http://codereview.chromium.org/9078003/diff/12001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/9078003/diff/12001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:73: const int64 kNumMillisToStallAfterNetworkChanges
= 2000;

Nit: rename this constant kDelayAfterNetworkChangesMs, for
consistency.

http://codereview.chromium.org/9078003/diff/12001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:645: // different).

The comment "(even though the specific error code might be
different)" is wrong.  If we get here, we failed again with the
same error code.

I suggest:
  // If it failed last time and failed again with the same error code this time,
  // then nothing has actually changed.

[eroman, 2012-01-04 22:40:11]

Thanks.
After completing some manual tests I will commit this.

http://codereview.chromium.org/9078003/diff/12001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/9078003/diff/12001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:73: const int64 kNumMillisToStallAfterNetworkChanges
= 2000;
On 2012/01/04 22:14:15, wtc wrote:
> 
> Nit: rename this constant kDelayAfterNetworkChangesMs, for
> consistency.

Done.

http://codereview.chromium.org/9078003/diff/12001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:645: // different).
On 2012/01/04 22:14:15, wtc wrote:
> 
> The comment "(even though the specific error code might be
> different)" is wrong.  If we get here, we failed again with the
> same error code.
> 
> I suggest:
>   // If it failed last time and failed again with the same error code this
time,
>   // then nothing has actually changed.

Done.

[commit-bot: I haz the power, 2012-01-04 22:48:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/9078003/16001

[commit-bot: I haz the power, 2012-01-05 00:36:08]

Try job failure for 9078003-16001 (retry) on win_rel for step
"test_shell_tests".
It's a second try, previously, step "test_shell_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-01-05 03:28:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/9078003/18001

[commit-bot: I haz the power, 2012-01-05 03:29:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/9078003/18001

[eroman, 2012-01-05 04:55:16]

I made some minor changes in patchset 4 to address a DCHECK that was hit on the
bot.

There was a type of ProxyResolverScriptData being used on win trybot that I
hadn't considered; changed to a switch statement to make sure I covered them
all. No new test added since this already had coverage on bots (albeit an
uncommon configuration).

[commit-bot: I haz the power, 2012-01-05 05:47:58]

Change committed as 116462

[wtc, 2012-01-05 22:08:26]

Patch Set 4 LGTM.

http://codereview.chromium.org/9078003/diff/12001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/9078003/diff/12001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:653: return script_data->utf16() !=
last_script_data_->utf16();

The code in the new Equals() method seems to imply that
the proxy_resolver_expects_pac_bytes_ member isn't necessary
and can be replaced by the expression
  last_script_data_->type() == TYPE_SCRIPT_CONTENTS