Implementation of ProxyResolver that uses a Mojo service.

Implementation of ProxyResolver that uses a Mojo service.

BUG=11746
Committed: https://crrev.com/07ff140809b970414d51b484d7a03c440241509b
Cr-Commit-Position: refs/heads/master@{#319848}

[Anand Mistry (off Chromium), 2015-02-16 04:03:49]

amistry@chromium.org changed reviewers:
+ eroman@chromium.org, sammc@chromium.org

[Sam McNally, 2015-02-16 05:52:38]

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
File net/proxy/proxy_resolver_mojo.cc (right):

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.cc:31: int Start();
This Job doesn't need a separate Start().

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.cc:74: // Should already have ensured the Mojo
resolved is connected.
I think this is missing a subject.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.cc:141: pac_script->utf16().empty())
{

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.cc:169: new
MojoHostResolverImpl(host_resolver_)));
This will leak if the host resolver pipe isn't closed before
host_resolver_binding_ is cleared. StrongBinding really does expect to be a
member variable.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.cc:171:
host_resolver_binding_->Bind(&host_resolver_ptr);
Binding can take this as a constructor arg.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.cc:178:
proxy_resolver_ptr_->SetPacScript(mojo::String::From(pac_script_->utf16()),
This and CancelSetPacScript can result in multiple SetPacScript requests
reaching the other side; the current implementation doesn't cope well with that.
We need to decide whether the mojom interface should allow this and fix one
side.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.cc:221: return job->Start();
Why not return ERR_IO_PENDING directly?

To be strictly correct, you should be checking the return value before setting
*request.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
File net/proxy/proxy_resolver_mojo.h (right):

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.h:44: explicit
ProxyResolverMojo(MojoProxyResolverFactory* proxy_resolver_factory,
No explicit.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.h:69: void SetupServices();
SetUpServices

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
File net/proxy/proxy_resolver_mojo_unittest.cc (right):

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo_unittest.cc:228: Request* MakeRequest(const GURL&
url) {
scoped_ptr<Request>

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo_unittest.cc:259:
ProxyResolverScriptData::FromUTF8(kScriptData));
That isn't empty.

[Anand Mistry (off Chromium), 2015-02-17 05:49:22]

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
File net/proxy/proxy_resolver_mojo.cc (right):

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.cc:31: int Start();
On 2015/02/16 05:52:38, Sam McNally wrote:
> This Job doesn't need a separate Start().

Done.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.cc:74: // Should already have ensured the Mojo
resolved is connected.
On 2015/02/16 05:52:38, Sam McNally wrote:
> I think this is missing a subject.

Removed the DCHECK since I don't think it's strictly true, or necessary.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.cc:141: pac_script->utf16().empty())
On 2015/02/16 05:52:38, Sam McNally wrote:
> {

Done.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.cc:169: new
MojoHostResolverImpl(host_resolver_)));
On 2015/02/16 05:52:38, Sam McNally wrote:
> This will leak if the host resolver pipe isn't closed before
> host_resolver_binding_ is cleared. StrongBinding really does expect to be a
> member variable.

Done.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.cc:171:
host_resolver_binding_->Bind(&host_resolver_ptr);
On 2015/02/16 05:52:38, Sam McNally wrote:
> Binding can take this as a constructor arg.

Done.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.cc:178:
proxy_resolver_ptr_->SetPacScript(mojo::String::From(pac_script_->utf16()),
On 2015/02/16 05:52:38, Sam McNally wrote:
> This and CancelSetPacScript can result in multiple SetPacScript requests
> reaching the other side; the current implementation doesn't cope well with
that.
> We need to decide whether the mojom interface should allow this and fix one
> side.

Fixed this side to deal with multiple outstanding SetPacScript requests.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.cc:221: return job->Start();
On 2015/02/16 05:52:38, Sam McNally wrote:
> Why not return ERR_IO_PENDING directly?
> 
> To be strictly correct, you should be checking the return value before setting
> *request.

Done.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
File net/proxy/proxy_resolver_mojo.h (right):

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.h:44: explicit
ProxyResolverMojo(MojoProxyResolverFactory* proxy_resolver_factory,
On 2015/02/16 05:52:38, Sam McNally wrote:
> No explicit.

Done.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo.h:69: void SetupServices();
On 2015/02/16 05:52:38, Sam McNally wrote:
> SetUpServices

Done.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
File net/proxy/proxy_resolver_mojo_unittest.cc (right):

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo_unittest.cc:228: Request* MakeRequest(const GURL&
url) {
On 2015/02/16 05:52:38, Sam McNally wrote:
> scoped_ptr<Request>

Done.

https://codereview.chromium.org/917863005/diff/80001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_mojo_unittest.cc:259:
ProxyResolverScriptData::FromUTF8(kScriptData));
On 2015/02/16 05:52:38, Sam McNally wrote:
> That isn't empty.

Done.

[Sam McNally, 2015-02-17 06:23:09]

https://codereview.chromium.org/917863005/diff/100001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo_unittest.cc (right):

https://codereview.chromium.org/917863005/diff/100001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:179: MockMojoProxyResolver*
GetMockResolver() {
MockMojoProxyResolver& mock_resolver()

https://codereview.chromium.org/917863005/diff/100001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:185: int NumCreateCalls() const {
return num_create_calls_; }
num_create_calls()

https://codereview.chromium.org/917863005/diff/100001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:252: base::RunLoop run_loop_;
The comment for RunLoop says it should be created on the stack.

https://codereview.chromium.org/917863005/diff/100001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:294: return scoped_ptr<Request>(new
Request(proxy_resolver_mojo_.get(), url));
make_scoped_ptr

https://codereview.chromium.org/917863005/diff/100001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:386: pac_script,
callback.callback()));
This should use a different pac script.

[Anand Mistry (off Chromium), 2015-02-18 06:48:52]

https://codereview.chromium.org/917863005/diff/100001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo_unittest.cc (right):

https://codereview.chromium.org/917863005/diff/100001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:179: MockMojoProxyResolver*
GetMockResolver() {
On 2015/02/17 06:23:09, Sam McNally wrote:
> MockMojoProxyResolver& mock_resolver()

Done.

https://codereview.chromium.org/917863005/diff/100001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:185: int NumCreateCalls() const {
return num_create_calls_; }
On 2015/02/17 06:23:09, Sam McNally wrote:
> num_create_calls()

Done.

https://codereview.chromium.org/917863005/diff/100001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:252: base::RunLoop run_loop_;
On 2015/02/17 06:23:09, Sam McNally wrote:
> The comment for RunLoop says it should be created on the stack.

Done.

https://codereview.chromium.org/917863005/diff/100001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:294: return scoped_ptr<Request>(new
Request(proxy_resolver_mojo_.get(), url));
On 2015/02/17 06:23:09, Sam McNally wrote:
> make_scoped_ptr

Done.

https://codereview.chromium.org/917863005/diff/100001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:386: pac_script,
callback.callback()));
On 2015/02/17 06:23:09, Sam McNally wrote:
> This should use a different pac script.

Done.

[eroman, 2015-02-25 00:24:30]

https://codereview.chromium.org/917863005/diff/140001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo.cc (right):

https://codereview.chromium.org/917863005/diff/140001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:157: pac_script_ = pac_script;
I don't think this is the right policy:

  (1) The PAC script can be large, we don't want to have multiple copies of it
around

  (2) There is no guarantee that calling SetPacScript() again with the same
script will succeed. PAC scripts are full programs, and may do weird stuff like
have timing dependencies.


Instead I believe what we need to do is:

  * Return a failure from the ProxyResolver interface if requests are made after
the connection is lost

  * ProxyService should re-apply its settings (it can re-download the PAC
script).




I am OK with starting off with this approach, but it should not be the final one
that we use.

[Anand Mistry (off Chromium), 2015-02-25 02:47:16]

https://codereview.chromium.org/917863005/diff/140001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo.cc (right):

https://codereview.chromium.org/917863005/diff/140001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:157: pac_script_ = pac_script;
On 2015/02/25 00:24:30, eroman wrote:
> I don't think this is the right policy:
> 
>   (1) The PAC script can be large, we don't want to have multiple copies of it
> around
> 
>   (2) There is no guarantee that calling SetPacScript() again with the same
> script will succeed. PAC scripts are full programs, and may do weird stuff
like
> have timing dependencies.
> 
> 
> Instead I believe what we need to do is:
> 
>   * Return a failure from the ProxyResolver interface if requests are made
after
> the connection is lost
> 
>   * ProxyService should re-apply its settings (it can re-download the PAC
> script).

So, the way I see it, I need to make two further changes (in a different CL):
1. Add a new net error code to indicate a fatal error so that ProxyService knows
to re-apply settings. Maybe a PAC_SCRIPT_TERMINATED or PROCESS_CRASHES. Other
suggestions?
2. In ProxyService, call
ProxyService::ResetProxyConfig(true);ProxyService::ApplyProxyConfigIfAvailable()
when that error occurs.

> I am OK with starting off with this approach, but it should not be the final
one
> that we use.

Ack.

[Sam McNally, 2015-02-25 06:33:54]

lgtm

https://codereview.chromium.org/917863005/diff/140001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/917863005/diff/140001/net/net.gypi#newcode1449
net/net.gypi:1449: 'proxy/proxy_resolver_mojo_unittest.cc',
Should there be a corresponding conditional removal of this test file in net.gyp
and BUILD.gn?

[Anand Mistry (off Chromium), 2015-02-25 06:52:27]

https://codereview.chromium.org/917863005/diff/140001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/917863005/diff/140001/net/net.gypi#newcode1449
net/net.gypi:1449: 'proxy/proxy_resolver_mojo_unittest.cc',
On 2015/02/25 06:33:53, Sam McNally wrote:
> Should there be a corresponding conditional removal of this test file in
net.gyp
> and BUILD.gn?

Oops. Done.

[Anand Mistry (off Chromium), 2015-02-27 04:05:48]

Sam: Can you take another look. I've made a couple of significant changes which
I'm back porting from my out-of-process change.

https://codereview.chromium.org/917863005/diff/140001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo.cc (right):

https://codereview.chromium.org/917863005/diff/140001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:157: pac_script_ = pac_script;
On 2015/02/25 02:47:16, Anand Mistry wrote:
> On 2015/02/25 00:24:30, eroman wrote:
> > I don't think this is the right policy:
> > 
> >   (1) The PAC script can be large, we don't want to have multiple copies of
it
> > around
> > 
> >   (2) There is no guarantee that calling SetPacScript() again with the same
> > script will succeed. PAC scripts are full programs, and may do weird stuff
> like
> > have timing dependencies.
> > 
> > 
> > Instead I believe what we need to do is:
> > 
> >   * Return a failure from the ProxyResolver interface if requests are made
> after
> > the connection is lost
> > 
> >   * ProxyService should re-apply its settings (it can re-download the PAC
> > script).
> 
> So, the way I see it, I need to make two further changes (in a different CL):
> 1. Add a new net error code to indicate a fatal error so that ProxyService
knows
> to re-apply settings. Maybe a PAC_SCRIPT_TERMINATED or PROCESS_CRASHES. Other
> suggestions?
> 2. In ProxyService, call
>
ProxyService::ResetProxyConfig(true);ProxyService::ApplyProxyConfigIfAvailable()
> when that error occurs.
> 
> > I am OK with starting off with this approach, but it should not be the final
> one
> > that we use.
> 
> Ack.

I've gone ahead and made this change here. Now, I no longer automatically set
the pac script when re-connecting. Also, the Mojo service is connected to lazily
instead of automatically on startup and disconnection. The main reasons for this
are to avoid creating a utility process if the proxy resolver isn't used, and to
avoid crash loops.

[Sam McNally, 2015-03-02 02:43:06]

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo.cc (right):

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:83: ReportResult(ERR_PAC_SCRIPT_FAILED,
Why report ERR_PAC_SCRIPT_FAILED here? Shouldn't this only be reached if the
utility process crashes and this connection is detected as disconnected first?

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:200: // will happen on the next request.
next SetPacScript request.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:220: // previous request returns
ERR_PAC_SCRIPT_TERMINATED due to the Mojo process
How about "proxy resolver process"?

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo_unittest.cc (right):

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:35: const GURL kExampleUrl =
GURL("http://www.example.com");
No global objects.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:38: public:
Remove.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:61: public:
Remove.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:118: : binding_(this, req.Pass()) {}
Move these out of line for consistency with the other methods.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:128: std::string GetPacScriptData() {
return pac_script_data_; }
pac_script_data()

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:145: std::list<SetPacScriptAction>
pac_script_actions_;
Why not std::queue?

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:534:
mojo_proxy_resolver_factory_.GetMockResolver().WaitForNextRequest();
Can you check that the client pipe is closed in response to the cancel.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:574: SetPacScript(1);
Can you check that making a GetProxyForURL request fails after a disconnect, but
before setting the PAC script again.

[Anand Mistry (off Chromium), 2015-03-02 06:44:00]

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo.cc (right):

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:83: ReportResult(ERR_PAC_SCRIPT_FAILED,
On 2015/03/02 02:43:05, Sam McNally wrote:
> Why report ERR_PAC_SCRIPT_FAILED here? Shouldn't this only be reached if the
> utility process crashes and this connection is detected as disconnected first?

Honestly, I've flip-flopped between the two and can't really decide. You're
right, this should only happen if the utility process crashes, taking down the
reply channel. On the other hand, this is only one request and relies on
particular semantic whereas the main service channel is for the entire service
and in no uncertain terms represents a disconnection from the Mojo service (and
hence process crash).

I've changed this back to TERMINATED since I have no strong opinion either way.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:200: // will happen on the next request.
On 2015/03/02 02:43:05, Sam McNally wrote:
> next SetPacScript request.

Done.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:220: // previous request returns
ERR_PAC_SCRIPT_TERMINATED due to the Mojo process
On 2015/03/02 02:43:05, Sam McNally wrote:
> How about "proxy resolver process"?

Done.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo_unittest.cc (right):

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:35: const GURL kExampleUrl =
GURL("http://www.example.com");
On 2015/03/02 02:43:05, Sam McNally wrote:
> No global objects.

Sigh!

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:38: public:
On 2015/03/02 02:43:06, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:61: public:
On 2015/03/02 02:43:06, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:118: : binding_(this, req.Pass()) {}
On 2015/03/02 02:43:06, Sam McNally wrote:
> Move these out of line for consistency with the other methods.

Done.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:128: std::string GetPacScriptData() {
return pac_script_data_; }
On 2015/03/02 02:43:06, Sam McNally wrote:
> pac_script_data()

Done.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:145: std::list<SetPacScriptAction>
pac_script_actions_;
On 2015/03/02 02:43:06, Sam McNally wrote:
> Why not std::queue?

Done.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:534:
mojo_proxy_resolver_factory_.GetMockResolver().WaitForNextRequest();
On 2015/03/02 02:43:06, Sam McNally wrote:
> Can you check that the client pipe is closed in response to the cancel.

Done.

https://codereview.chromium.org/917863005/diff/220001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:574: SetPacScript(1);
On 2015/03/02 02:43:06, Sam McNally wrote:
> Can you check that making a GetProxyForURL request fails after a disconnect,
but
> before setting the PAC script again.

Done.

[Sam McNally, 2015-03-02 06:51:17]

LGTM

https://codereview.chromium.org/917863005/diff/240001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo_unittest.cc (right):

https://codereview.chromium.org/917863005/diff/240001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:252: std::map<int,
std::list<SetPacScriptAction>> pac_script_actions_;
std::queue here too?

[Anand Mistry (off Chromium), 2015-03-02 07:01:30]

https://codereview.chromium.org/917863005/diff/240001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo_unittest.cc (right):

https://codereview.chromium.org/917863005/diff/240001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo_unittest.cc:252: std::map<int,
std::list<SetPacScriptAction>> pac_script_actions_;
On 2015/03/02 06:51:17, Sam McNally wrote:
> std::queue here too?

No. You can't use std::queue with range-based for loops, and I kinda like those.

[Anand Mistry (off Chromium), 2015-03-02 23:47:12]

Eric: Ping! Also, is there someone else you'd like to see these reviews load
balanced to?

[eroman, 2015-03-06 03:31:34]

Unfortunately the other good net reviewers for this are similarly overloaded :)

I'll try to improve my latency, doing a bulk review pass tonight!

[eroman, 2015-03-06 04:46:14]

lgtm

https://codereview.chromium.org/917863005/diff/240001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo.cc (right):

https://codereview.chromium.org/917863005/diff/240001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:175: void
ProxyResolverMojo::CancelPendingRequests() {
nit: Please call this "AbortPendingRequests" or something other than cancel.

In the network stack "Cancel" has a consistent meaning, and implies that
callbacks are not invoked or something is abandoned.

Since this runs callbacks please use a name that better captures that. Also
running the callback from the destructor is not obvious

https://codereview.chromium.org/917863005/diff/240001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:184: while (!pending_jobs_.empty()) {
This seems incomplete. Running a callback here could cause various other
re-entrancy issues, like calling SetPacScript() again. If that were to happen,
then mojo_proxy_resolver_ptr may not be properly setup (since it is cleared
after returning from CancelPendingRequests.

[Anand Mistry (off Chromium), 2015-03-06 06:54:00]

https://codereview.chromium.org/917863005/diff/240001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo.cc (right):

https://codereview.chromium.org/917863005/diff/240001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:175: void
ProxyResolverMojo::CancelPendingRequests() {
On 2015/03/06 04:46:14, eroman wrote:
> nit: Please call this "AbortPendingRequests" or something other than cancel.
> 
> In the network stack "Cancel" has a consistent meaning, and implies that
> callbacks are not invoked or something is abandoned.
> 
> Since this runs callbacks please use a name that better captures that. Also
> running the callback from the destructor is not obvious

Done.

https://codereview.chromium.org/917863005/diff/240001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.cc:184: while (!pending_jobs_.empty()) {
On 2015/03/06 04:46:14, eroman wrote:
> This seems incomplete. Running a callback here could cause various other
> re-entrancy issues, like calling SetPacScript() again. If that were to happen,
> then mojo_proxy_resolver_ptr may not be properly setup (since it is cleared
> after returning from CancelPendingRequests.

The latter issue of setting up the new connection in SetPacScript() I've
addressed and tested. However, the more general problem of there being
re-entrancy issues is something I don't think can be solved here. This code,
as-is, handles the cases where pending requests might be cancelled in the
callback. For the case where SetPacScript() is called in a callback, this works
now, as long as the caller ensures only one call to SetPacScript() is in flight.
Another case is where GetProxyForURL() is called during a callback. This won't
ever succeed since SetPacScript() needs to complete first which can't happen
inline.

I think multi-process is going to bring up more of these issues that wouldn't
have come up in the past.

[Anand Mistry (off Chromium), 2015-03-10 02:00:13]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-03-10 02:00:14]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/917863005/#ps280001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-03-10 02:00:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/917863005/280001

[commit-bot: I haz the power, 2015-03-10 04:06:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-10 04:06:05]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Anand Mistry (off Chromium), 2015-03-10 05:46:27]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-03-10 05:46:27]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/917863005/#ps300001
(title: "Fix build.")

[commit-bot: I haz the power, 2015-03-10 05:46:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/917863005/300001

[commit-bot: I haz the power, 2015-03-10 07:34:16]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-03-10 07:35:14]

Patchset 16 (id:??) landed as
https://crrev.com/07ff140809b970414d51b484d7a03c440241509b
Cr-Commit-Position: refs/heads/master@{#319848}