Track sources of proxy settings.

Track sources of proxy settings.

ProxyConfig keeps track of the source of proxy settings. During proxy resolution ProxyService tags the resulting ProxyInfo with the same source information.

BUG=none
TEST=net_unittests


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

[asanka, 2012-05-16 23:48:56]

I broke off the proxy configuration source tracking into a separate CL.

There are no users of the source information other than logging in this CL. I'll
update net-internals to print the source if this looks okay.

[eroman, 2012-05-17 19:52:22]

Just came back from vacation! Trying to catch up, hope to respond by end of day.

[eroman, 2012-05-18 03:58:55]

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_config.h#ne...
net/proxy/proxy_config.h:13: #include "net/proxy/proxy_config_source.h"
I believe this should be sorted lower.

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service_win.cc (right):

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_win.cc:161: *config =
ProxyConfig::CreateDirect();
In this case the "source" will be Unknown. I wander if we want a separate source
to indicate like SOURCE_SYSTEM_FAILED.

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_config_sour...
File net/proxy/proxy_config_source.h (right):

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_config_sour...
net/proxy/proxy_config_source.h:13: // The source information is used
nit: this line can be longer.

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_info.cc
File net/proxy/proxy_info.cc (right):

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_info.cc#new...
net/proxy/proxy_info.cc:21: proxy_retry_info_ = other.proxy_retry_info_;
Should this copy the source?

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_info.cc#new...
net/proxy/proxy_info.cc:26: proxy_retry_info_.clear();
Should this reset the source too?

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_info.cc#new...
net/proxy/proxy_info.cc:31: proxy_retry_info_.clear();
Should this reset the source too?

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_info.cc#new...
net/proxy/proxy_info.cc:36: proxy_retry_info_.clear();
Should this reset the source too?

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

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:1123: (did_use_proxy_rules ? config_.source() :
PROXY_CONFIG_SOURCE_NONE);
I'm not sure that I follow this.

Apply() can return false when the rules are "empty". But those empty rules may
still have come from the system proxy settings.

It seems like the signature for Apply() can be left as void and the result
should ALWAYS have a source.

The one caveat is when we failed to fetch the system settings and defaulted to
something else. In that case I think the source should be something like
SYSTEM_FAILED

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:1245: config_.set_source(fetched_config_.source());
Perhaps this should be done internally by ProxyScriptDecider.

[asanka, 2012-05-18 16:27:00]

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_config.h#ne...
net/proxy/proxy_config.h:13: #include "net/proxy/proxy_config_source.h"
On 2012/05/18 03:58:56, eroman wrote:
> I believe this should be sorted lower.

Done.

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service_win.cc (right):

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_win.cc:161: *config =
ProxyConfig::CreateDirect();
On 2012/05/18 03:58:56, eroman wrote:
> In this case the "source" will be Unknown. I wander if we want a separate
source
> to indicate like SOURCE_SYSTEM_FAILED.

I added a source named SYSTEM_FAILED.

Based on http://code.google.com/p/chromium/issues/detail?id=118385#c14 , it
looks like this call would succeed even in the absence of connection specific
settings. So I think this specific failure would be quite rare.

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_config_sour...
File net/proxy/proxy_config_source.h (right):

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_config_sour...
net/proxy/proxy_config_source.h:13: // The source information is used
On 2012/05/18 03:58:56, eroman wrote:
> nit: this line can be longer.

Done.

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_info.cc
File net/proxy/proxy_info.cc (right):

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_info.cc#new...
net/proxy/proxy_info.cc:21: proxy_retry_info_ = other.proxy_retry_info_;
On 2012/05/18 03:58:56, eroman wrote:
> Should this copy the source?

The original intent was to set/reset config_id_ and config_source_ together. The
config_id_ was only being modified in proxy_service.cc, so I chose to set the
config_source_ there as well.

I added set/reset for config_id_ and config_source_ here for consistency.

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_info.cc#new...
net/proxy/proxy_info.cc:26: proxy_retry_info_.clear();
On 2012/05/18 03:58:56, eroman wrote:
> Should this reset the source too?

Done. See above.

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_info.cc#new...
net/proxy/proxy_info.cc:31: proxy_retry_info_.clear();
On 2012/05/18 03:58:56, eroman wrote:
> Should this reset the source too?

Done. See above.

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_info.cc#new...
net/proxy/proxy_info.cc:36: proxy_retry_info_.clear();
On 2012/05/18 03:58:56, eroman wrote:
> Should this reset the source too?

Done. See above.

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

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:1123: (did_use_proxy_rules ? config_.source() :
PROXY_CONFIG_SOURCE_NONE);
On 2012/05/18 03:58:56, eroman wrote:
> I'm not sure that I follow this.
> 
> Apply() can return false when the rules are "empty". But those empty rules may
> still have come from the system proxy settings.

I wanted to distinguish between:
- There are no proxy settings, so we fell back to using a direct connection
(includes the case where the rules were empty as well as the case where there
was no per-scheme setting).
- There are proxy settings, but they resolved to direct connections for this
URL.

This distinction is relevant to detecting whether the proxy resolution
represents a proxy bypass. Do you see a better way to represent this?

> It seems like the signature for Apply() can be left as void and the result
> should ALWAYS have a source.
> 
> The one caveat is when we failed to fetch the system settings and defaulted to
> something else. In that case I think the source should be something like
> SYSTEM_FAILED

I added this.

http://codereview.chromium.org/10310179/diff/5001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:1245: config_.set_source(fetched_config_.source());
On 2012/05/18 03:58:56, eroman wrote:
> Perhaps this should be done internally by ProxyScriptDecider.

Do you mean to fold PacSource into ConfigSource?

[eroman, 2012-05-25 02:16:38]

I haven't forgotten about this, will give it another pass real soon now.

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

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

http://codereview.chromium.org/10310179/diff/7005/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:1123: (did_use_proxy_rules ? config_.source() :
PROXY_CONFIG_SOURCE_NONE);
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!

[asanka, 2012-06-08 23:27:10]

On 2012/06/02 02:06:10, eroman wrote:
> http://codereview.chromium.org/10310179/diff/7005/net/proxy/proxy_service.cc
> File net/proxy/proxy_service.cc (right):
> 
>
http://codereview.chromium.org/10310179/diff/7005/net/proxy/proxy_service.cc#...
> net/proxy/proxy_service.cc:1123: (did_use_proxy_rules ? config_.source() :
> PROXY_CONFIG_SOURCE_NONE);
> 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.

The "did we bypass a system proxy" condition looks like:

  is_direct_only() &&        // [1] listsize == 1 && is_direct &&
                             //     proxy_retry_info.empty()
  config_source == SYSTEM && // [2] Only consider system proxy settings
  (was_from_bypass ||        // [3] Direct connection resulting from bypass
                             //     rules in ProxyConfig
   was_from_pac_script)      // [4] proxy list derived from PAC script

The [3] and [4] correspond to steps 3 and 4 in
http://blogs.msdn.com/b/ieinternals/archive/2012/06/05/the-local-intranet-sec...
.

So far we have [1] and [2] :).

[3] is known in ProxyConfig::ProxyRules::Apply(). [4] is known in
ProxyService::PacRequest.

Perhaps instead a did_bypass_proxy bit can be set in ProxyInfo by
ProxyRules::Apply() and PacRequest::QueryDidComplete()?

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

Yeah. I think setting it directly in |result| would be better.

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

I also apologize for how ugly this is.

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

[eroman, 2012-06-11 23:18:34]

LGTM.

Thanks for working through my comments!

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_config_sou...
File net/proxy/proxy_config_source.cc (right):

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_config_sou...
net/proxy/proxy_config_source.cc:14: static const char* kSourceNames[] = {
optional nit: Don't need the static if in anonymous namespace

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_config_sou...
File net/proxy/proxy_config_source.h (right):

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_config_sou...
net/proxy/proxy_config_source.h:13: // The source information is used for
credentials use policy determination and
"for credentials use policy determination" doesn't sound right, is there a word
missing or too many?

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_config_sou...
net/proxy/proxy_config_source.h:18: PROXY_CONFIG_SOURCE_NONE,          // No
explicit proxy settings.
I don't think we should have this, can it be deleted?

I think the existing user of SOURCE_NONE can either be set to SOURCE_UNKNOWN or
SOURCE_CUSTOM.

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_config_sou...
net/proxy/proxy_config_source.h:37: }  // namespace
namespace net

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_list.h
File net/proxy/proxy_list.h (right):

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_list.h#new...
net/proxy/proxy_list.h:50: size_t Size() const;
nit regarding capitalization:

either call size() or GetSize(). My preference is the former.

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

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_service.cc...
net/proxy/proxy_service.cc:170: config->set_source(PROXY_CONFIG_SOURCE_NONE);
See earlier comment. I think this should be either SOURCE_CUSTOM or
SOURCE_UNKNOWN.

[asanka, 2012-06-12 19:37:18]

Thanks! I addressed comments and added tests for the new code.

I'll add it to the CQ once the try jobs complete.

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_config_sou...
File net/proxy/proxy_config_source.cc (right):

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_config_sou...
net/proxy/proxy_config_source.cc:14: static const char* kSourceNames[] = {
On 2012/06/11 23:18:34, eroman wrote:
> optional nit: Don't need the static if in anonymous namespace

Done.

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_config_sou...
File net/proxy/proxy_config_source.h (right):

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_config_sou...
net/proxy/proxy_config_source.h:13: // The source information is used for
credentials use policy determination and
On 2012/06/11 23:18:34, eroman wrote:
> "for credentials use policy determination" doesn't sound right, is there a
word
> missing or too many?

Reworded.

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_config_sou...
net/proxy/proxy_config_source.h:18: PROXY_CONFIG_SOURCE_NONE,          // No
explicit proxy settings.
On 2012/06/11 23:18:34, eroman wrote:
> I don't think we should have this, can it be deleted?
> 
> I think the existing user of SOURCE_NONE can either be set to SOURCE_UNKNOWN
or
> SOURCE_CUSTOM.

Done. Went with SOURCE_UNKNOWN.

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_config_sou...
net/proxy/proxy_config_source.h:37: }  // namespace
On 2012/06/11 23:18:34, eroman wrote:
> namespace net

Done.

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_list.h
File net/proxy/proxy_list.h (right):

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_list.h#new...
net/proxy/proxy_list.h:50: size_t Size() const;
On 2012/06/11 23:18:34, eroman wrote:
> nit regarding capitalization:
> 
> either call size() or GetSize(). My preference is the former.

Done. Went with size().

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

http://codereview.chromium.org/10310179/diff/23008/net/proxy/proxy_service.cc...
net/proxy/proxy_service.cc:170: config->set_source(PROXY_CONFIG_SOURCE_NONE);
On 2012/06/11 23:18:34, eroman wrote:
> See earlier comment. I think this should be either SOURCE_CUSTOM or
> SOURCE_UNKNOWN.

Done. Went with SOURCE_UNKNOWN.

[commit-bot: I haz the power, 2012-06-12 20:24:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/10310179/25060

[commit-bot: I haz the power, 2012-06-12 23:32:52]

Change committed as 141784

[eroman, 2012-10-30 00:38:25]

https://chromiumcodereview.appspot.com/10310179/diff/25060/net/proxy/proxy_in...
File net/proxy/proxy_info.cc (right):

https://chromiumcodereview.appspot.com/10310179/diff/25060/net/proxy/proxy_in...
net/proxy/proxy_info.cc:31: Reset();
FYI: interestingly, there was fragile code relying on UseDirect() *not*
resetting config_id_! (see http://crbug.com/143984). Yuck.