Improve performance of proxy resolver by tracing DNS dependencies.

Improve performance of proxy resolver by tracing DNS dependencies.

This replaces the multi-threaded V8 proxy resolver implementation, with a faster single-threaded one. The single-threaded version uses some magic to avoid blocking on DNS dependencies, so it is able to handle more parallel requests than the multi-threaded one.

Design document: https://docs.google.com/a/chromium.org/document/d/16Ij5OcVnR3s0MH4Z5XkhI9VTPoMJdaBn9rKreAmGOdE/edit

This has the benefit of reducing the number of threads that Chrome uses for PAC evaluation, while at the same time speeding up proxy resolving for PAC scripts that do DNS resolving (due to better parallelism).

I ran a benchmark to evaluate the effectiveness of this new approach. The benchmark simulates loading the http://www.newyorktimes.com webpage with slow DNS (where each DNS resolve takes 2 seconds), and a maximum DNS resolver parallelism of 10 requests. This webpage resolves the proxy for 221 URLs, across 40 unique hostnames.

  - Proxy resolving using the old multithreaded code took 24.076 seconds [*]
  - Proxy resolving using the new code took 8.011 seconds [*]
  - Without a PAC script, resolving the DNS took 8.003 seconds

The new proxy resolving times (8.011s) are much closer to the theoretical best (8.003s)!

[*] The PAC script I used for the test was a fairly complex script 20kb (a version of google's corp PAC script modified to always call dnsResolve(host)).

I will be adding histograms in a follow-up CL, to measure how often requests need to be restarted, or fall-back to synchronous mode.

BUG=119151
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179714

[eroman, 2013-01-12 00:45:30]

Matt, could you review this for me?
=)

[mmenke, 2013-01-12 00:51:16]

On 2013/01/12 00:45:30, eroman wrote:
> Matt, could you review this for me?
> =)

Happy to.  May not be able to get back to you until Tuesday, though.

[mmenke, 2013-01-12 00:52:10]

On 2013/01/12 00:51:16, Matt Menke wrote:
> On 2013/01/12 00:45:30, eroman wrote:
> > Matt, could you review this for me?
> > =)
> 
> Happy to.  May not be able to get back to you until Tuesday, though.

Or longer, depending on how much new code I have to learn.  :)

[eroman, 2013-01-12 01:07:18]

No problem, take your time :)
Thanks!

[mmenke, 2013-01-15 17:11:00]

On 2013/01/12 01:07:18, eroman wrote:
> No problem, take your time :)
> Thanks!

Patch set 3 looks to be corrupt.  Since only the gyp was changed, I can still
review set 2.

[eroman, 2013-01-15 23:52:49]

I have re-uploaded to fix corrupt upload in patchset3. Reviewing patchset2 works
too. Thanks

[mmenke, 2013-01-17 20:55:35]

On 2013/01/15 23:52:49, eroman wrote:
> I have re-uploaded to fix corrupt upload in patchset3. Reviewing patchset2
works
> too. Thanks

Finally getting to this now.  Sorry for the delay, was sick for a day or so,
further delaying things.

Maybe we should have a PAC manager.  Sure, there's only one effective PAC at a
time, but we could call it Pacman.  On Windows, it would be MsPacman, of course.

[eroman, 2013-01-17 21:10:39]

Thanks Matt!

Actually I think some sort of PAC manager would be a good thing to have, I will
certainly keep your naming suggestions in mind for it :)

Since right now with multiprofiles what happens is each profile creates its own
ProxyService, which ends up provisioning its own PAC thread. Right now it is
very wasteful since each ProxyService may create up to 2 PAC threads. This
change drops it to 1 thread per ProxyService. However better yet would be to
have 1 PAC thread globally shared by all ProxyServices.

I have been fiddling a bit with this CL to resolve issues coming up on the
trybots. There is one issue remaining on TSAN which doesn't like my cancellation
optimization. I have been discussing this with some experts, and have decided to
simply remove the optimization (this will be a minor adjustment to current
patchset, so feel free to start reviewing!)

[eroman, 2013-01-17 21:11:27]

oops in previous message "2 threads" should have been "4 threads".

[mmenke, 2013-01-17 21:14:44]

On 2013/01/17 21:11:27, eroman wrote:
> oops in previous message "2 threads" should have been "4 threads".

You just meant "2 threads for very large values of 2", no doubt.

Hadn't realized we have one proxy config service per profile, though it makes
sense that there are some things we don't want to share.

Oh, and by "getting to this now", I actually mean I'll probably get back to you
tomorrow, since it'll take a while to review.  Meant to include that in the
original message, but got sidetracked by more important matters.

[mmenke, 2013-01-17 21:15:35]

On 2013/01/17 21:14:44, Matt Menke wrote:
> On 2013/01/17 21:11:27, eroman wrote:
> > oops in previous message "2 threads" should have been "4 threads".
> 
> You just meant "2 threads for very large values of 2", no doubt.
> 
> Hadn't realized we have one proxy config service per profile, though it makes
> sense that there are some things we don't want to share.
> 
> Oh, and by "getting to this now", I actually mean I'll probably get back to
you
> tomorrow, since it'll take a while to review.  Meant to include that in the
> original message, but got sidetracked by more important matters.

(Erm...More important meaning my joke.  Not implying this isn't an important CL
- very nifty idea)

[mmenke, 2013-01-18 19:59:40]

Still have yet to go over the tests.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/multi_threaded_...
File net/proxy/multi_threaded_proxy_resolver.h (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/multi_threaded_...
net/proxy/multi_threaded_proxy_resolver.h:74: class NET_EXPORT_PRIVATE
MultiThreadedProxyResolver
Not getting rid of this?

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8.h (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.h:92: friend class Context;
I believe inner classes don't have to be friends to have access to your guts.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:27: //
https://docs.google.com/a/chromium.org/document/d/16Ij5OcVnR3s0MH4Z5XkhI9VTPo...
"Sorry, the file you have requested does not exist."

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:90: // seems to be misbehaving under the
tracing optimization.
You call these blocking and non-blocking elsewhere.  Suggest a consistent naming
scheme.  I'm happy either way.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:96: // The lifetiime of Jobs does not
exceed that of the ProxyResolverV8Tracing that
nit: lifetime

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:113: const CompletionCallback& callback)
{
I recommend de-inlining all these functions.  With a 600 line class, think
inlining makes it harder to get a high level view of a class.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:120: // initialization has completed
successfully!
Setting the PAC can call scripts?

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:154: // (c) The script is blocked waiting
for DNS
(d)  The script has stopped, and there's a pending DNS lookup on this thread
which will do the DNS lookup and then post a task to the worker thread to start
the script again.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:159: // for correctness, however it does
avoid doing unecessary work.
It is necessary to in the blocking case, isn't it?  Otherwise, we'd hang  on the
next lookup on shutdown (Or fail a CHECK).

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:171: if (pending_dns_.get()) {
nit:  Preferred style is not to use get().

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:185: if (pending_dns_.get())
nit:  Preferred style is not to use get().

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:205: string16 message;
Could you make these const?  Not sure if that would make vector sad or not.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:446: void
LogEventToCurrentRequestAndGlobally(
nit:  Fix indent.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:469: ResolveDnsBlocking(host, op,
output);
nit:  4 space indent.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:520: // If this execution was alread
abandoned can fail right away. Only 1 DNS
nit "already"

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:532: base::MD5Update(&exec_.dns_digest,
base::IntToString(op));
Does this really get us anything we care about over the "exec_.num_dns <=
last_num_dns_" test?

I suppose if your goal in life were to construct a PAC that would fail the MD5
test but pass the first non-cached lookup test, you could manage it, but doesn't
really seem worth the effort to me.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:539: ScheduleRestartWithBlockingDns();
Restarting instead of just switching is primarily because there may be queued
alerts, too, so it's simpler just to restart?

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:642: return;
I don't think this cache hit optimization is worth it, first pass, and may well
not be worth doing at all.

If we do go that route, think we could share a bit more logic with the blocking
route, on the worker thread.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:741: bool nonblocking_dns_;
Wonder if this should be a property of the PAC script.  Then we could detect
PACs that cache results from their different signatures, reset the JS
environment, and run all scripts synchronously.

Believe it would result in us handling all scripts correctly, as opposed to just
those that don't cache results.  Of course, it would make us slower for such
PACs, but at least they'd work.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:741: bool nonblocking_dns_;
Suggest using blocking_dns_ instead.  "!non_blocking_dns_" gets a bit silly.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:748:
scoped_refptr<ProxyResolverScriptData> script_data_;
What do you think of splitting this class into two subclasses, to make it much
clearer which variables and methods live on each thread, and what their
interface is.

Currently, it's not easy to distinguish them, and to be sure there are no
contract violations.

The IOThread half can keep a reference to the worker thread half, so you can get
rid of the Add/RemoveRef calls.

It still wouldn't really have a clear owner, and while a weak_ptr could replace
some cancelled checks, it wouldn't be be able to replace the ones on the worker
thread, so doesn't really clean things up much, just serves to help ensure
thread safety.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.h (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.h:45: ProxyResolverErrorObserver*
error_observer,
May want to take this as a scoped_ptr, just to make the ownership a little more
explicit.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:47: EXPECT_TRUE(ok);
Can't you just do:

EXPECT_TRUE(ok) << "Failed to read file: " << path.value();

[mmenke, 2013-01-18 20:02:39]

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:748:
scoped_refptr<ProxyResolverScriptData> script_data_;
On 2013/01/18 19:59:40, Matt Menke wrote:
> What do you think of splitting this class into two subclasses, to make it much
> clearer which variables and methods live on each thread, and what their
> interface is.
> 
> Currently, it's not easy to distinguish them, and to be sure there are no
> contract violations.
> 
> The IOThread half can keep a reference to the worker thread half, so you can
get
> rid of the Add/RemoveRef calls.
> 
> It still wouldn't really have a clear owner, and while a weak_ptr could
replace
> some cancelled checks, it wouldn't be be able to replace the ones on the
worker
> thread, so doesn't really clean things up much, just serves to help ensure
> thread safety.

If you think that's too ugly, clearly separating out and labeling which
members/methods can be accessed on what thread would be an improvement.

[eroman, 2013-01-23 03:26:02]

https://codereview.chromium.org/11885009/diff/42002/net/proxy/multi_threaded_...
File net/proxy/multi_threaded_proxy_resolver.h (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/multi_threaded_...
net/proxy/multi_threaded_proxy_resolver.h:74: class NET_EXPORT_PRIVATE
MultiThreadedProxyResolver
On 2013/01/18 19:59:40, Matt Menke wrote:
> Not getting rid of this?

I would love to get rid of that, unfortunately there are other consumers of it
:(

The other guys are using it in conjunction with their system proxy resolver
service. I *could* theoretically just drop them into single-threaded mode, but I
would rather approach that with more caution later.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8.h (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.h:92: friend class Context;
On 2013/01/18 19:59:40, Matt Menke wrote:
> I believe inner classes don't have to be friends to have access to your guts.

Done.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:27: //
https://docs.google.com/a/chromium.org/document/d/16Ij5OcVnR3s0MH4Z5XkhI9VTPo...
On 2013/01/18 19:59:40, Matt Menke wrote:
> "Sorry, the file you have requested does not exist."

Are you sure you copied the full line exactly? (it is multi-line). Works for me,
both when signed in and not.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:90: // seems to be misbehaving under the
tracing optimization.
On 2013/01/18 19:59:40, Matt Menke wrote:
> You call these blocking and non-blocking elsewhere.  Suggest a consistent
naming
> scheme.  I'm happy either way.

Done -- renamed everything to blocking/non-blocking.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:96: // The lifetiime of Jobs does not
exceed that of the ProxyResolverV8Tracing that
On 2013/01/18 19:59:40, Matt Menke wrote:
> nit: lifetime

Done.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:113: const CompletionCallback& callback)
{
On 2013/01/18 19:59:40, Matt Menke wrote:
> I recommend de-inlining all these functions.  With a 600 line class, think
> inlining makes it harder to get a high level view of a class.

Done.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:120: // initialization has completed
successfully!
On 2013/01/18 19:59:40, Matt Menke wrote:
> Setting the PAC can call scripts?

Setting the PAC involves "sourcing" the script. It has the sideffect of running
any code in the global scope, which might include function calls.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:154: // (c) The script is blocked waiting
for DNS
On 2013/01/18 19:59:40, Matt Menke wrote:
> (d)  The script has stopped, and there's a pending DNS lookup on this thread
> which will do the DNS lookup and then post a task to the worker thread to
start
> the script again.

Done.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:159: // for correctness, however it does
avoid doing unecessary work.
On 2013/01/18 19:59:40, Matt Menke wrote:
> It is necessary to in the blocking case, isn't it?  Otherwise, we'd hang  on
the
> next lookup on shutdown (Or fail a CHECK).

Yes that is true, cancellation which interrupts a DNS request is mandatory. I'll
adjust the wording by deleting the later phrases. (I don't want to be too
specific or it is hard to read). Mostly that "correctness" issue was interesting
when I did unprotected reads from the worker thread of cancelled_, but that is
no longer the case.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:171: if (pending_dns_.get()) {
On 2013/01/18 19:59:40, Matt Menke wrote:
> nit:  Preferred style is not to use get().

Done.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:185: if (pending_dns_.get())
On 2013/01/18 19:59:40, Matt Menke wrote:
> nit:  Preferred style is not to use get().

Done.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:205: string16 message;
On 2013/01/18 19:59:40, Matt Menke wrote:
> Could you make these const?  Not sure if that would make vector sad or not.

Yeah const makes std::vector sad, because push_back relies on the assigment
operator, which cannot be implicitly generated when the members are const :(

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:446: void
LogEventToCurrentRequestAndGlobally(
On 2013/01/18 19:59:40, Matt Menke wrote:
> nit:  Fix indent.

Done.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:469: ResolveDnsBlocking(host, op,
output);
On 2013/01/18 19:59:40, Matt Menke wrote:
> nit:  4 space indent.

Done.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:520: // If this execution was alread
abandoned can fail right away. Only 1 DNS
On 2013/01/18 19:59:40, Matt Menke wrote:
> nit "already"

Done.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:532: base::MD5Update(&exec_.dns_digest,
base::IntToString(op));
On 2013/01/18 19:59:40, Matt Menke wrote:
> Does this really get us anything we care about over the "exec_.num_dns <=
> last_num_dns_" test?
> 
> I suppose if your goal in life were to construct a PAC that would fail the MD5
> test but pass the first non-cached lookup test, you could manage it, but
doesn't
> really seem worth the effort to me.

I chose to enforce the sequence of all DNS requests received, since that is the
strongest check I can do. I could have chosen something weaker like the sequence
of unique hostnames, but the implementation for those is actually more
complicated (since you would need sets).

As mentioned in the comment, saving a strict history could be expensive, since
scripts might have hundreds of "cache hits" (i.e. repeated calls to dnsResolve
with the same hostname).

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:539: ScheduleRestartWithBlockingDns();
On 2013/01/18 19:59:40, Matt Menke wrote:
> Restarting instead of just switching is primarily because there may be queued
> alerts, too, so it's simpler just to restart?

Correct, restarting is to keep things simple. There are various bits of state
that would need to be twiddled with to support resuming in blocking mode.

Since this isn't supposed to happen often (at all), the slight performance
penalty is worth the code maintainability I think.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:642: return;
On 2013/01/18 19:59:40, Matt Menke wrote:
> I don't think this cache hit optimization is worth it, first pass, and may
well
> not be worth doing at all.
> 
> If we do go that route, think we could share a bit more logic with the
blocking
> route, on the worker thread.

This cuts down quite a few restarts, and IMO is definitely worth it.

Commonly PAC scripts will contain both a call to myIpAddress() and
dnsResolve(host). The IP address one is extremely likely to be serviced from the
cache.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:741: bool nonblocking_dns_;
On 2013/01/18 19:59:40, Matt Menke wrote:
> Wonder if this should be a property of the PAC script.  Then we could detect
> PACs that cache results from their different signatures, reset the JS
> environment, and run all scripts synchronously.
> 
> Believe it would result in us handling all scripts correctly, as opposed to
just
> those that don't cache results.  Of course, it would make us slower for such
> PACs, but at least they'd work.

To make sure I understand:

Do you mean keep track of when a request falls back to blocking mode, and avoid
ever running requests for that PAC script in non-blocking mode again?

I don't think that alone is worth it. Here is why:

For starters, I don't believe such scripts truly exist in the wild, although as
a follow-up I would like to histogram this.

If such scripts are a reality and worth optimizing for, then we will need to do
better ==> right now by falling back to synchronous mode we have effectively
reduced the parallelism from 4 under MultiThreadedProxyResolver to just 1. So a
better fallback strategy would be to use MultiThreadedProxyResolver for such
scripts.

But yeah, I am hoping we won't need to do that, since I can't imagine legitimate
scripts behaving this way.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:741: bool nonblocking_dns_;
On 2013/01/18 19:59:40, Matt Menke wrote:
> Suggest using blocking_dns_ instead.  "!non_blocking_dns_" gets a bit silly.

Done.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:748:
scoped_refptr<ProxyResolverScriptData> script_data_;
On 2013/01/18 19:59:40, Matt Menke wrote:
> What do you think of splitting this class into two subclasses, to make it much
> clearer which variables and methods live on each thread, and what their
> interface is.
> 
> Currently, it's not easy to distinguish them, and to be sure there are no
> contract violations.
> 
> The IOThread half can keep a reference to the worker thread half, so you can
get
> rid of the Add/RemoveRef calls.
> 
> It still wouldn't really have a clear owner, and while a weak_ptr could
replace
> some cancelled checks, it wouldn't be be able to replace the ones on the
worker
> thread, so doesn't really clean things up much, just serves to help ensure
> thread safety.

I couldn't get it any cleaner splitting off into subclasses, since there is
still a fair bit of overlapping state (that lives between both threads).

I have re-organized the state variables, added more comments, and split up more
methods into *Blocking *NonBlocking versions so there is less |if
(blocking_dns_)| to follow. Hopefully this is easier to read.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:47: EXPECT_TRUE(ok);
On 2013/01/18 19:59:40, Matt Menke wrote:
> Can't you just do:
> 
> EXPECT_TRUE(ok) << "Failed to read file: " << path.value();

Done.

[mmenke, 2013-01-23 20:03:05]

Sorry again for my extreme slowness in reviewing this.  Doing another pass now,
but want to get a better understanding of the failure modes before getting back
to you, so probably won't send you anything until tomorrow.

[mmenke, 2013-01-24 21:06:33]

Sorry for being slow.  Think I have a solid grasp of the code in context, now.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:27: //
https://docs.google.com/a/chromium.org/document/d/16Ij5OcVnR3s0MH4Z5XkhI9VTPo...
On 2013/01/23 03:26:02, eroman wrote:
> On 2013/01/18 19:59:40, Matt Menke wrote:
> > "Sorry, the file you have requested does not exist."
> 
> Are you sure you copied the full line exactly? (it is multi-line). Works for
me,
> both when signed in and not.

Oops...A straight copy gets the spaces inserted by the Rietveld layout code. 
Shockingly, docs.google.com isn't smart enough to remove random extra spaces in
the URL.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:532: base::MD5Update(&exec_.dns_digest,
base::IntToString(op));
On 2013/01/23 03:26:02, eroman wrote:
> On 2013/01/18 19:59:40, Matt Menke wrote:
> > Does this really get us anything we care about over the "exec_.num_dns <=
> > last_num_dns_" test?
> > 
> > I suppose if your goal in life were to construct a PAC that would fail the
MD5
> > test but pass the first non-cached lookup test, you could manage it, but
> doesn't
> > really seem worth the effort to me.
> 
> I chose to enforce the sequence of all DNS requests received, since that is
the
> strongest check I can do. I could have chosen something weaker like the
sequence
> of unique hostnames, but the implementation for those is actually more
> complicated (since you would need sets).
> 
> As mentioned in the comment, saving a strict history could be expensive, since
> scripts might have hundreds of "cache hits" (i.e. repeated calls to dnsResolve
> with the same hostname).

I was just thinking that it's not too likely that this test would fail in the
case where the "exec_.num_dns <= last_num_dns_" test below passes.

My only real complaints about this are added complexity of two early exit cases.
 Nothing against using MD5s.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:539: ScheduleRestartWithBlockingDns();
On 2013/01/23 03:26:02, eroman wrote:
> On 2013/01/18 19:59:40, Matt Menke wrote:
> > Restarting instead of just switching is primarily because there may be
queued
> > alerts, too, so it's simpler just to restart?
> 
> Correct, restarting is to keep things simple. There are various bits of state
> that would need to be twiddled with to support resuming in blocking mode.
> 
> Since this isn't supposed to happen often (at all), the slight performance
> penalty is worth the code maintainability I think.

I was thinking that you could get rid of "ScheduleRestartWithBlockingDns()",
saving a little code and simplifying things a little, but with the extra state,
doesn't simplify things as I had hoped.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:642: return;
On 2013/01/23 03:26:02, eroman wrote:
> On 2013/01/18 19:59:40, Matt Menke wrote:
> > I don't think this cache hit optimization is worth it, first pass, and may
> well
> > not be worth doing at all.
> > 
> > If we do go that route, think we could share a bit more logic with the
> blocking
> > route, on the worker thread.
> 
> This cuts down quite a few restarts, and IMO is definitely worth it.
> 
> Commonly PAC scripts will contain both a call to myIpAddress() and
> dnsResolve(host). The IP address one is extremely likely to be serviced from
the
> cache.

Suppose it depends on how expensive script execution is and how expensive
waiting on roundtrips to the IO thread are.  If we didn't block, we could be
running the next several couple PAC scripts in the queue, and posting tasks to
get what addresses they need.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.cc:699: 
DCHECK(js_bindings_);?

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.cc:731: 
DCHECK(js_bindings);?

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8.h (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.h:92: friend class Context;
nit:  No need to friend an inner class.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:104: // spawned this Job.
Suggest separating into two sentences, or making it a compound sentence.

Kept on reading this as "|parent| is [non-owned and references ...]" rather than
"|parent [is non-owned] and [references ...]"

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:107: void StartSetPacScript(
Suggest a comment either here or in the class description that the public
functions are all only called on the origin thread.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:149: void Start(Operation op, bool
blocking_dns, const CompletionCallback& callback);
nit:  Line too long.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:300: Start(SET_PAC_SCRIPT, true
/*blocking*/, callback);
If the PAC script changes to a new PAC script, what about ongoing PAC lookups? 
If they aren't cancelled elsewhere, they all may become blocking if the new
script doesn't match the old one, right?

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:320: // There are three main possibility
to consider for cancellation:
five?

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:321: // (a) The job has been posted to
the worker thread, but not yet started
nit:  started->[re]started

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:324: // (c) The script has stopped and
there is a pending DNS request.
nit:  (d)

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:324: // (c) The script has stopped and
there is a pending DNS request.
(e) The script has a pending NotifyCallerOnOriginLoop.

Worth testing this?  Can't be 100 percent sure we're in this state, but can be
fairly sure.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:603: if (dns_cache_.size() >
kMaxUniqueResolveDnsPerExec) {
No test goes through here, I believe.  This code is complicated enough that I
believe we should try and hit every case in tests that we reasonably can.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:647: if (!blocking_dns_) {
Rather than have all this cruft here, suggest moving this block to below the
Resolve() call and checking for ERR_IO_PENDING rather than ERR_CACHE_MISS. 
Saves you a couple lines of code, and the double call into the HostResolver.

Could even have cache hits use OnDnsOperationComplete on cache hits and follow
the blocking_ path if cache_hit_ is true (Which should be made a class
variable).  Then you could get rid of this block, always set cache_hit_ based on
whether or not result is ERR_IO_PENDING, and just call event_.Signal() on cache
misses when not blocking.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:658: SaveDnsToLocalCache(host, op,
result, addresses);
HostResolver cache hit case doesn't seem to be tested (Not to be confused with
"local cache" hit), either in the blocking or non-blocking case.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:695: worker_loop()->PostTask(FROM_HERE,
base::Bind(&Job::ExecuteNonBlocking, this));
nit:  Line too long

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:702: void
ProxyResolverV8Tracing::Job::ScheduleRestartWithBlockingDns() {
In the interest of failing early, suggest:

DCHECK(!blocking_dns_);
DCHECK(!abandoned_);

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:702: void
ProxyResolverV8Tracing::Job::ScheduleRestartWithBlockingDns() {
Any reason not to just set abandoned_ to true here, rather than just before
every call to this function?

Could set blocking_dns_ to true, too, if you wanted (And get rid of
RestartWithBlockingDns()), though that doesn't give us too much.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:702: void
ProxyResolverV8Tracing::Job::ScheduleRestartWithBlockingDns() {
CheckIsOnWorkerThread(); for documentation purposes.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:707: void
ProxyResolverV8Tracing::Job::RestartWithBlockingDns() {
CheckIsOnWorkerThread();

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:717: bool* return_value) {
CheckIsOnWorkerThread();

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:731: const net::AddressList& addresses) {
"CheckIsOnOriginThread();" for documentation purposes.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:801: alerts_and_errors_byte_cost_ +=
message.size() * 2;
Should we toss in a sizeof(AlertOrError)?  Otherwise, could add a bunch of
0-length messages until Bad Things happen.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:827: bool is_alert, int line_number,
const string16& message) {
Suggest a CheckIsOnWorkerThread() for documentation reasons.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:847: NetLog::StringCallback("message",
&message));
So we basically just silently consume PAC script alerts?  So we're supporting
them only for debugging Chrome itself?

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:890: v8_resolver_.reset(new
ProxyResolverV8);
Any compelling reason not to include this and thread_'s initialization in the
initializer list?

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:32:
scoped_refptr<ProxyResolverScriptData> LoadData(const char* filename) {
nit:  Suggest "LoadScript" or "LoadScriptData", since I think the "Script" part
of "ProxyResolverScriptData" is a clearer identifier than "Data".

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:62:
resolver->SetPacScript(LoadData(filename), callback.callback());
nit:  Looks to me like this will fit on one line, and be exactly as long as the
function declaration.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:79: CapturingBoundNetLog
req_log;
nit:  Google C++ style guide is pretty anti-abbreviation.  Think request is
short enough to just write it out.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:82: ProxyResolverV8Tracing
resolver(&host_resolver, error_observer, &log);
May want to put some of these in a text fixture.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:94: EXPECT_EQ(OK,
callback.WaitForResult());
Could also put these in a function to make the tests a little simpler.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:97:
proxy_info.proxy_server().ToURI());
nit:  Can fit on a single line.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:110:
TEST(ProxyResolverV8TracingTest, JavascriptError) {
Worth testing alert and then an error, in the non-blocking case?  Could see a
bug occurring where the alerts were dropped and just the error reported.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:152:
TEST(ProxyResolverV8TracingTest, TooManyAlerts) {
Suggest alerts with 0-length bodies, just to be complete.

Can you get multiple error messages?  If so, should there be an equivalent error
message test?

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:175:
proxy_info.proxy_server().ToURI());
nit:  Can fit on one less line.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:295: // The script itself only
does 2 DNS resolves, however it constructs the
nit:  "2 DNS resolves per execution"

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:352:
proxy_info.proxy_server().ToURI());
nit:  Fits on a single line.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:418: CHECK(false);
Can't we just do expect/assert here?

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:425:
TEST(ProxyResolverV8TracingTest, CancelAll) {
Suggest a more deterministic cancellation test, which should hopefully fail in a
more predictable manner on regressions, in addition to this test.

Just copy this test, but only use two requests.  Wait some brief period (10
milliseconds?  50?) before cancelling the requests, and cancel them in reverse
order.  Second request (first deleted) will be deleted while it's a queued task.
 First request will presumably be deleted while it's waiting for a DNS lookup to
check the cache.

Could actually use more requests, but two is all you need.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:425:
TEST(ProxyResolverV8TracingTest, CancelAll) {
Another potential test, looking for clean destruction:

Send a request, wait a bit, cancel the request, then delete the resolver, and
run the message loop.

Possibly with a variant - have the final notification that resolution was
completed pending.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:447: }
Suggest a MessageLoop::RunAllPending(); at the end of these tests, particularly
if the net test fixture doesn't do it.  If the next test fixture does do it,
still think for completeness, we should have a test that does it (Cancelled and
the resolver exists when the message gets to the IO thread, vs cancelled and it
doesn't).

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:470: base::Bind(&CrashCallback),
&request1, BoundNetLog());
EXPECT_EQ(ERR_IO_PENDING, rv);?

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:480: }
Looks like we never cancel a request that has an outstanding completion task in
flight.  While we can't be completely sure a request is in this state (As
opposed to pending on the other thread, or executing on the other thread), I
still think it's worth doing the best we can.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_unittest.cc:46: }
Remove duplicated code block.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_unittest.cc:99: ProxyResolverV8WithMockBindings() :
ProxyResolverV8() {
nit:  ProxyResolverV8() not needed.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_unittest.cc:131: private:
nit:  Add blank line before private

[eroman, 2013-01-24 22:13:58]

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:532: base::MD5Update(&exec_.dns_digest,
base::IntToString(op));
On 2013/01/24 21:06:33, Matt Menke wrote:
> On 2013/01/23 03:26:02, eroman wrote:
> > On 2013/01/18 19:59:40, Matt Menke wrote:
> > > Does this really get us anything we care about over the "exec_.num_dns <=
> > > last_num_dns_" test?
> > > 
> > > I suppose if your goal in life were to construct a PAC that would fail the
> MD5
> > > test but pass the first non-cached lookup test, you could manage it, but
> > doesn't
> > > really seem worth the effort to me.
> > 
> > I chose to enforce the sequence of all DNS requests received, since that is
> the
> > strongest check I can do. I could have chosen something weaker like the
> sequence
> > of unique hostnames, but the implementation for those is actually more
> > complicated (since you would need sets).
> > 
> > As mentioned in the comment, saving a strict history could be expensive,
since
> > scripts might have hundreds of "cache hits" (i.e. repeated calls to
dnsResolve
> > with the same hostname).
> 
> I was just thinking that it's not too likely that this test would fail in the
> case where the "exec_.num_dns <= last_num_dns_" test below passes.
> 
> My only real complaints about this are added complexity of two early exit
cases.
>  Nothing against using MD5s.

Thanks for discussing this issue!

Indeed, none of the tests I have right now need the MD5 check to succeed.

You are correct that constructing a PAC script that triggers this requires quite
a bit of craziness, and may not be worth doing the extra work for (could rely on
the kMaxUniqueResolveDnsPerExec detecting a problem in those cases).

I will start by adding at least one unittest that triggers this situation, and
then re-evaluate if enforcing same-ordering makes sense.

I suspect you are right, and we shouldn't bother MD5-ing. If these scripts do
exist and cause problems, then users will be best served if we fall-back to
synchronous mode for all requests anyway.

[eroman, 2013-01-24 22:53:00]

OK I am convinced that you are right, the MD5 is unnecessary. I will remove it!

A pathological case where enforcing the sequence matters would be a PAC script
whose DNS resolves depends on the count of how many times it has been called
before, and where the preceding hostnames are re-ordered. For instance:


-----------
Iteration 1:
-----------
  dnsResolve('host1')
-----------
Iteration 2:
-----------
  dnsResolve('host1')
  dnsResolve('host2')
-----------
Iteration 3:
-----------
  dnsResolve('host2')
  dnsResolve('host1')
  dnsResolve('host3')
-----------
Iteration 4:
-----------
  dnsResolve('host2')
  dnsResolve('host3')
  dnsResolve('host1')
  dnsResolve('host4')
-----------
Iteration 5:
-----------
  dnsResolve('host2')
  dnsResolve('host3')
  dnsResolve('host4')
  dnsResolve('host1')
  dnsResolve('host5')
-----------
Iteration 5:
-----------
  dnsResolve('host2')
  dnsResolve('host3')
  dnsResolve('host4')
  dnsResolve('host5')
  dnsResolve('host1')
  dnsResolve('host6')

...


In this case, the fact that the ordering changes between consecutive executions
is a signal that the script relies on sideffects, and with it we would fall-back
to synchronous after Iteration 3, and hence would complete in Iteration 4.

This is a small victory though, since if the sequence never ends the synchronous
execution will be getting progressively worse too.

For all intents and purposes such a PAC script is supremely broken, and will
cause problems with or without our optimization.

[mattmenke_gmail.com, 2013-01-24 22:57:25]

On 2013/01/24 22:53:00, eroman wrote:
> OK I am convinced that you are right, the MD5 is unnecessary. I will remove
it!
> 
> A pathological case where enforcing the sequence matters would be a PAC script
> whose DNS resolves depends on the count of how many times it has been called
> before, and where the preceding hostnames are re-ordered. For instance:
> 
> 
> -----------
> Iteration 1:
> -----------
>   dnsResolve('host1')
> -----------
> Iteration 2:
> -----------
>   dnsResolve('host1')
>   dnsResolve('host2')
> -----------
> Iteration 3:
> -----------
>   dnsResolve('host2')
>   dnsResolve('host1')
>   dnsResolve('host3')
> -----------
> Iteration 4:
> -----------
>   dnsResolve('host2')
>   dnsResolve('host3')
>   dnsResolve('host1')
>   dnsResolve('host4')
> -----------
> Iteration 5:
> -----------
>   dnsResolve('host2')
>   dnsResolve('host3')
>   dnsResolve('host4')
>   dnsResolve('host1')
>   dnsResolve('host5')
> -----------
> Iteration 5:
> -----------
>   dnsResolve('host2')
>   dnsResolve('host3')
>   dnsResolve('host4')
>   dnsResolve('host5')
>   dnsResolve('host1')
>   dnsResolve('host6')
> 
> ...
> 
> 
> In this case, the fact that the ordering changes between consecutive
executions
> is a signal that the script relies on sideffects, and with it we would
fall-back
> to synchronous after Iteration 3, and hence would complete in Iteration 4.
> 
> This is a small victory though, since if the sequence never ends the
synchronous
> execution will be getting progressively worse too.
> 
> For all intents and purposes such a PAC script is supremely broken, and will
> cause problems with or without our optimization.

I was thinking along different lines:

var g_iteration = 0;

++g_iteration;
for (var i = 0; i < g_iteration; ++i) {
  myIpAddress();  // Just for kicks.
}

dnsResolve('host' + g_iteration);

[mattmenke_gmail.com, 2013-01-24 22:58:24]

On 2013/01/24 22:57:25, mattmenke wrote:
> On 2013/01/24 22:53:00, eroman wrote:
> > OK I am convinced that you are right, the MD5 is unnecessary. I will remove
> it!
> > 
> > A pathological case where enforcing the sequence matters would be a PAC
script
> > whose DNS resolves depends on the count of how many times it has been called
> > before, and where the preceding hostnames are re-ordered. For instance:
> > 
> > 
> > -----------
> > Iteration 1:
> > -----------
> >   dnsResolve('host1')
> > -----------
> > Iteration 2:
> > -----------
> >   dnsResolve('host1')
> >   dnsResolve('host2')
> > -----------
> > Iteration 3:
> > -----------
> >   dnsResolve('host2')
> >   dnsResolve('host1')
> >   dnsResolve('host3')
> > -----------
> > Iteration 4:
> > -----------
> >   dnsResolve('host2')
> >   dnsResolve('host3')
> >   dnsResolve('host1')
> >   dnsResolve('host4')
> > -----------
> > Iteration 5:
> > -----------
> >   dnsResolve('host2')
> >   dnsResolve('host3')
> >   dnsResolve('host4')
> >   dnsResolve('host1')
> >   dnsResolve('host5')
> > -----------
> > Iteration 5:
> > -----------
> >   dnsResolve('host2')
> >   dnsResolve('host3')
> >   dnsResolve('host4')
> >   dnsResolve('host5')
> >   dnsResolve('host1')
> >   dnsResolve('host6')
> > 
> > ...
> > 
> > 
> > In this case, the fact that the ordering changes between consecutive
> executions
> > is a signal that the script relies on sideffects, and with it we would
> fall-back
> > to synchronous after Iteration 3, and hence would complete in Iteration 4.
> > 
> > This is a small victory though, since if the sequence never ends the
> synchronous
> > execution will be getting progressively worse too.
> > 
> > For all intents and purposes such a PAC script is supremely broken, and will
> > cause problems with or without our optimization.
> 
> I was thinking along different lines:
> 
> var g_iteration = 0;
> 
> ++g_iteration;
> for (var i = 0; i < g_iteration; ++i) {
>   myIpAddress();  // Just for kicks.
> }
> 
> dnsResolve('host' + g_iteration);

Err...I was thinking along different lines when looking at weird cases that
would fail in one situation, but not the other, that is.

[mmenke, 2013-01-24 23:00:10]

On 2013/01/24 22:58:24, mattmenke wrote:
> On 2013/01/24 22:57:25, mattmenke wrote:
> > On 2013/01/24 22:53:00, eroman wrote:
> > > OK I am convinced that you are right, the MD5 is unnecessary. I will
remove
> > it!
> > > 
> > > A pathological case where enforcing the sequence matters would be a PAC
> script
> > > whose DNS resolves depends on the count of how many times it has been
called
> > > before, and where the preceding hostnames are re-ordered. For instance:
> > > 
> > > 
> > > -----------
> > > Iteration 1:
> > > -----------
> > >   dnsResolve('host1')
> > > -----------
> > > Iteration 2:
> > > -----------
> > >   dnsResolve('host1')
> > >   dnsResolve('host2')
> > > -----------
> > > Iteration 3:
> > > -----------
> > >   dnsResolve('host2')
> > >   dnsResolve('host1')
> > >   dnsResolve('host3')
> > > -----------
> > > Iteration 4:
> > > -----------
> > >   dnsResolve('host2')
> > >   dnsResolve('host3')
> > >   dnsResolve('host1')
> > >   dnsResolve('host4')
> > > -----------
> > > Iteration 5:
> > > -----------
> > >   dnsResolve('host2')
> > >   dnsResolve('host3')
> > >   dnsResolve('host4')
> > >   dnsResolve('host1')
> > >   dnsResolve('host5')
> > > -----------
> > > Iteration 5:
> > > -----------
> > >   dnsResolve('host2')
> > >   dnsResolve('host3')
> > >   dnsResolve('host4')
> > >   dnsResolve('host5')
> > >   dnsResolve('host1')
> > >   dnsResolve('host6')
> > > 
> > > ...
> > > 
> > > 
> > > In this case, the fact that the ordering changes between consecutive
> > executions
> > > is a signal that the script relies on sideffects, and with it we would
> > fall-back
> > > to synchronous after Iteration 3, and hence would complete in Iteration 4.
> > > 
> > > This is a small victory though, since if the sequence never ends the
> > synchronous
> > > execution will be getting progressively worse too.
> > > 
> > > For all intents and purposes such a PAC script is supremely broken, and
will
> > > cause problems with or without our optimization.
> > 
> > I was thinking along different lines:
> > 
> > var g_iteration = 0;
> > 
> > ++g_iteration;
> > for (var i = 0; i < g_iteration; ++i) {
> >   myIpAddress();  // Just for kicks.
> > }
> > 
> > dnsResolve('host' + g_iteration);
> 
> Err...I was thinking along different lines when looking at weird cases that
> would fail in one situation, but not the other, that is.

Oops.  Think that's the first time I've ever used my personal account on
codereview.  I'll never live down the shame!  Think I've used it on
code.google.com once or twice, though.

[eroman, 2013-01-24 23:07:15]

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:642: return;
On 2013/01/24 21:06:33, Matt Menke wrote:
> On 2013/01/23 03:26:02, eroman wrote:
> > On 2013/01/18 19:59:40, Matt Menke wrote:
> > > I don't think this cache hit optimization is worth it, first pass, and may
> > well
> > > not be worth doing at all.
> > > 
> > > If we do go that route, think we could share a bit more logic with the
> > blocking
> > > route, on the worker thread.
> > 
> > This cuts down quite a few restarts, and IMO is definitely worth it.
> > 
> > Commonly PAC scripts will contain both a call to myIpAddress() and
> > dnsResolve(host). The IP address one is extremely likely to be serviced from
> the
> > cache.
> 
> Suppose it depends on how expensive script execution is and how expensive
> waiting on roundtrips to the IO thread are.  If we didn't block, we could be
> running the next several couple PAC scripts in the queue, and posting tasks to
> get what addresses they need.

One final comment on this issue:

Waiting for the IO thread should be very short. In which case waiting for the
cached value makes sense whether or not the script execution is slow (since it
just waits an infinitesimal amount of time).

If on the other hand waiting for the task to reach the IO thread is NOT short,
then this suggests there is a slowdown happening on the IO thread.

If there is a slowdown on the IO thread, then there will also be a slowdown in
processing our DNS requests, and there will be a slowdown in receiving
simultaneous proxy resolve requests. So the benefit to restarting the PAC script
to discover other request's DNS also decreases.

[mmenke, 2013-01-24 23:10:41]

On 2013/01/24 23:07:15, eroman wrote:
>
https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
> File net/proxy/proxy_resolver_v8_tracing.cc (right):
> 
>
https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
> net/proxy/proxy_resolver_v8_tracing.cc:642: return;
> On 2013/01/24 21:06:33, Matt Menke wrote:
> > On 2013/01/23 03:26:02, eroman wrote:
> > > On 2013/01/18 19:59:40, Matt Menke wrote:
> > > > I don't think this cache hit optimization is worth it, first pass, and
may
> > > well
> > > > not be worth doing at all.
> > > > 
> > > > If we do go that route, think we could share a bit more logic with the
> > > blocking
> > > > route, on the worker thread.
> > > 
> > > This cuts down quite a few restarts, and IMO is definitely worth it.
> > > 
> > > Commonly PAC scripts will contain both a call to myIpAddress() and
> > > dnsResolve(host). The IP address one is extremely likely to be serviced
from
> > the
> > > cache.
> > 
> > Suppose it depends on how expensive script execution is and how expensive
> > waiting on roundtrips to the IO thread are.  If we didn't block, we could be
> > running the next several couple PAC scripts in the queue, and posting tasks
to
> > get what addresses they need.
> 
> One final comment on this issue:
> 
> Waiting for the IO thread should be very short. In which case waiting for the
> cached value makes sense whether or not the script execution is slow (since it
> just waits an infinitesimal amount of time).
> 
> If on the other hand waiting for the task to reach the IO thread is NOT short,
> then this suggests there is a slowdown happening on the IO thread.
> 
> If there is a slowdown on the IO thread, then there will also be a slowdown in
> processing our DNS requests, and there will be a slowdown in receiving
> simultaneous proxy resolve requests. So the benefit to restarting the PAC
script
> to discover other request's DNS also decreases.

I may have been thinking script re-execution is lighter weight than it really is
(Or than it may be, with poorly written scripts and on slower devices).

[eroman, 2013-01-25 03:02:01]

Thanks for your detailed review comments!

I have uploaded a patch that addresses most of the comments 

(I have not yet addressed all the cancellation unittest comments).

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.cc:699: 
On 2013/01/24 21:06:33, Matt Menke wrote:
> DCHECK(js_bindings_);?

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.cc:731: 
On 2013/01/24 21:06:33, Matt Menke wrote:
> DCHECK(js_bindings);?

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8.h (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.h:92: friend class Context;
On 2013/01/24 21:06:33, Matt Menke wrote:
> nit:  No need to friend an inner class.

But I want more friends :(
So lonely

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:104: // spawned this Job.
On 2013/01/24 21:06:33, Matt Menke wrote:
> Suggest separating into two sentences, or making it a compound sentence.
> 
> Kept on reading this as "|parent| is [non-owned and references ...]" rather
than
> "|parent [is non-owned] and [references ...]"

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:107: void StartSetPacScript(
On 2013/01/24 21:06:33, Matt Menke wrote:
> Suggest a comment either here or in the class description that the public
> functions are all only called on the origin thread.

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:149: void Start(Operation op, bool
blocking_dns, const CompletionCallback& callback);
On 2013/01/24 21:06:33, Matt Menke wrote:
> nit:  Line too long.

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:300: Start(SET_PAC_SCRIPT, true
/*blocking*/, callback);
On 2013/01/24 21:06:33, Matt Menke wrote:
> If the PAC script changes to a new PAC script, what about ongoing PAC lookups?

> If they aren't cancelled elsewhere, they all may become blocking if the new
> script doesn't match the old one, right?

There shouldn't be any outstanding PAC requests when calling SetPacScript
(proxy_service.cc is supposed to cancel everything before calling it).

I'll add some comments to that effect for now. I could probably also add a
counter for in-progress jobs to assert this fact, what do you think?

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:320: // There are three main possibility
to consider for cancellation:
On 2013/01/24 21:06:33, Matt Menke wrote:
> five?

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:321: // (a) The job has been posted to
the worker thread, but not yet started
On 2013/01/24 21:06:33, Matt Menke wrote:
> nit:  started->[re]started

What i meant here was the first execution, hence "started". the "restart" case
probably falls under (d).

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:324: // (c) The script has stopped and
there is a pending DNS request.
On 2013/01/24 21:06:33, Matt Menke wrote:
> (e) The script has a pending NotifyCallerOnOriginLoop.
> 
> Worth testing this?  Can't be 100 percent sure we're in this state, but can be
> fairly sure.

This can probabilistic-ally happen in the CancelSome and CancelAll tests. I
could add some sleeps to make it more likely, what do you think?

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:603: if (dns_cache_.size() >
kMaxUniqueResolveDnsPerExec) {
On 2013/01/24 21:06:33, Matt Menke wrote:
> No test goes through here, I believe.  This code is complicated enough that I
> believe we should try and hit every case in tests that we reasonably can.

Thanks. Now covered by the InfiniteDNSSequence* tests.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:647: if (!blocking_dns_) {
On 2013/01/24 21:06:33, Matt Menke wrote:
> Rather than have all this cruft here, suggest moving this block to below the
> Resolve() call and checking for ERR_IO_PENDING rather than ERR_CACHE_MISS. 
> Saves you a couple lines of code, and the double call into the HostResolver.
> 
> Could even have cache hits use OnDnsOperationComplete on cache hits and follow
> the blocking_ path if cache_hit_ is true (Which should be made a class
> variable).  Then you could get rid of this block, always set cache_hit_ based
on
> whether or not result is ERR_IO_PENDING, and just call event_.Signal() on
cache
> misses when not blocking.

Done -- Good idea, I have removed the duplicate calls into HostResolver.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:658: SaveDnsToLocalCache(host, op,
result, addresses);
On 2013/01/24 21:06:33, Matt Menke wrote:
> HostResolver cache hit case doesn't seem to be tested (Not to be confused with
> "local cache" hit), either in the blocking or non-blocking case.

Thanks! Added a test to exercise this: DnsChecksCache

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:695: worker_loop()->PostTask(FROM_HERE,
base::Bind(&Job::ExecuteNonBlocking, this));
On 2013/01/24 21:06:33, Matt Menke wrote:
> nit:  Line too long

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:702: void
ProxyResolverV8Tracing::Job::ScheduleRestartWithBlockingDns() {
On 2013/01/24 21:06:33, Matt Menke wrote:
> In the interest of failing early, suggest:
> 
> DCHECK(!blocking_dns_);
> DCHECK(!abandoned_);

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:702: void
ProxyResolverV8Tracing::Job::ScheduleRestartWithBlockingDns() {
On 2013/01/24 21:06:33, Matt Menke wrote:
> Any reason not to just set abandoned_ to true here, rather than just before
> every call to this function?
> 
> Could set blocking_dns_ to true, too, if you wanted (And get rid of
> RestartWithBlockingDns()), though that doesn't give us too much.

I can't set blocking_dns_ to true, because we may still be executing inside of
the proxy resolver. If DnsResolve() were to get called again, it would see
|blocking_dns_| and start doing the stuff for blocking DNS (blocking_dns_ trumps
abandoned).

For this reason, I post a task back to the message loop to set blocking_dns_
once the stack has unwound.

I have added a comment explaining this since it isn't obvious, and moved the
assignment to abandoned_ into the function.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:707: void
ProxyResolverV8Tracing::Job::RestartWithBlockingDns() {
On 2013/01/24 21:06:33, Matt Menke wrote:
> CheckIsOnWorkerThread();

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:717: bool* return_value) {
On 2013/01/24 21:06:33, Matt Menke wrote:
> CheckIsOnWorkerThread();

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:731: const net::AddressList& addresses) {
On 2013/01/24 21:06:33, Matt Menke wrote:
> "CheckIsOnOriginThread();" for documentation purposes.

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:801: alerts_and_errors_byte_cost_ +=
message.size() * 2;
On 2013/01/24 21:06:33, Matt Menke wrote:
> Should we toss in a sizeof(AlertOrError)?  Otherwise, could add a bunch of
> 0-length messages until Bad Things happen.

Excellent point. Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:827: bool is_alert, int line_number,
const string16& message) {
On 2013/01/24 21:06:33, Matt Menke wrote:
> Suggest a CheckIsOnWorkerThread() for documentation reasons.

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:847: NetLog::StringCallback("message",
&message));
On 2013/01/24 21:06:33, Matt Menke wrote:
> So we basically just silently consume PAC script alerts?  So we're supporting
> them only for debugging Chrome itself?

Correct. Actually popping up an alert() is silly, especially in a blocking
manner.

alert() is analogous to a printf() for authors trying to debug PAC scripts.

In the case of Chrome it is sent to about:net-internals. In the case of Firefox
it gets sent to the error console.

IE actually *does* pop open an alert dialog, which is pretty ridiculous if you
ask me.

The world needs fewer alerts, not more :)

Note also that this is not changing Chrome's current behavior; it already is
doing this.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:890: v8_resolver_.reset(new
ProxyResolverV8);
On 2013/01/24 21:06:33, Matt Menke wrote:
> Any compelling reason not to include this and thread_'s initialization in the
> initializer list?

It is so I can use base::ThreadRestrictions::ScopedAllowIO

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:32:
scoped_refptr<ProxyResolverScriptData> LoadData(const char* filename) {
On 2013/01/24 21:06:33, Matt Menke wrote:
> nit:  Suggest "LoadScript" or "LoadScriptData", since I think the "Script"
part
> of "ProxyResolverScriptData" is a clearer identifier than "Data".

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:62:
resolver->SetPacScript(LoadData(filename), callback.callback());
On 2013/01/24 21:06:33, Matt Menke wrote:
> nit:  Looks to me like this will fit on one line, and be exactly as long as
the
> function declaration.

Comes out to 81 chars now that I have renamed LoadData :)

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:79: CapturingBoundNetLog
req_log;
On 2013/01/24 21:06:33, Matt Menke wrote:
> nit:  Google C++ style guide is pretty anti-abbreviation.  Think request is
> short enough to just write it out.

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:82: ProxyResolverV8Tracing
resolver(&host_resolver, error_observer, &log);
On 2013/01/24 21:06:33, Matt Menke wrote:
> May want to put some of these in a text fixture.

I have gone back and forth on that. The problem I had is some tests use a
different host resolver dependency, and the code started to get weird to support
the different creation modes.

Although the current style duplicates the setup, I found it more readable.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:94: EXPECT_EQ(OK,
callback.WaitForResult());
On 2013/01/24 21:06:33, Matt Menke wrote:
> Could also put these in a function to make the tests a little simpler.

Unless you feel this is worthwhile, I plan to leave as-is. 

(I prefer to use a minimum of abstraction for tests to make it very explicit
what is being tested).

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:97:
proxy_info.proxy_server().ToURI());
On 2013/01/24 21:06:33, Matt Menke wrote:
> nit:  Can fit on a single line.

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:110:
TEST(ProxyResolverV8TracingTest, JavascriptError) {
On 2013/01/24 21:06:33, Matt Menke wrote:
> Worth testing alert and then an error, in the non-blocking case?  Could see a
> bug occurring where the alerts were dropped and just the error reported.

Done -- I modified this existing test rather than add a new one.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:152:
TEST(ProxyResolverV8TracingTest, TooManyAlerts) {
On 2013/01/24 21:06:33, Matt Menke wrote:
> Suggest alerts with 0-length bodies, just to be complete.

Done.

> Can you get multiple error messages?  If so, should there be an equivalent
error
> message test?

I don't *think* it is possible to get multiple error messages, however these are
delivered by V8 so I am not sure. I wrote it to support multiple ones being
received, however I don't know how to write a script that would exercise this.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:175:
proxy_info.proxy_server().ToURI());
On 2013/01/24 21:06:33, Matt Menke wrote:
> nit:  Can fit on one less line.

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:295: // The script itself only
does 2 DNS resolves, however it constructs the
On 2013/01/24 21:06:33, Matt Menke wrote:
> nit:  "2 DNS resolves per execution"

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:352:
proxy_info.proxy_server().ToURI());
On 2013/01/24 21:06:33, Matt Menke wrote:
> nit:  Fits on a single line.

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:418: CHECK(false);
On 2013/01/24 21:06:33, Matt Menke wrote:
> Can't we just do expect/assert here?

Call me paranoid, but I feel more comfortable with a crash. Cancellation is
tricky, and I want to be sure if this ever executes, be it on the wrong thread,
or after the test has already completed, then something will fail :)

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:425:
TEST(ProxyResolverV8TracingTest, CancelAll) {
On 2013/01/24 21:06:33, Matt Menke wrote:
> Suggest a more deterministic cancellation test, which should hopefully fail in
a
> more predictable manner on regressions, in addition to this test.
> 
> Just copy this test, but only use two requests.  Wait some brief period (10
> milliseconds?  50?) before cancelling the requests, and cancel them in reverse
> order.  Second request (first deleted) will be deleted while it's a queued
task.
>  First request will presumably be deleted while it's waiting for a DNS lookup
to
> check the cache.
> 
> Could actually use more requests, but two is all you need.

Let me get back on this one (want to respond to the other comments first)!

I kinda like the raciness of the current CancelAll(), since it may exercise
things I didn't necessarily think to test.

I actually got an interesting failure on TSAN and Valgrind from this test which
I wasn't anticipating :)

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:447: }
On 2013/01/24 21:06:33, Matt Menke wrote:
> Suggest a MessageLoop::RunAllPending(); at the end of these tests,
particularly
> if the net test fixture doesn't do it.  If the next test fixture does do it,
> still think for completeness, we should have a test that does it (Cancelled
and
> the resolver exists when the message gets to the IO thread, vs cancelled and
it
> doesn't).

I suspect that not running the message loop before deleting is a stronger test,
since more stuff may get deleted before it runs. But let me try.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:470: base::Bind(&CrashCallback),
&request1, BoundNetLog());
On 2013/01/24 21:06:33, Matt Menke wrote:
> EXPECT_EQ(ERR_IO_PENDING, rv);?

Done.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_unittest.cc:46: }
On 2013/01/24 21:06:33, Matt Menke wrote:
> Remove duplicated code block.

Oops!

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_unittest.cc:99: ProxyResolverV8WithMockBindings() :
ProxyResolverV8() {
On 2013/01/24 21:06:33, Matt Menke wrote:
> nit:  ProxyResolverV8() not needed.

Done. (leftover from earlier iteration).

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_unittest.cc:131: private:
On 2013/01/24 21:06:33, Matt Menke wrote:
> nit:  Add blank line before private

Done.

[mmenke, 2013-01-25 05:03:28]

Just some quick responses.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:300: Start(SET_PAC_SCRIPT, true
/*blocking*/, callback);
On 2013/01/25 03:02:01, eroman wrote:
> On 2013/01/24 21:06:33, Matt Menke wrote:
> > If the PAC script changes to a new PAC script, what about ongoing PAC
lookups?
> 
> > If they aren't cancelled elsewhere, they all may become blocking if the new
> > script doesn't match the old one, right?
> 
> There shouldn't be any outstanding PAC requests when calling SetPacScript
> (proxy_service.cc is supposed to cancel everything before calling it).
> 
> I'll add some comments to that effect for now. I could probably also add a
> counter for in-progress jobs to assert this fact, what do you think?

Thought that might be the case, but looks like the MultiThreadedProxyResolver
has some code to handle it.  I like the idea of having a counter for sanity
checking.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:321: // (a) The job has been posted to
the worker thread, but not yet started
On 2013/01/25 03:02:01, eroman wrote:
> On 2013/01/24 21:06:33, Matt Menke wrote:
> > nit:  started->[re]started
> 
> What i meant here was the first execution, hence "started". the "restart" case
> probably falls under (d).

I think it falls under a), actually.  We run a script, it does a host resolver
lookup.  Lookup isn't resolved synchronously, so the task is abandoned.  We get
a result, and post a tast to the worker thread to run again with the newly added
entry to the cache.  queuing this task is much more like the start case than the
pending DNS lookup case.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:324: // (c) The script has stopped and
there is a pending DNS request.
On 2013/01/25 03:02:01, eroman wrote:
> On 2013/01/24 21:06:33, Matt Menke wrote:
> > (e) The script has a pending NotifyCallerOnOriginLoop.
> > 
> > Worth testing this?  Can't be 100 percent sure we're in this state, but can
be
> > fairly sure.
> 
> This can probabilistic-ally happen in the CancelSome and CancelAll tests. I
> could add some sleeps to make it more likely, what do you think?

I was thinking this:  Start a lookup with a synchronous DNS resolver, wait x
milliseconds.  Run all pending tasks (Don't run until idle - believe there's a
function for this).  Then wait x milliseconds.  Then cancel the task.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:847: NetLog::StringCallback("message",
&message));
On 2013/01/25 03:02:01, eroman wrote:
> On 2013/01/24 21:06:33, Matt Menke wrote:
> > So we basically just silently consume PAC script alerts?  So we're
supporting
> > them only for debugging Chrome itself?
> 
> Correct. Actually popping up an alert() is silly, especially in a blocking
> manner.
> 
> alert() is analogous to a printf() for authors trying to debug PAC scripts.
> 
> In the case of Chrome it is sent to about:net-internals. In the case of
Firefox
> it gets sent to the error console.
> 
> IE actually *does* pop open an alert dialog, which is pretty ridiculous if you
> ask me.
> 
> The world needs fewer alerts, not more :)
> 
> Note also that this is not changing Chrome's current behavior; it already is
> doing this.

In an ideal world, we'd have some mechanism to surface them to developers. 
Given that we aren't surfacing them to developers, wonder if it's even worth
buffering them as we currently are.  Since you're the one who actually has to
debug the code, I defer to you in whether it's worth the effort.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:82: ProxyResolverV8Tracing
resolver(&host_resolver, error_observer, &log);
On 2013/01/25 03:02:01, eroman wrote:
> On 2013/01/24 21:06:33, Matt Menke wrote:
> > May want to put some of these in a text fixture.
> 
> I have gone back and forth on that. The problem I had is some tests use a
> different host resolver dependency, and the code started to get weird to
support
> the different creation modes.
> 
> Although the current style duplicates the setup, I found it more readable.

These tests aren't quite as boiler-platey as I initially thought.  I'm fine with
writing everything out.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:418: CHECK(false);
On 2013/01/25 03:02:01, eroman wrote:
> On 2013/01/24 21:06:33, Matt Menke wrote:
> > Can't we just do expect/assert here?
> 
> Call me paranoid, but I feel more comfortable with a crash. Cancellation is
> tricky, and I want to be sure if this ever executes, be it on the wrong
thread,
> or after the test has already completed, then something will fail :)

Amusingly, was some grumping over CHECK's in testing code in the FTP over proxy
CL.  I don't feel particularly strongly about this, either way.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:425:
TEST(ProxyResolverV8TracingTest, CancelAll) {
On 2013/01/25 03:02:01, eroman wrote:
> On 2013/01/24 21:06:33, Matt Menke wrote:
> > Suggest a more deterministic cancellation test, which should hopefully fail
in
> a
> > more predictable manner on regressions, in addition to this test.
> > 
> > Just copy this test, but only use two requests.  Wait some brief period (10
> > milliseconds?  50?) before cancelling the requests, and cancel them in
reverse
> > order.  Second request (first deleted) will be deleted while it's a queued
> task.
> >  First request will presumably be deleted while it's waiting for a DNS
lookup
> to
> > check the cache.
> > 
> > Could actually use more requests, but two is all you need.
> 
> Let me get back on this one (want to respond to the other comments first)!
> 
> I kinda like the raciness of the current CancelAll(), since it may exercise
> things I didn't necessarily think to test.
> 
> I actually got an interesting failure on TSAN and Valgrind from this test
which
> I wasn't anticipating :)

That's why I was suggesting another test, rather than removing this one.  :)

[mmenke, 2013-01-25 05:07:46]

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:447: }
On 2013/01/25 03:02:01, eroman wrote:
> On 2013/01/24 21:06:33, Matt Menke wrote:
> > Suggest a MessageLoop::RunAllPending(); at the end of these tests,
> particularly
> > if the net test fixture doesn't do it.  If the next test fixture does do it,
> > still think for completeness, we should have a test that does it (Cancelled
> and
> > the resolver exists when the message gets to the IO thread, vs cancelled and
> it
> > doesn't).
> 
> I suspect that not running the message loop before deleting is a stronger
test,
> since more stuff may get deleted before it runs. But let me try.

I'm not sure about this, either way.  If we have queued messages to a message
loop, and the loop is destroyed, aren't the messages just destroyed without ever
being run?  So seems like we would't really testing anything, unless the message
loop is run and the code that checks if the Job is cancelled runs.

[eroman, 2013-01-25 21:42:25]

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:300: Start(SET_PAC_SCRIPT, true
/*blocking*/, callback);
On 2013/01/25 05:03:28, Matt Menke wrote:
> On 2013/01/25 03:02:01, eroman wrote:
> > On 2013/01/24 21:06:33, Matt Menke wrote:
> > > If the PAC script changes to a new PAC script, what about ongoing PAC
> lookups?
> > 
> > > If they aren't cancelled elsewhere, they all may become blocking if the
new
> > > script doesn't match the old one, right?
> > 
> > There shouldn't be any outstanding PAC requests when calling SetPacScript
> > (proxy_service.cc is supposed to cancel everything before calling it).
> > 
> > I'll add some comments to that effect for now. I could probably also add a
> > counter for in-progress jobs to assert this fact, what do you think?
> 
> Thought that might be the case, but looks like the MultiThreadedProxyResolver
> has some code to handle it.  I like the idea of having a counter for sanity
> checking.

MutliThreadedProxyResolver asserts that there are no outstanding jobs. I have
added similar assertions to ProxyResolverV8Tracing.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:847: NetLog::StringCallback("message",
&message));
On 2013/01/25 05:03:28, Matt Menke wrote:
> On 2013/01/25 03:02:01, eroman wrote:
> > On 2013/01/24 21:06:33, Matt Menke wrote:
> > > So we basically just silently consume PAC script alerts?  So we're
> supporting
> > > them only for debugging Chrome itself?
> > 
> > Correct. Actually popping up an alert() is silly, especially in a blocking
> > manner.
> > 
> > alert() is analogous to a printf() for authors trying to debug PAC scripts.
> > 
> > In the case of Chrome it is sent to about:net-internals. In the case of
> Firefox
> > it gets sent to the error console.
> > 
> > IE actually *does* pop open an alert dialog, which is pretty ridiculous if
you
> > ask me.
> > 
> > The world needs fewer alerts, not more :)
> > 
> > Note also that this is not changing Chrome's current behavior; it already is
> > doing this.
> 
> In an ideal world, we'd have some mechanism to surface them to developers. 
> Given that we aren't surfacing them to developers, wonder if it's even worth
> buffering them as we currently are.  Since you're the one who actually has to
> debug the code, I defer to you in whether it's worth the effort.

Developers actually do use alerts(), so it is good to deliver them in the
expected order (would be weird to see all the intermediary alerts from
restarts).

Since buffering them is pretty straight forward I believe this is worth doing.

Not buffering errors could lead to incorrect results, since there may be error
observers, and a partial execution could give an error whereas once the DNS
dependency is resolved it may not.

[mmenke, 2013-01-25 21:47:42]

On 2013/01/25 21:42:25, eroman wrote:
> Developers actually do use alerts(), so it is good to deliver them in the
> expected order (would be weird to see all the intermediary alerts from
> restarts).
> 
> Since buffering them is pretty straight forward I believe this is worth doing.
> 
> Not buffering errors could lead to incorrect results, since there may be error
> observers, and a partial execution could give an error whereas once the DNS
> dependency is resolved it may not.

Where do developers see alerts?  Looks like we only dump them to the NetLog,
which isn't really intended for developer use.  DevTools doesn't seem to be
watching for them, either.

[eroman, 2013-01-25 21:48:44]

net-internals and the chrome log


On Fri, Jan 25, 2013 at 1:47 PM, <mmenke@chromium.org> wrote:

> On 2013/01/25 21:42:25, eroman wrote:
>
>> Developers actually do use alerts(), so it is good to deliver them in the
>> expected order (would be weird to see all the intermediary alerts from
>> restarts).
>>
>
>  Since buffering them is pretty straight forward I believe this is worth
>> doing.
>>
>
>  Not buffering errors could lead to incorrect results, since there may be
>> error
>> observers, and a partial execution could give an error whereas once the
>> DNS
>> dependency is resolved it may not.
>>
>
> Where do developers see alerts?  Looks like we only dump them to the
> NetLog,
> which isn't really intended for developer use.  DevTools doesn't seem to be
> watching for them, either.
>
>
https://codereview.chromium.**org/11885009/<https://codereview.chromium.org/1...
>

[mmenke, 2013-01-25 21:52:52]

Neither of which is really exposed to developers.  This is done deliberately,
since net-internals isn't internationalized, and the raw output doesn't really
have a spec.

Anyhow, like I said, I'd defer to you, but doesn't seem like it's really in a
place developers really have access to.

On 2013/01/25 21:48:44, eroman wrote:
> net-internals and the chrome log
> 
> 
> On Fri, Jan 25, 2013 at 1:47 PM, <mailto:mmenke@chromium.org> wrote:
> 
> > On 2013/01/25 21:42:25, eroman wrote:
> >
> >> Developers actually do use alerts(), so it is good to deliver them in the
> >> expected order (would be weird to see all the intermediary alerts from
> >> restarts).
> >>
> >
> >  Since buffering them is pretty straight forward I believe this is worth
> >> doing.
> >>
> >
> >  Not buffering errors could lead to incorrect results, since there may be
> >> error
> >> observers, and a partial execution could give an error whereas once the
> >> DNS
> >> dependency is resolved it may not.
> >>
> >
> > Where do developers see alerts?  Looks like we only dump them to the
> > NetLog,
> > which isn't really intended for developer use.  DevTools doesn't seem to be
> > watching for them, either.
> >
> >
>
https://codereview.chromium.**org/11885009/%3Chttps://codereview.chromium.org...>
> >

[eroman, 2013-01-25 22:00:31]

Sure, we can continue that discussion on another thread, since I think it is
separate from this changelist. The alerts() are there where developers (i.e
sysadmins) *can* see them, and this ability is being used. Having a better UI
for them would be great, however not urgent.

[eroman, 2013-01-25 22:11:39]

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.h (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.h:45: ProxyResolverErrorObserver*
error_observer,
On 2013/01/18 19:59:40, Matt Menke wrote:
> May want to take this as a scoped_ptr, just to make the ownership a little
more
> explicit.

I gave this a try but didn't much care for the resulting code, so will stick
with raw pointers.

Here is the sort of problems I ran into.

// ----- Original code
MockErrorObserver* error_observer = new MockErrorObserver;
ProxyResolverV8Tracing resolver(&host_resolver, error_observer, &log);

// ----- Failed translation 1
scoped_ptr<MockErrorObserver> error_observer(new MockErrorObserver);
ProxyResolverV8Tracing resolver(&host_resolver, error_observer.Pass(), &log);

<=== The above doesn't work because scoped_ptr<MockErrorObserver> is not
convertible to scoped_ptr<ProxyResolverErrorObserver> (the interface type).

// ----- Failed translation 2
scoped_ptr<MockErrorObserver> error_observer(new MockErrorObserver);
ProxyResolverV8Tracing resolver(&host_resolver, error_observer.PassAs(), &log);

<=== The above doesn't work either because it can't infer the type of the
template parameter (not exactly sure why not).

// ----- "successful" translation
scoped_ptr<MockErrorObserver> error_observer(new MockErrorObserver);
ProxyResolverV8Tracing resolver(&host_resolver,
error_observer.PassAs<ProxyResolverErrorObserver>(), &log);

<=== EEEEEEEEW. No thanks.

[eroman, 2013-01-26 01:13:55]

OK, I added some more cancellation tests, I think the coverage is in pretty good
shape.

There are 8 locations in the code which check for "cancelled_.IsSet()". All
except 1 of these are getting hit by the tests (I added some logging and ran the
tests to verify this).

The one that is not getting exercised is the check in Job::ExecuteBlocking(). I
haven't thought too hard about how to make a reliable test for this, can
probably live without one.

[mmenke, 2013-01-26 01:37:13]

On 2013/01/26 01:13:55, eroman wrote:
> OK, I added some more cancellation tests, I think the coverage is in pretty
good
> shape.
> 
> There are 8 locations in the code which check for "cancelled_.IsSet()". All
> except 1 of these are getting hit by the tests (I added some logging and ran
the
> tests to verify this).
> 
> The one that is not getting exercised is the check in Job::ExecuteBlocking().
I
> haven't thought too hard about how to make a reliable test for this, can
> probably live without one.

I'm in agreement that it's not needed.

[mmenke, 2013-01-29 20:19:08]

A bunch of comments because I'm picky.  Only holding off on signing off because
I want to understand if we're running cancelled tasks in the unit tests after
(Or before) destruction of the proxy resolver, or if they're just destroyed when
the message loop is.

Being destroyed without being run would be bad, I think.

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.h (right):

https://codereview.chromium.org/11885009/diff/42002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.h:45: ProxyResolverErrorObserver*
error_observer,
On 2013/01/25 22:11:39, eroman wrote:
> On 2013/01/18 19:59:40, Matt Menke wrote:
> > May want to take this as a scoped_ptr, just to make the ownership a little
> more
> > explicit.
> 
> I gave this a try but didn't much care for the resulting code, so will stick
> with raw pointers.
> 
> Here is the sort of problems I ran into.
> 
> // ----- Original code
> MockErrorObserver* error_observer = new MockErrorObserver;
> ProxyResolverV8Tracing resolver(&host_resolver, error_observer, &log);
> 
> // ----- Failed translation 1
> scoped_ptr<MockErrorObserver> error_observer(new MockErrorObserver);
> ProxyResolverV8Tracing resolver(&host_resolver, error_observer.Pass(), &log);
> 
> <=== The above doesn't work because scoped_ptr<MockErrorObserver> is not
> convertible to scoped_ptr<ProxyResolverErrorObserver> (the interface type).
> 
> // ----- Failed translation 2
> scoped_ptr<MockErrorObserver> error_observer(new MockErrorObserver);
> ProxyResolverV8Tracing resolver(&host_resolver, error_observer.PassAs(),
&log);
> 
> <=== The above doesn't work either because it can't infer the type of the
> template parameter (not exactly sure why not).
> 
> // ----- "successful" translation
> scoped_ptr<MockErrorObserver> error_observer(new MockErrorObserver);
> ProxyResolverV8Tracing resolver(&host_resolver,
> error_observer.PassAs<ProxyResolverErrorObserver>(), &log);
> 
> <=== EEEEEEEEW. No thanks.

Yea, that is more than a little horrifyingly hideous.

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:447: }
On 2013/01/25 05:07:46, Matt Menke wrote:
> On 2013/01/25 03:02:01, eroman wrote:
> > On 2013/01/24 21:06:33, Matt Menke wrote:
> > > Suggest a MessageLoop::RunAllPending(); at the end of these tests,
> > particularly
> > > if the net test fixture doesn't do it.  If the next test fixture does do
it,
> > > still think for completeness, we should have a test that does it
(Cancelled
> > and
> > > the resolver exists when the message gets to the IO thread, vs cancelled
and
> > it
> > > doesn't).
> > 
> > I suspect that not running the message loop before deleting is a stronger
> test,
> > since more stuff may get deleted before it runs. But let me try.
> 
> I'm not sure about this, either way.  If we have queued messages to a message
> loop, and the loop is destroyed, aren't the messages just destroyed without
ever
> being run?  So seems like we would't really testing anything, unless the
message
> loop is run and the code that checks if the Job is cancelled runs.

Did you check if those final posted jobs ever run or not?

https://codereview.chromium.org/11885009/diff/70002/net/data/proxy_resolver_v...
File net/data/proxy_resolver_v8_tracing_unittest/global_sideffects3.js (right):

https://codereview.chromium.org/11885009/diff/70002/net/data/proxy_resolver_v...
net/data/proxy_resolver_v8_tracing_unittest/global_sideffects3.js:3: function
FindProxyForURL(url, host) {
I like this one.  Fun for the whole family.  Assuming they're all geeks, of
course.  Does make you feel a bit sorry for the hapless proxy resolver, though.

https://codereview.chromium.org/11885009/diff/70002/net/net_unittests.isolate
File net/net_unittests.isolate (right):

https://codereview.chromium.org/11885009/diff/70002/net/net_unittests.isolate...
net/net_unittests.isolate:241:
'data/proxy_resolver_v8_tracing_unittest/too_many_empty_alerts.js',
Mac only?  What is this for?  iOS, with the android equivalent in
android/pylib/gtest/single_test_runner.py?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_error_observer.h (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_error_observer.h:14: // Interface for observing
JavaScript error messages from PAC scripts.
May want to mention that this can be called on any thread.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.cc:632: v8::Unlocker unlocker(args.GetIsolate());
If we're running v8 on only one thread...This is still needed to be compatible
with using v8 outside of net, I assume, like in --single-process?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:9: #include
"base/string_number_conversions.h"
Is this used anywhere?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:10: #include "base/string_util.h"
Not sure about this one, either.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:317: // (a) The job has been posted to
the worker thread, but not yet started
Suggest "but script execution has not yet started."  The function that posts the
job to the worker thread is called "Start", so the job has technically Started.

Also, I still think this much more closely resembles the "task to run the script
again, with a new entry in the local cache" case than the pending DNS request
case.  :)

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:319: // (c) The script is blocked on the
worker thread waiting for DNS
nit:  The above 3 lines should all end in periods.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:338: Release();
Hmm...One alternative that may be marginally better is to have the Job hold onto
a reference to itself, and release it on cancel/creation.

Not that much better, but I still prefer it to having an unowned reference in
the ether.  WDYT?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:351: DCHECK(!pending_dns_);
DCHECK(callback_.is_null());?  (Or is_empty, or empty, or whatever the function
is).

We clear it on cancellation or when we call it, so it should always end up
cleared, assuming we called on of those two functions.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:364: CheckIsOnOriginThread();
DCHECK(callback_.is_null());?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:370: CheckIsOnOriginThread();
DCHECK(!callback_.is_null());?  Mostly to make sure outstanding_callbacks_ is
only decremented once.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:373: callback_ = CompletionCallback();
callback_.Reset()?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:399: // Post a task to the origin thread,
to invoke the user's callback.
nit:  Function-level comments should be with function declaration.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:413: 
DCHECK(!callback_.is_empty());?  (or .empty() or .is_null(), or whatever it is)

Maybe also a DCHECK(!pending_dns_);

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:420:
DCHECK(parent_->set_pac_script_job_);
Maybe DCHECK_EQ(this, parent_->set_pac_script_job_.get())?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:452: 
Suggest DCHECKing that alerts_and_errors_ is empty either here or in
NotifyCaller (and clearing it in DispatchBufferedAlertsAndErrors(), if you do
the latter).

Could also DCHECK that abandoned_ is false.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:468: should_restart_with_blocking_dns_ =
false;
Can't you DCHECK on this at the start of this function?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:647: DCHECK(!pending_dns_);
This can go before cancelled_.IsSet(), right?  On cancellation, we cancel the
DNS lookup at the same time.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:663: pending_dns_ = NULL;
Is pending_dns_ even written to in this case?  If not, can just DCHECK on it.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:742: cache_value = std::string();
Hmm....  Are compilers smart enough to optimize this out?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:750: iter != addresses.end(); ++iter) {
No tests check this code.  Can't really do the MY_IP_ADDRESS_EX case, but we can
manage DNS_RESOLVE_EX with one / multiple addresses.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:750: iter != addresses.end(); ++iter) {
Hmm....No test uses multiple IPs, either.  Suggest testing that in the EX cases,
in particular, but may as well do the v4 case as well.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:750: iter != addresses.end(); ++iter) {
Should we check IPv6 as well?  Just make sure the output string is in the
expected format.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:755: cache_value = std::string();
Does this happen?  Doesn't look like it to me.  Could DCHECK on it instead.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:784: // convenience, to avoid defining
custom comparators.
nit:  Function-level comments should be at the declaration, not definition (And
in this case, there's already a copy of this comment above the definition).

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:814: // memory. (consider a script which
does megabytes worth of alerts()).
nit:  "Consider".  Also, think either the parentheses or the preceding period
aren't needed.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:924: DCHECK(!callback.is_null());
DCHECK(!set_pac_script_job_);?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.h (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.h:9: #include <vector>
Neither of these is used in thsi file.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.h:81: int num_outstanding_callbacks_;
DISALLOW_COPY_AND_ASSIGN, and add the correspoding header?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:52: std::string
GetParamsJson(const CapturingNetLog::CapturedEntry& entry) {
Worth making this a method of CapturingNetLog::CapturedEntry?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:153: "of null\n",
error_observer->GetOutput());
Should we be depending on the exact v8 error string in net/?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:199: EXPECT_EQ("foo:3",
proxy_info.proxy_server().ToURI());
Think it's worth a comment that iteration 1 does a DNS lookup, 2 runs into the
issue with two many alerts, and the third blocking iteration is the one that
returns the results.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:266: EXPECT_TRUE(
I can just imagine this test spewing 2000 lines of garbage.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:391: EXPECT_EQ(0u,
host_resolver.num_resolve_from_cache());
Think it's worth a note that this is only incremented on cache only resolutions.
 Alternatively, could just remove all the calls to num_resolve_from_cache in
this file.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:500:
TEST(ProxyResolverV8TracingTest, InfiniteDNSSequence) {
Suggest modifying this to make sure the first 20 dnsResolves were correctly
accessed from the cache.  Just count the non-NULL results in the loop and use it
as the port or something.

Not something that I think is terribly likely to regress without setting off
other failures, but I'm paranoid.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:765:
MessageLoop::current()->Quit();
Call me paranoid, but can we either ASSERT / CHECK that we're in
WaitUntilREquestIsReceived here, or not quit if we aren't?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:899:
TEST(ProxyResolverV8TracingTest, CancelWhileOutstandingBlockingDns) {
CancelSetPacWhile...?

All the others cancel a normal job.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_unittest.cc:127: scoped_ptr<MockJSBindings>
mock_js_bindings_;
nit:  Don't think this needs to be a scoped pointer.

[eroman, 2013-01-29 21:03:21]

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:447: }
On 2013/01/29 20:19:08, Matt Menke wrote:
> On 2013/01/25 05:07:46, Matt Menke wrote:
> > On 2013/01/25 03:02:01, eroman wrote:
> > > On 2013/01/24 21:06:33, Matt Menke wrote:
> > > > Suggest a MessageLoop::RunAllPending(); at the end of these tests,
> > > particularly
> > > > if the net test fixture doesn't do it.  If the next test fixture does do
> it,
> > > > still think for completeness, we should have a test that does it
> (Cancelled
> > > and
> > > > the resolver exists when the message gets to the IO thread, vs cancelled
> and
> > > it
> > > > doesn't).
> > > 
> > > I suspect that not running the message loop before deleting is a stronger
> > test,
> > > since more stuff may get deleted before it runs. But let me try.
> > 
> > I'm not sure about this, either way.  If we have queued messages to a
message
> > loop, and the loop is destroyed, aren't the messages just destroyed without
> ever
> > being run?  So seems like we would't really testing anything, unless the
> message
> > loop is run and the code that checks if the Job is cancelled runs.
> 
> Did you check if those final posted jobs ever run or not?

There are two sets of tasks to consider:
 (a) Those posted to the worker thread
 (b) Those posted to the origin thread

Those posted to the worker thread are always run, because the worker thread is
owned by ProxyResolverV8Tracing. So during ~ProxyResolverV8Tracing the thread is
stopped, and any outstanding tasks are run.

As far as those posted on the origin thread, there are only 2 tasks:
NotifyCallerOnOriginLoop and DoDnsOperation. The unittests cover running both of
these tasks after the request has been cancelled.

For the most part, these two tasks will be run after the ProxyResolverV8Tracing
has been deleted. They get run as a sideffect of starting the next test, when it
runs the message loop.

I like the fact that they run after deletion of the parent. However I can add in
some run message loop lines in the various tests so this get exercised before
returning from the test.

[mmenke, 2013-01-29 21:10:46]

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:447: }
On 2013/01/29 21:03:21, eroman wrote:
> On 2013/01/29 20:19:08, Matt Menke wrote:
> > On 2013/01/25 05:07:46, Matt Menke wrote:
> > > On 2013/01/25 03:02:01, eroman wrote:
> > > > On 2013/01/24 21:06:33, Matt Menke wrote:
> > > > > Suggest a MessageLoop::RunAllPending(); at the end of these tests,
> > > > particularly
> > > > > if the net test fixture doesn't do it.  If the next test fixture does
do
> > it,
> > > > > still think for completeness, we should have a test that does it
> > (Cancelled
> > > > and
> > > > > the resolver exists when the message gets to the IO thread, vs
cancelled
> > and
> > > > it
> > > > > doesn't).
> > > > 
> > > > I suspect that not running the message loop before deleting is a
stronger
> > > test,
> > > > since more stuff may get deleted before it runs. But let me try.
> > > 
> > > I'm not sure about this, either way.  If we have queued messages to a
> message
> > > loop, and the loop is destroyed, aren't the messages just destroyed
without
> > ever
> > > being run?  So seems like we would't really testing anything, unless the
> > message
> > > loop is run and the code that checks if the Job is cancelled runs.
> > 
> > Did you check if those final posted jobs ever run or not?
> 
> There are two sets of tasks to consider:
>  (a) Those posted to the worker thread
>  (b) Those posted to the origin thread
> 
> Those posted to the worker thread are always run, because the worker thread is
> owned by ProxyResolverV8Tracing. So during ~ProxyResolverV8Tracing the thread
is
> stopped, and any outstanding tasks are run.
> 
> As far as those posted on the origin thread, there are only 2 tasks:
> NotifyCallerOnOriginLoop and DoDnsOperation. The unittests cover running both
of
> these tasks after the request has been cancelled.
> 
> For the most part, these two tasks will be run after the
ProxyResolverV8Tracing
> has been deleted. They get run as a sideffect of starting the next test, when
it
> runs the message loop.
> 
> I like the fact that they run after deletion of the parent. However I can add
in
> some run message loop lines in the various tests so this get exercised before
> returning from the test.

Yea, I meant the ones posted to the origin message loop.  As long as they are in
fact run, I'm happy.  However, running them as a side effect of the next test I
really do not like.  Getting failures for the last test when running the next
one seems like it's just a bad idea.  My suggestion is to add a test fixture and
call MessageLoop::RunAllPending() on destruction.  Don't actually need to do
anything else, other than switch the TESTs to TEST_Fs.

I'd still like to take a couple minutes to skim over your responses to my
comments after your next upload, but I expect to sign off on it without (much)
comment.

[eroman, 2013-01-29 21:13:34]

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/56001/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:447: }
On 2013/01/29 21:10:47, Matt Menke wrote:
> On 2013/01/29 21:03:21, eroman wrote:
> > On 2013/01/29 20:19:08, Matt Menke wrote:
> > > On 2013/01/25 05:07:46, Matt Menke wrote:
> > > > On 2013/01/25 03:02:01, eroman wrote:
> > > > > On 2013/01/24 21:06:33, Matt Menke wrote:
> > > > > > Suggest a MessageLoop::RunAllPending(); at the end of these tests,
> > > > > particularly
> > > > > > if the net test fixture doesn't do it.  If the next test fixture
does
> do
> > > it,
> > > > > > still think for completeness, we should have a test that does it
> > > (Cancelled
> > > > > and
> > > > > > the resolver exists when the message gets to the IO thread, vs
> cancelled
> > > and
> > > > > it
> > > > > > doesn't).
> > > > > 
> > > > > I suspect that not running the message loop before deleting is a
> stronger
> > > > test,
> > > > > since more stuff may get deleted before it runs. But let me try.
> > > > 
> > > > I'm not sure about this, either way.  If we have queued messages to a
> > message
> > > > loop, and the loop is destroyed, aren't the messages just destroyed
> without
> > > ever
> > > > being run?  So seems like we would't really testing anything, unless the
> > > message
> > > > loop is run and the code that checks if the Job is cancelled runs.
> > > 
> > > Did you check if those final posted jobs ever run or not?
> > 
> > There are two sets of tasks to consider:
> >  (a) Those posted to the worker thread
> >  (b) Those posted to the origin thread
> > 
> > Those posted to the worker thread are always run, because the worker thread
is
> > owned by ProxyResolverV8Tracing. So during ~ProxyResolverV8Tracing the
thread
> is
> > stopped, and any outstanding tasks are run.
> > 
> > As far as those posted on the origin thread, there are only 2 tasks:
> > NotifyCallerOnOriginLoop and DoDnsOperation. The unittests cover running
both
> of
> > these tasks after the request has been cancelled.
> > 
> > For the most part, these two tasks will be run after the
> ProxyResolverV8Tracing
> > has been deleted. They get run as a sideffect of starting the next test,
when
> it
> > runs the message loop.
> > 
> > I like the fact that they run after deletion of the parent. However I can
add
> in
> > some run message loop lines in the various tests so this get exercised
before
> > returning from the test.
> 
> Yea, I meant the ones posted to the origin message loop.  As long as they are
in
> fact run, I'm happy.  However, running them as a side effect of the next test
I
> really do not like.  Getting failures for the last test when running the next
> one seems like it's just a bad idea.  My suggestion is to add a test fixture
and
> call MessageLoop::RunAllPending() on destruction.  Don't actually need to do
> anything else, other than switch the TESTs to TEST_Fs.
> 
> I'd still like to take a couple minutes to skim over your responses to my
> comments after your next upload, but I expect to sign off on it without (much)
> comment.

Yep, that is exactly what I was planning too :) Will test and upload that
shortly.

[eroman, 2013-01-29 22:51:23]

https://codereview.chromium.org/11885009/diff/70002/net/net_unittests.isolate
File net/net_unittests.isolate (right):

https://codereview.chromium.org/11885009/diff/70002/net/net_unittests.isolate...
net/net_unittests.isolate:241:
'data/proxy_resolver_v8_tracing_unittest/too_many_empty_alerts.js',
On 2013/01/29 20:19:08, Matt Menke wrote:
> Mac only?  What is this for?  iOS, with the android equivalent in
> android/pylib/gtest/single_test_runner.py?

I just copied the existing pattern. I assumed these were for all platforms not
just Mac, but honestly didn't spend too much time parsing the gyp.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_error_observer.h (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_error_observer.h:14: // Interface for observing
JavaScript error messages from PAC scripts.
On 2013/01/29 20:19:08, Matt Menke wrote:
> May want to mention that this can be called on any thread.

Done.

It is ambiguous what thread this will be called on, since this interface isn't
tied to any particular implementation (for instance if used from the
multithreaded proxy resolver it could be any of the workers).

I have added a generic comment.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.cc:632: v8::Unlocker unlocker(args.GetIsolate());
On 2013/01/29 20:19:08, Matt Menke wrote:
> If we're running v8 on only one thread...This is still needed to be compatible
> with using v8 outside of net, I assume, like in --single-process?

Note that we disable v8-proxy resolver in --single-process mode

Both WebKit and this code are using the "default isolate". Although this code is
careful to lock entries into V8, the webkit bindings DO NOT, so it is not safe
to run them together.

I would like to use a self-contained isolate for ProxyResolverV8, but that isn't
possible yet -- It would be too expensive to create a new isolate, since V8 is
going to create a default one anyway (see http://crbug.com/171457).

Once that can be addressed however, we can get rid of all the scary comments
explaining why it is not safe to use in single-process mode, and simply enable
it everywhere.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:9: #include
"base/string_number_conversions.h"
On 2013/01/29 20:19:08, Matt Menke wrote:
> Is this used anywhere?

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:10: #include "base/string_util.h"
On 2013/01/29 20:19:08, Matt Menke wrote:
> Not sure about this one, either.

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:317: // (a) The job has been posted to
the worker thread, but not yet started
On 2013/01/29 20:19:08, Matt Menke wrote:
> Suggest "but script execution has not yet started."  The function that posts
the
> job to the worker thread is called "Start", so the job has technically
Started.

Done.

> 
> Also, I still think this much more closely resembles the "task to run the
script
> again, with a new entry in the local cache" case than the pending DNS request
> case.  :)

Ok fine, I added another case :) Also reworded them a bit.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:338: Release();
On 2013/01/29 20:19:08, Matt Menke wrote:
> Hmm...One alternative that may be marginally better is to have the Job hold
onto
> a reference to itself, and release it on cancel/creation.
> 
> Not that much better, but I still prefer it to having an unowned reference in
> the ether.  WDYT?

Can you explain what that would look like? (Since AFAICT this is a reference
owned by the job, being released on cancellation/completion).

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:351: DCHECK(!pending_dns_);
On 2013/01/29 20:19:08, Matt Menke wrote:
> DCHECK(callback_.is_null());?  (Or is_empty, or empty, or whatever the
function
> is).
> 
> We clear it on cancellation or when we call it, so it should always end up
> cleared, assuming we called on of those two functions.

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:364: CheckIsOnOriginThread();
On 2013/01/29 20:19:08, Matt Menke wrote:
> DCHECK(callback_.is_null());?

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:370: CheckIsOnOriginThread();
On 2013/01/29 20:19:08, Matt Menke wrote:
> DCHECK(!callback_.is_null());?  Mostly to make sure outstanding_callbacks_ is
> only decremented once.

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:373: callback_ = CompletionCallback();
On 2013/01/29 20:19:08, Matt Menke wrote:
> callback_.Reset()?

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:399: // Post a task to the origin thread,
to invoke the user's callback.
On 2013/01/29 20:19:08, Matt Menke wrote:
> nit:  Function-level comments should be with function declaration.

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:413: 
On 2013/01/29 20:19:08, Matt Menke wrote:
> DCHECK(!callback_.is_empty());?  (or .empty() or .is_null(), or whatever it
is)
> 
> Maybe also a DCHECK(!pending_dns_);

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:420:
DCHECK(parent_->set_pac_script_job_);
On 2013/01/29 20:19:08, Matt Menke wrote:
> Maybe DCHECK_EQ(this, parent_->set_pac_script_job_.get())?

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:452: 
On 2013/01/29 20:19:08, Matt Menke wrote:
> Suggest DCHECKing that alerts_and_errors_ is empty either here or in
> NotifyCaller (and clearing it in DispatchBufferedAlertsAndErrors(), if you do
> the latter).
> 
> Could also DCHECK that abandoned_ is false.

I don't reset that state on fallback so I can't currently assert it.

Do you want me to change that?

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:468: should_restart_with_blocking_dns_ =
false;
On 2013/01/29 20:19:08, Matt Menke wrote:
> Can't you DCHECK on this at the start of this function?

This is the only place I in initialize it. I favored doing initialization of
non-blocking only state only when going down the non-blocking path.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:647: DCHECK(!pending_dns_);
On 2013/01/29 20:19:08, Matt Menke wrote:
> This can go before cancelled_.IsSet(), right?  On cancellation, we cancel the
> DNS lookup at the same time.

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:647: DCHECK(!pending_dns_);
On 2013/01/29 20:19:08, Matt Menke wrote:
> This can go before cancelled_.IsSet(), right?  On cancellation, we cancel the
> DNS lookup at the same time.

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:663: pending_dns_ = NULL;
On 2013/01/29 20:19:08, Matt Menke wrote:
> Is pending_dns_ even written to in this case?  If not, can just DCHECK on it.

I prefer to explicitly NULL this just in case the host resolver decides to write
to it.

It would be an easy bug for the HostResolver to end up setting the handle
temporarily, and then deciding to complete the request synchronously. This is
subtle enough that I don't want to rely on it never getting set.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:742: cache_value = std::string();
On 2013/01/29 20:19:08, Matt Menke wrote:
> Hmm....  Are compilers smart enough to optimize this out?

I doubt it.

I wrote it this way, because I wanted to be explicit that the cache value is
empty string in failure case. I don't believe this will have any real impact on
performance, but I can change it if you like.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:750: iter != addresses.end(); ++iter) {
On 2013/01/29 20:19:08, Matt Menke wrote:
> No tests check this code.  Can't really do the MY_IP_ADDRESS_EX case, but we
can
> manage DNS_RESOLVE_EX with one / multiple addresses.

I was lazy on these because it requires some additional mocking of the host
resolver (to specify separate rules for ipv4-only vs unspec).

I will get back to you once I have the tests in place.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:755: cache_value = std::string();
On 2013/01/29 20:19:08, Matt Menke wrote:
> Does this happen?  Doesn't look like it to me.  Could DCHECK on it instead.

Done.

This was a carry over from when the string conversion was using the OS libraries
(which could fail).

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:784: // convenience, to avoid defining
custom comparators.
On 2013/01/29 20:19:08, Matt Menke wrote:
> nit:  Function-level comments should be at the declaration, not definition
(And
> in this case, there's already a copy of this comment above the definition).

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:814: // memory. (consider a script which
does megabytes worth of alerts()).
On 2013/01/29 20:19:08, Matt Menke wrote:
> nit:  "Consider".  Also, think either the parentheses or the preceding period
> aren't needed.

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:924: DCHECK(!callback.is_null());
On 2013/01/29 20:19:08, Matt Menke wrote:
> DCHECK(!set_pac_script_job_);?

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.h (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.h:9: #include <vector>
On 2013/01/29 20:19:08, Matt Menke wrote:
> Neither of these is used in thsi file.

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.h:81: int num_outstanding_callbacks_;
On 2013/01/29 20:19:08, Matt Menke wrote:
> DISALLOW_COPY_AND_ASSIGN, and add the correspoding header?

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:52: std::string
GetParamsJson(const CapturingNetLog::CapturedEntry& entry) {
On 2013/01/29 20:19:08, Matt Menke wrote:
> Worth making this a method of CapturingNetLog::CapturedEntry?

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:153: "of null\n",
error_observer->GetOutput());
On 2013/01/29 20:19:08, Matt Menke wrote:
> Should we be depending on the exact v8 error string in net/?

If it turns into a maintenance problem I can generalize the test, however I
don't anticipate these changing any time soon.

(The ProxyResolverV8 unittests already hardcode some error strings, and those
have not had to be changed in years.)

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:199: EXPECT_EQ("foo:3",
proxy_info.proxy_server().ToURI());
On 2013/01/29 20:19:08, Matt Menke wrote:
> Think it's worth a comment that iteration 1 does a DNS lookup, 2 runs into the
> issue with two many alerts, and the third blocking iteration is the one that
> returns the results.

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:266: EXPECT_TRUE(
On 2013/01/29 20:19:08, Matt Menke wrote:
> I can just imagine this test spewing 2000 lines of garbage.

Lulz. Changed to ASSERT_TRUE() instead

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:391: EXPECT_EQ(0u,
host_resolver.num_resolve_from_cache());
On 2013/01/29 20:19:08, Matt Menke wrote:
> Think it's worth a note that this is only incremented on cache only
resolutions.
>  Alternatively, could just remove all the calls to num_resolve_from_cache in
> this file.

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:500:
TEST(ProxyResolverV8TracingTest, InfiniteDNSSequence) {
On 2013/01/29 20:19:08, Matt Menke wrote:
> Suggest modifying this to make sure the first 20 dnsResolves were correctly
> accessed from the cache.  Just count the non-NULL results in the loop and use
it
> as the port or something.
> 
> Not something that I think is terribly likely to regress without setting off
> other failures, but I'm paranoid.

Done. I admire your paranoia.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:765:
MessageLoop::current()->Quit();
On 2013/01/29 20:19:08, Matt Menke wrote:
> Call me paranoid, but can we either ASSERT / CHECK that we're in
> WaitUntilREquestIsReceived here, or not quit if we aren't?

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:899:
TEST(ProxyResolverV8TracingTest, CancelWhileOutstandingBlockingDns) {
On 2013/01/29 20:19:08, Matt Menke wrote:
> CancelSetPacWhile...?
> 
> All the others cancel a normal job.

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_unittest.cc:127: scoped_ptr<MockJSBindings>
mock_js_bindings_;
On 2013/01/29 20:19:08, Matt Menke wrote:
> nit:  Don't think this needs to be a scoped pointer.

Done.

[mmenke, 2013-01-29 23:10:31]

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:338: Release();
On 2013/01/29 22:51:23, eroman wrote:
> On 2013/01/29 20:19:08, Matt Menke wrote:
> > Hmm...One alternative that may be marginally better is to have the Job hold
> onto
> > a reference to itself, and release it on cancel/creation.
> > 
> > Not that much better, but I still prefer it to having an unowned reference
in
> > the ether.  WDYT?
> 
> Can you explain what that would look like? (Since AFAICT this is a reference
> owned by the job, being released on cancellation/completion).

Replace the two Release() calls with:

owned_self_reference_ = NULL;

And the AddRef call with:

owned_self_reference_ = this;

Basically, I'm happier with an explicitly owned reference, rather than a
dangling reference in the ether without explicit ownership.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:452: 
On 2013/01/29 22:51:23, eroman wrote:
> On 2013/01/29 20:19:08, Matt Menke wrote:
> > Suggest DCHECKing that alerts_and_errors_ is empty either here or in
> > NotifyCaller (and clearing it in DispatchBufferedAlertsAndErrors(), if you
do
> > the latter).
> > 
> > Could also DCHECK that abandoned_ is false.
> 
> I don't reset that state on fallback so I can't currently assert it.
> 
> Do you want me to change that?

Didn't realize that.  I don't feel particularly strongly about it.  We have unit
tests for it, anyways.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:468: should_restart_with_blocking_dns_ =
false;
On 2013/01/29 22:51:23, eroman wrote:
> On 2013/01/29 20:19:08, Matt Menke wrote:
> > Can't you DCHECK on this at the start of this function?
> 
> This is the only place I in initialize it. I favored doing initialization of
> non-blocking only state only when going down the non-blocking path.

Ah, just assumed it was initialized on construction.  That's fine.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:663: pending_dns_ = NULL;
On 2013/01/29 22:51:23, eroman wrote:
> On 2013/01/29 20:19:08, Matt Menke wrote:
> > Is pending_dns_ even written to in this case?  If not, can just DCHECK on
it.
> 
> I prefer to explicitly NULL this just in case the host resolver decides to
write
> to it.
> 
> It would be an easy bug for the HostResolver to end up setting the handle
> temporarily, and then deciding to complete the request synchronously. This is
> subtle enough that I don't want to rely on it never getting set.

OK

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:742: cache_value = std::string();
On 2013/01/29 22:51:23, eroman wrote:
> On 2013/01/29 20:19:08, Matt Menke wrote:
> > Hmm....  Are compilers smart enough to optimize this out?
> 
> I doubt it.
> 
> I wrote it this way, because I wanted to be explicit that the cache value is
> empty string in failure case. I don't believe this will have any real impact
on
> performance, but I can change it if you like.

That's fine.  Of course, the only concern with this (Like 99% of Chrome code) is
binary size.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:153: "of null\n",
error_observer->GetOutput());
On 2013/01/29 22:51:23, eroman wrote:
> On 2013/01/29 20:19:08, Matt Menke wrote:
> > Should we be depending on the exact v8 error string in net/?
> 
> If it turns into a maintenance problem I can generalize the test, however I
> don't anticipate these changing any time soon.
> 
> (The ProxyResolverV8 unittests already hardcode some error strings, and those
> have not had to be changed in years.)

SGTM.

https://codereview.chromium.org/11885009/diff/69006/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/11885009/diff/69006/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.cc:700: if (!context_.get())
Since you're removing these CApturingNetLog, may as well remove this ".get()" as
well.  There's also one in SetPacScript further down.

[eroman, 2013-01-30 01:25:25]

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:338: Release();
On 2013/01/29 23:10:31, Matt Menke wrote:
> On 2013/01/29 22:51:23, eroman wrote:
> > On 2013/01/29 20:19:08, Matt Menke wrote:
> > > Hmm...One alternative that may be marginally better is to have the Job
hold
> > onto
> > > a reference to itself, and release it on cancel/creation.
> > > 
> > > Not that much better, but I still prefer it to having an unowned reference
> in
> > > the ether.  WDYT?
> > 
> > Can you explain what that would look like? (Since AFAICT this is a reference
> > owned by the job, being released on cancellation/completion).
> 
> Replace the two Release() calls with:
> 
> owned_self_reference_ = NULL;
> 
> And the AddRef call with:
> 
> owned_self_reference_ = this;
> 
> Basically, I'm happier with an explicitly owned reference, rather than a
> dangling reference in the ether without explicit ownership.

Done.

https://codereview.chromium.org/11885009/diff/70002/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8_tracing.cc:750: iter != addresses.end(); ++iter) {
On 2013/01/29 22:51:23, eroman wrote:
> On 2013/01/29 20:19:08, Matt Menke wrote:
> > No tests check this code.  Can't really do the MY_IP_ADDRESS_EX case, but we
> can
> > manage DNS_RESOLVE_EX with one / multiple addresses.
> 
> I was lazy on these because it requires some additional mocking of the host
> resolver (to specify separate rules for ipv4-only vs unspec).
> 
> I will get back to you once I have the tests in place.

Done -- I have added tests for both dnsResolveEx() and myIpAddressEx(), by
modifying "dns.js"

https://codereview.chromium.org/11885009/diff/69006/net/proxy/proxy_resolver_...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/11885009/diff/69006/net/proxy/proxy_resolver_...
net/proxy/proxy_resolver_v8.cc:700: if (!context_.get())
On 2013/01/29 23:10:31, Matt Menke wrote:
> Since you're removing these CApturingNetLog, may as well remove this ".get()"
as
> well.  There's also one in SetPacScript further down.

Done.

[mmenke, 2013-01-30 01:34:20]

LGTM as-is.

Worth checking the multiple IP case with *Ex?  MockHostResolver supports
comma-separated address lists, so should be easy to add.

[mmenke, 2013-01-30 01:36:39]

On 2013/01/30 01:34:20, Matt Menke wrote:
> LGTM as-is.
> 
> Worth checking the multiple IP case with *Ex?  MockHostResolver supports
> comma-separated address lists, so should be easy to add.

Oh, and do we have HostResolver returns network error and/or no host names
cases?

[eroman, 2013-01-30 01:38:56]

I have tests for where dnsResolveEx() returns multiple values. This is where I
mock out 2 values:

host_resolver.rules()->AddIPLiteralRule("host1", "::1,192.168.1.1", "");

I have tests where dnsResolve() returns a failure, but not dnsResolveEx(). I
will add a failure case for dnsResolveEx() too

[mmenke, 2013-01-30 01:42:37]

On 2013/01/30 01:38:56, eroman wrote:
> I have tests for where dnsResolveEx() returns multiple values. This is where I
> mock out 2 values:
> 
> host_resolver.rules()->AddIPLiteralRule("host1", "::1,192.168.1.1", "");

Oops.  Misread that as "::192.168.1.1".

> I have tests where dnsResolve() returns a failure, but not dnsResolveEx(). I
> will add a failure case for dnsResolveEx() too

Thanks!

[eroman, 2013-01-30 01:46:11]

Ok done.

I will also re-run the micro benchmark I referenced in the CL description, and
update the numbers in case it has changed (since we have changed a number of the
implementation details, including removing the MD5 checksumming).

[mmenke, 2013-01-30 01:50:19]

On 2013/01/30 01:46:11, eroman wrote:
> Ok done.
> 
> I will also re-run the micro benchmark I referenced in the CL description, and
> update the numbers in case it has changed (since we have changed a number of
the
> implementation details, including removing the MD5 checksumming).

Great!  Do we have any UMA on this?  I'll be curious to see how much it speeds
thing up in practice for those unfortunate enough to be using PACs.

[mmenke, 2013-01-30 01:51:31]

On 2013/01/30 01:50:19, Matt Menke wrote:
> On 2013/01/30 01:46:11, eroman wrote:
> > Ok done.
> > 
> > I will also re-run the micro benchmark I referenced in the CL description,
and
> > update the numbers in case it has changed (since we have changed a number of
> the
> > implementation details, including removing the MD5 checksumming).
> 
> Great!  Do we have any UMA on this?  I'll be curious to see how much it speeds
> thing up in practice for those unfortunate enough to be using PACs.

Note:  Not suggesting you add it if we don't, I just like to see numbers and
think "Hey, we're actually doing something useful!"

[eroman, 2013-01-30 02:16:19]

No, unfortunately there isn't a metric from before to compare against

[commit-bot: I haz the power, 2013-01-30 03:07:18]

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

[commit-bot: I haz the power, 2013-01-30 03:07:38]

Presubmit check for 11885009-76157 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  net/data/proxy_resolver_v8_tracing_unittest/simple.js \
  net/data/proxy_resolver_v8_tracing_unittest/dns.js \
  net/data/proxy_resolver_v8_tracing_unittest/dns_during_init.js \
  net/data/proxy_resolver_v8_tracing_unittest/error.js \
  net/data/proxy_resolver_v8_tracing_unittest/global_sideffects1.js \
  net/data/proxy_resolver_v8_tracing_unittest/global_sideffects2.js \
  net/data/proxy_resolver_v8_tracing_unittest/global_sideffects3.js \
  net/data/proxy_resolver_v8_tracing_unittest/global_sideffects4.js \
  net/data/proxy_resolver_v8_tracing_unittest/simple_dns.js \
  net/data/proxy_resolver_v8_tracing_unittest/too_many_alerts.js \
  net/data/proxy_resolver_v8_tracing_unittest/too_many_empty_alerts.js

** Presubmit ERRORS **
Banned functions were used.
    net/proxy/proxy_resolver_v8_tracing.cc:919:
      New code should not use ScopedAllowIO. Post a task to the blocking
      pool or the FILE thread instead.

Presubmit checks took 6.0s to calculate.

[eroman, 2013-01-30 20:28:11]

I will be ignoring those presubmit warnings.

The reason I think that is OK to do:

 * The copyright warnings are for test data files -- I actually don't want all
of those starting with comment lines, since it homogenizes the inputs to the
test too much. I am not strongly opposed to adding them however.

 * The scoped AllowScopedIO warning is legitimate. However this CL is not really
adding a new bypass, it is simply moving an existing one (before it would have
been hitting the one in ~MultiThreadedProxyResolver). Since this is not an
addition, I think it is reasonable to defer the fix to http://crbug.com/69710.
This CL is already large enough that I don't want to address the additional
shutdown, cancellation, and leak concerns with trying to move the worker to a
non-joinable thread.

I will likely be committing this manually later today.

[mmenke, 2013-01-30 20:43:30]

On 2013/01/30 20:28:11, eroman wrote:
> I will be ignoring those presubmit warnings.
> 
> The reason I think that is OK to do:
> 
>  * The copyright warnings are for test data files -- I actually don't want all
> of those starting with comment lines, since it homogenizes the inputs to the
> test too much. I am not strongly opposed to adding them however.
> 
>  * The scoped AllowScopedIO warning is legitimate. However this CL is not
really
> adding a new bypass, it is simply moving an existing one (before it would have
> been hitting the one in ~MultiThreadedProxyResolver). Since this is not an
> addition, I think it is reasonable to defer the fix to http://crbug.com/69710.
> This CL is already large enough that I don't want to address the additional
> shutdown, cancellation, and leak concerns with trying to move the worker to a
> non-joinable thread.
> 
> I will likely be committing this manually later today.

I agree about the scoped IO.  The copyright warnings I have no strong feelings
about.