Add a command line flag to force all network traffic through a particular ser...

Add a command line flag to force all network traffic through a particular server.
The command line is:
  --fixed-server=host:port

When set, all traffic will be diverted through this server.  This is useful for
testing purposes with fixed servers.

BUG=none
TEST=none



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

[eroman, 2009-11-03 00:40:42]

http://codereview.chromium.org/345034/diff/5008/6011
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/345034/diff/5008/6011#newcode23
Line 23: #include "net/base/mock_host_resolver.h"
Please don't define ForcedHostResolver in mock_host_resolver.h:
mock_host_resolver.h is intended to be only available to tests.

It is an error that right now we link mock_host_resolver.cc into chrome (and
there is a TODO about this in net.gyp).

http://codereview.chromium.org/345034/diff/5008/6011#newcode218
Line 218: // Check for forced HostResolver
This comment doesn't add much value over what the code says.
Perhaps something more descriptive, like:

// Set a HostResolver that resolves EVERY hostname:port to the same addresslist,
regardless of what hostname/port actually are.

http://codereview.chromium.org/345034/diff/5008/6011#newcode222
Line 222: net::ForcedHostResolver* resolver = new net::ForcedHostResolver(host);
This needs to move into chrome_browser_net::GetGlobalHostResolver().

Unfortunately the HostResolver creation code does not happen on the IO thread.
This is because the DNS prefetcher is needing it during initialization on the UI
thread, and must share the same instance.
There is a bug on file to change this (http://crbug.com/25335), at which point
the creation code could be a part of the ChromeURLRequestContext....

http://codereview.chromium.org/345034/diff/5008/6014
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/345034/diff/5008/6014#newcode566
Line 566: //   --force-resolve=myserver:1000
This flag seems like a bad idea to me.

Why can't you use a SOCKS proxy here?

In which case, it would suffice to pass the flag:

--proxy-server=socks5://foobar

SOCKS is a transparent proxy, so it is protocol agnostic and would work equally
well for FLIP traffic. 

Moreover with version 4A and 5 of SOCKS you can have the proxy server take care
of the DNS resolutions.

http://codereview.chromium.org/345034/diff/5008/6015
File net/base/host_resolver.h (right):

http://codereview.chromium.org/345034/diff/5008/6015#newcode120
Line 120: // If the operating cannnot be completed synchronously, ERR_IO_PENDING
will
operating --> operation.

http://codereview.chromium.org/345034/diff/5008/6010
File net/base/mock_host_resolver.cc (right):

http://codereview.chromium.org/345034/diff/5008/6010#newcode222
Line 222: // Parse the host pattern:  "host:port".  This only works for IPv4
addresses,
Use net::ParseHostAndPort() -- I added that specifically so it works properly
with IPv6 literals.

http://codereview.chromium.org/345034/diff/5008/6010#newcode233
Line 233: scoped_refptr<HostResolverImpl> resolver = new HostResolverImpl(NULL,
0, 0);
No need to create a HostResolverImpl -- HostResolverImpl() is a layer that
handles concurrent requests and caching, of which we have neither here.

The function you want to be using here instead is:

int SystemHostResolverProc(const std::string& host, AddressFamily
address_family,  AddressList* addrlist);

(host_resolver_proc.h)

http://codereview.chromium.org/345034/diff/5008/6010#newcode235
Line 235: int rv = resolver->Resolve(info, &address_, NULL, NULL, NULL);
This is bad!

By doing a synchronous DNS resolve we are blocking the thread.
(In current snapshot it will be the IO thread you are blocking, if you follow
comment about GetGlobalHostResolver() then it will be the UI thread you are
blocking).

I would only be cool with this if we restricted the flag to IP literals, and did
a numeric-only resolve here so we know it will be fast.

http://codereview.chromium.org/345034/diff/5008/6009
File net/base/mock_host_resolver.h (right):

http://codereview.chromium.org/345034/diff/5008/6009#newcode92
Line 92: class ForcedHostResolver : public HostResolver {
I recommend calling this "Fixed" Instead of "Forced".

We use the "Fixed" naming in several other places for such implementations.

http://codereview.chromium.org/345034/diff/5008/6009#newcode94
Line 94: // |host| is a string representing the resolution.
We generally call this |host_and_port| to remove ambiguity on when ":<port>" and
optional, and how it works for IPv6.

Speaking of which, this should work for IPv6 literals like:

"[::1]:80"

[Mike Belshe, 2009-11-03 01:46:01]

http://codereview.chromium.org/345034/diff/5008/6011
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/345034/diff/5008/6011#newcode23
Line 23: #include "net/base/mock_host_resolver.h"
On 2009/11/03 00:40:42, eroman wrote:
> Please don't define ForcedHostResolver in mock_host_resolver.h:
> mock_host_resolver.h is intended to be only available to tests.
> 
> It is an error that right now we link mock_host_resolver.cc into chrome (and
> there is a TODO about this in net.gyp).

Done.

http://codereview.chromium.org/345034/diff/5008/6011#newcode218
Line 218: // Check for forced HostResolver
On 2009/11/03 00:40:42, eroman wrote:
> This comment doesn't add much value over what the code says.
> Perhaps something more descriptive, like:
> 
> // Set a HostResolver that resolves EVERY hostname:port to the same
addresslist,
> regardless of what hostname/port actually are.

Done.

http://codereview.chromium.org/345034/diff/5008/6011#newcode222
Line 222: net::ForcedHostResolver* resolver = new net::ForcedHostResolver(host);
On 2009/11/03 00:40:42, eroman wrote:
> This needs to move into chrome_browser_net::GetGlobalHostResolver().
> 
> Unfortunately the HostResolver creation code does not happen on the IO thread.
> This is because the DNS prefetcher is needing it during initialization on the
UI
> thread, and must share the same instance.
> There is a bug on file to change this (http://crbug.com/25335), at which point
> the creation code could be a part of the ChromeURLRequestContext....

Done.

http://codereview.chromium.org/345034/diff/5008/6014
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/345034/diff/5008/6014#newcode566
Line 566: //   --force-resolve=myserver:1000
I don't want to have to run and configure a socks server; then my performance
tests will have the added burden of needing a socks server which doesn't alter
the performance characteristics of the stream.

On 2009/11/03 00:40:42, eroman wrote:
> This flag seems like a bad idea to me.
> 
> Why can't you use a SOCKS proxy here?
> 
> In which case, it would suffice to pass the flag:
> 
> --proxy-server=socks5://foobar
> 
> SOCKS is a transparent proxy, so it is protocol agnostic and would work
equally
> well for FLIP traffic. 
> 
> Moreover with version 4A and 5 of SOCKS you can have the proxy server take
care
> of the DNS resolutions.

http://codereview.chromium.org/345034/diff/5008/6015
File net/base/host_resolver.h (right):

http://codereview.chromium.org/345034/diff/5008/6015#newcode120
Line 120: // If the operating cannnot be completed synchronously, ERR_IO_PENDING
will
On 2009/11/03 00:40:42, eroman wrote:
> operating --> operation.

Done.

http://codereview.chromium.org/345034/diff/5008/6010
File net/base/mock_host_resolver.cc (right):

http://codereview.chromium.org/345034/diff/5008/6010#newcode222
Line 222: // Parse the host pattern:  "host:port".  This only works for IPv4
addresses,
On 2009/11/03 00:40:42, eroman wrote:
> Use net::ParseHostAndPort() -- I added that specifically so it works properly
> with IPv6 literals.

Done.

http://codereview.chromium.org/345034/diff/5008/6010#newcode233
Line 233: scoped_refptr<HostResolverImpl> resolver = new HostResolverImpl(NULL,
0, 0);
On 2009/11/03 00:40:42, eroman wrote:
> No need to create a HostResolverImpl -- HostResolverImpl() is a layer that
> handles concurrent requests and caching, of which we have neither here.
> 
> The function you want to be using here instead is:
> 
> int SystemHostResolverProc(const std::string& host, AddressFamily
> address_family,  AddressList* addrlist);
> 
> (host_resolver_proc.h)

Done.

http://codereview.chromium.org/345034/diff/5008/6010#newcode235
Line 235: int rv = resolver->Resolve(info, &address_, NULL, NULL, NULL);
I don't see the concern.  We do the resolve once, and never do it again.  This
is only for testing purposes, when the user has manually specified a
"--fixed-server" parameter.  Isn't it okay to take a little time?

Didn't realize the threading constraint, so that has to be fixed, of course.


On 2009/11/03 00:40:42, eroman wrote:
> This is bad!
> 
> By doing a synchronous DNS resolve we are blocking the thread.
> (In current snapshot it will be the IO thread you are blocking, if you follow
> comment about GetGlobalHostResolver() then it will be the UI thread you are
> blocking).
> 
> I would only be cool with this if we restricted the flag to IP literals, and
did
> a numeric-only resolve here so we know it will be fast.

http://codereview.chromium.org/345034/diff/5008/6009
File net/base/mock_host_resolver.h (right):

http://codereview.chromium.org/345034/diff/5008/6009#newcode92
Line 92: class ForcedHostResolver : public HostResolver {
On 2009/11/03 00:40:42, eroman wrote:
> I recommend calling this "Fixed" Instead of "Forced".
> 
> We use the "Fixed" naming in several other places for such implementations.

Done.

http://codereview.chromium.org/345034/diff/5008/6009#newcode94
Line 94: // |host| is a string representing the resolution.
Is there a common function for parsing this?

On 2009/11/03 00:40:42, eroman wrote:
> We generally call this |host_and_port| to remove ambiguity on when ":<port>"
and
> optional, and how it works for IPv6.
> 
> Speaking of which, this should work for IPv6 literals like:
> 
> "[::1]:80"

[eroman, 2009-11-03 04:24:06]

http://codereview.chromium.org/345034/diff/4008/4011
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/345034/diff/4008/4011#newcode565
Line 565: // Force all requests to go to this server.
Can you enhance this comment section?
In particular:
 * This commandline is only for tests, and shouldn't ordinarily be used
 * This commandline can slow down startup/first-request, as it does synchronous
lookups blocking the UI/main thread.
 * Is the [:port] optional, or required? What happens if it is optional.

http://codereview.chromium.org/345034/diff/4008/4011#newcode566
Line 566: //   --fixed-server=myserver:1000
Per our discussion, can you come up with a more descriptive name.
Some ideas:

--testing-fixed-host-resolver

http://codereview.chromium.org/345034/diff/4008/4015
File net/base/fixed_host_resolver.cc (right):

http://codereview.chromium.org/345034/diff/4008/4015#newcode21
Line 21: DCHECK(rv == OK);
See comment the later comment about needing to handle failure cases.
Not doing so can result in crashes (not just debug mode).

http://codereview.chromium.org/345034/diff/4008/4015#newcode23
Line 23: if (port > 0)
The handling of port seems weird here.
I see two plausible policies:

(1) port is not optional in |host_and_port|, so --fixed-server="foobar" is
considered a failure.

(2) port is optional in |host_and_port|, so --fixed-server="foobar" is assumed
to mean "only rewrite the host, but keep the port". If you define things this
way, then the Resolve() function below needs to return an address list with the
proper port.


What it does right now is undefined, since we don't define what port
SystemHostResolverProc() will set.

http://codereview.chromium.org/345034/diff/4008/4015#newcode33
Line 33: *addresses = address_;
The constructor doesn't handle the error cases -- that means if either
ParseHostAndPort() failed, or SystemHostResolverProc() failed, then |address_|
will be un-initialized.

That means if any consumer uses the address, it will crash (NULL-pointer deref).

I suggest that instead you save some |int result_| member during initialization,
and then here:

if (result_ != OK)
  return result;

[eroman, 2009-11-03 04:30:41]

http://codereview.chromium.org/345034/diff/4008/4011
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/345034/diff/4008/4011#newcode565
Line 565: // Force all requests to go to this server.
On 2009/11/03 04:24:06, eroman wrote:
> Can you enhance this comment section?
> In particular:
>  * This commandline is only for tests, and shouldn't ordinarily be used
>  * This commandline can slow down startup/first-request, as it does
synchronous
> lookups blocking the UI/main thread.
>  * Is the [:port] optional, or required? What happens if it is optional.
>  

Another thing I think you should mention in the comment is that the resolution
happens exactly once.

So for the duration that chrome is running, it will use that same address list
-- we won't see if the DNS mapping on the underlying system has changed.

[Mike Belshe, 2009-11-03 05:01:09]

http://codereview.chromium.org/345034/diff/4008/4011
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/345034/diff/4008/4011#newcode565
Line 565: // Force all requests to go to this server.
On 2009/11/03 04:24:06, eroman wrote:
> Can you enhance this comment section?
> In particular:
>  * This commandline is only for tests, and shouldn't ordinarily be used
>  * This commandline can slow down startup/first-request, as it does
synchronous
> lookups blocking the UI/main thread.
>  * Is the [:port] optional, or required? What happens if it is optional.
>  

Done.

http://codereview.chromium.org/345034/diff/4008/4011#newcode566
Line 566: //   --fixed-server=myserver:1000
On 2009/11/03 04:24:06, eroman wrote:
> Per our discussion, can you come up with a more descriptive name.
> Some ideas:
> 
> --testing-fixed-host-resolver

Done.

http://codereview.chromium.org/345034/diff/4008/4015
File net/base/fixed_host_resolver.cc (right):

http://codereview.chromium.org/345034/diff/4008/4015#newcode21
Line 21: DCHECK(rv == OK);
On 2009/11/03 04:24:06, eroman wrote:
> See comment the later comment about needing to handle failure cases.
> Not doing so can result in crashes (not just debug mode).

Done.

http://codereview.chromium.org/345034/diff/4008/4015#newcode23
Line 23: if (port > 0)
On 2009/11/03 04:24:06, eroman wrote:
> The handling of port seems weird here.
> I see two plausible policies:
> 
> (1) port is not optional in |host_and_port|, so --fixed-server="foobar" is
> considered a failure.
> 
> (2) port is optional in |host_and_port|, so --fixed-server="foobar" is assumed
> to mean "only rewrite the host, but keep the port". If you define things this
> way, then the Resolve() function below needs to return an address list with
the
> proper port.
> 
> 
> What it does right now is undefined, since we don't define what port
> SystemHostResolverProc() will set.

Done.

http://codereview.chromium.org/345034/diff/4008/4015#newcode33
Line 33: *addresses = address_;
On 2009/11/03 04:24:06, eroman wrote:
> The constructor doesn't handle the error cases -- that means if either
> ParseHostAndPort() failed, or SystemHostResolverProc() failed, then |address_|
> will be un-initialized.
> 
> That means if any consumer uses the address, it will crash (NULL-pointer
deref).
> 
> I suggest that instead you save some |int result_| member during
initialization,
> and then here:
> 
> if (result_ != OK)
>   return result;

Done.  It now returns ERR_NAME_NOT_RESOLVED for any failure to initialize.  The
initializers also log to the log file information about what happened.

[eroman, 2009-11-03 05:26:43]

lgtm

http://codereview.chromium.org/345034/diff/12005/9017
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/345034/diff/12005/9017#newcode488
Line 488: std::string host =
for consistency may want to call this |host_and_port|.

http://codereview.chromium.org/345034/diff/12005/9012
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/345034/diff/12005/9012#newcode572
Line 572: //   --fixed-server=myserver:1000
update this to match the new name.

http://codereview.chromium.org/345034/diff/12005/9015
File net/base/fixed_host_resolver.h (right):

http://codereview.chromium.org/345034/diff/12005/9015#newcode15
Line 15: // A FixedHostResolver resovles all addresses to a single address.
typo -> resovles --> resolves.