Allows a proxy bypass entry to match on a specific port, used by the...

Allows a proxy bypass entry to match on a specific port, used by the
linux ProxyConfigService.

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

[eroman, 2009-04-14 20:37:07]

> Bug: http://crbug.com/8143

Best to format this as "BUG=8143", since mal runs a script that scans check-ins
for said format (to annotate the bug with checkin info). Maybe this format is
recognized by the script too, but I can't vouch for it.

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

http://codereview.chromium.org/73038/diff/1/2#newcode614
Line 614: if ((port+1) < bypass_url_domain.end() && IsAsciiDigit(*(port+1)) &&
space between binary operators "+".

http://codereview.chromium.org/73038/diff/1/2#newcode616
Line 616: bypass_url_domain.find(":", 0, colon) == std::string::npos))
This looks like a memory error.

std::string::find(const char*s, size_t pos, size_t len);

My understanding is that the |len| is the length of |s| (the  substring to
search for). In which case, the above would do illegal memory accesses if
|colon| > 2

I believe what you are trying to do instead is search a subrange of
|bypass_url_domain|, so how about one of these:

colon > bypass_url_domain.find(":")
end != std::find(begin, begin + colon, ':')

However you choose to address this, please add a corresponding regression test.

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

http://codereview.chromium.org/73038/diff/1/3#newcode695
Line 695: {
+1 on lowering the scopes, that was irritating me as well.

http://codereview.chromium.org/73038/diff/1/3#newcode775
Line 775: config.proxy_bypass_local_names = false;
For the new material, I would recommend putting this into a new test case (since
the current ProxyBypassList) is already crazy big.

How about naming this portion ProxyBypassListWithPorts?

[sdoyon, 2009-04-16 15:09:46]

On Tue, 14 Apr 2009, eroman@chromium.org wrote:

>> Bug: http://crbug.com/8143
>
> Best to format this as "BUG=8143", since mal runs a script that scans
> check-ins for said format (to annotate the bug with checkin info). Maybe
> this format is recognized by the script too, but I can't vouch for it.

Done.

> http://codereview.chromium.org/73038/diff/1/2
> File net/proxy/proxy_service.cc (right):
>
> http://codereview.chromium.org/73038/diff/1/2#newcode614
> Line 614: if ((port+1) < bypass_url_domain.end() &&
> IsAsciiDigit(*(port+1)) &&
> space between binary operators "+".

Done.

> http://codereview.chromium.org/73038/diff/1/2#newcode616
> Line 616: bypass_url_domain.find(":", 0, colon) == std::string::npos))
> This looks like a memory error.
>
> std::string::find(const char*s, size_t pos, size_t len);
>
> My understanding is that the |len| is the length of |s| (the  substring
> to search for). In which case, the above would do illegal memory
> accesses if |colon| > 2

Yikes!!
Nasty.

I also seem to have neglected that a scheme is prepended. OK redid that part.

> However you choose to address this, please add a corresponding
> regression test.

I'm assuming you mean a unit test? I had added one, but clearly it
didn't suffice. So I added more. The textual comparison of IPv6
addresses is kind of bogus, until we address the TODO.

> http://codereview.chromium.org/73038/diff/1/3
> File net/proxy/proxy_service_unittest.cc (right):

> http://codereview.chromium.org/73038/diff/1/3#newcode775
> Line 775: config.proxy_bypass_local_names = false;
> For the new material, I would recommend putting this into a new test
> case (since the current ProxyBypassList) is already crazy big.
>
> How about naming this portion ProxyBypassListWithPorts?

Sure. Done.

[eroman, 2009-04-16 19:21:56]

LGTM

http://codereview.chromium.org/73038/diff/1002/1003
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/73038/diff/1002/1003#newcode604
Line 604: scheme_colon = bypass_url_domain.find("://");
nit: instead of calling find(), could set the position during construction of
bypass_url_domain:

std::string bypass_url_domain_with_scheme = url.scheme();
scheme_colon = url_domain_with_scheme.length(); // <--- HERE
bypass_url_domain_with_scheme += "://";

http://codereview.chromium.org/73038/diff/1002/1003#newcode623
Line 623: std::find(domain_begin, port_iter, ':') == port_iter))
my head hurts. looks right tho.

[sdoyon, 2009-04-17 14:19:33]

On Thu, 16 Apr 2009, eroman@chromium.org wrote:

> LGTM
>
>
> http://codereview.chromium.org/73038/diff/1002/1003
> File net/proxy/proxy_service.cc (right):
>
> http://codereview.chromium.org/73038/diff/1002/1003#newcode604
> Line 604: scheme_colon = bypass_url_domain.find("://");
> nit: instead of calling find(), could set the position during
> construction of bypass_url_domain:
>
> std::string bypass_url_domain_with_scheme = url.scheme();
> scheme_colon = url_domain_with_scheme.length(); // <--- HERE
> bypass_url_domain_with_scheme += "://";

Oh indeed!

> http://codereview.chromium.org/73038/diff/1002/1003#newcode623
> Line 623: std::find(domain_begin, port_iter, ':') == port_iter))
> my head hurts. looks right tho.

Good.

Thanks