Fix a memory error in ProxyResolverV8Tracing.

Fix a memory error in ProxyResolverV8Tracing.

The problem could happen when executing:
  ProxyResolverV8TracingTest.CancelWhileBlockedInNonBlockingDns

The issue was the Job was not expecting it could get cancelled within the call to the HostResolver. It now handles this case, and I have added a Sleep() so the timing conditions for this problem manifest consistently.

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

[eroman, 2013-01-31 20:32:07]

Please also check the "Commit" box if you approve this. Thanks!

[mmenke, 2013-01-31 21:11:22]

https://codereview.chromium.org/12088090/diff/1/net/proxy/proxy_resolver_v8_t...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/12088090/diff/1/net/proxy/proxy_resolver_v8_t...
net/proxy/proxy_resolver_v8_tracing.cc:683: pending_dns_ = dns_request;
This isn't correct.  If we're doing a blocking call and complete synchronously,
we won't set pending_dns_ here, and then we'll run into the DCHECK in
OnDnsOperationComplete.

https://codereview.chromium.org/12088090/diff/1/net/proxy/proxy_resolver_v8_t...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/12088090/diff/1/net/proxy/proxy_resolver_v8_t...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:869:
TEST_F(ProxyResolverV8TracingTest, CancelWhileBlockedInNonBlockingDns) {
Hmm...The purpose of this test was to cancel before the non blocking DNS hears
about the cache miss.  Too bad there's no
"MessageLoop::PostTaskToRunBeforeTheCurrentlyExecutingTaskWasPosted()".

We could just wait a couple milliseconds after posting the instead.  Not
perfect, but it avoids getting us into this unexpected situation.

[eroman, 2013-01-31 21:29:49]

https://codereview.chromium.org/12088090/diff/1/net/proxy/proxy_resolver_v8_t...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/12088090/diff/1/net/proxy/proxy_resolver_v8_t...
net/proxy/proxy_resolver_v8_tracing.cc:683: pending_dns_ = dns_request;
On 2013/01/31 21:11:23, Matt Menke wrote:
> This isn't correct.  If we're doing a blocking call and complete
synchronously,
> we won't set pending_dns_ here, and then we'll run into the DCHECK in
> OnDnsOperationComplete.

Actually it is the DCHECK in OnDnsOperationComplete() which is wrong. If the
request completed synchronously, then pending_dns_ should be NULL. Adding the
assignment shouldn't change that catch.

Good catch! This is a left-over from one of my prior refactorings.

For now I have removed the DCHECK. I will follow-up with a test to exercise this
situation, and see if I can't clarify the code in that as well.

https://codereview.chromium.org/12088090/diff/1/net/proxy/proxy_resolver_v8_t...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/12088090/diff/1/net/proxy/proxy_resolver_v8_t...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:869:
TEST_F(ProxyResolverV8TracingTest, CancelWhileBlockedInNonBlockingDns) {
On 2013/01/31 21:11:23, Matt Menke wrote:
> Hmm...The purpose of this test was to cancel before the non blocking DNS hears
> about the cache miss.  Too bad there's no
> "MessageLoop::PostTaskToRunBeforeTheCurrentlyExecutingTaskWasPosted()".
> 
> We could just wait a couple milliseconds after posting the instead.  Not
> perfect, but it avoids getting us into this unexpected situation.

I think the test works as is (it is testing two important situations now). Are
you suggesting changing it or adding a new one?

[mmenke, 2013-01-31 21:33:53]

On 2013/01/31 21:29:49, eroman wrote:
>
https://codereview.chromium.org/12088090/diff/1/net/proxy/proxy_resolver_v8_t...
> File net/proxy/proxy_resolver_v8_tracing.cc (right):
> 
>
https://codereview.chromium.org/12088090/diff/1/net/proxy/proxy_resolver_v8_t...
> net/proxy/proxy_resolver_v8_tracing.cc:683: pending_dns_ = dns_request;
> On 2013/01/31 21:11:23, Matt Menke wrote:
> > This isn't correct.  If we're doing a blocking call and complete
> synchronously,
> > we won't set pending_dns_ here, and then we'll run into the DCHECK in
> > OnDnsOperationComplete.
> 
> Actually it is the DCHECK in OnDnsOperationComplete() which is wrong. If the
> request completed synchronously, then pending_dns_ should be NULL. Adding the
> assignment shouldn't change that catch.
> 
> Good catch! This is a left-over from one of my prior refactorings.
> 
> For now I have removed the DCHECK. I will follow-up with a test to exercise
this
> situation, and see if I can't clarify the code in that as well.
> 
>
https://codereview.chromium.org/12088090/diff/1/net/proxy/proxy_resolver_v8_t...
> File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):
> 
>
https://codereview.chromium.org/12088090/diff/1/net/proxy/proxy_resolver_v8_t...
> net/proxy/proxy_resolver_v8_tracing_unittest.cc:869:
> TEST_F(ProxyResolverV8TracingTest, CancelWhileBlockedInNonBlockingDns) {
> On 2013/01/31 21:11:23, Matt Menke wrote:
> > Hmm...The purpose of this test was to cancel before the non blocking DNS
hears
> > about the cache miss.  Too bad there's no
> > "MessageLoop::PostTaskToRunBeforeTheCurrentlyExecutingTaskWasPosted()".
> > 
> > We could just wait a couple milliseconds after posting the instead.  Not
> > perfect, but it avoids getting us into this unexpected situation.
> 
> I think the test works as is (it is testing two important situations now). Are
> you suggesting changing it or adding a new one?

Changing it.  I'd rather we do without test-specific code in production code,
when we can avoid it.  Since it's an unusual exit, seems like it gives us even
more cancellation cases to worry about.

That having been said, this LGTM, for now.  I'll check the commit button.

[mmenke, 2013-01-31 21:34:31]

Oops...forgot to publish a comment.

https://codereview.chromium.org/12088090/diff/1/net/proxy/proxy_resolver_v8_t...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/12088090/diff/1/net/proxy/proxy_resolver_v8_t...
net/proxy/proxy_resolver_v8_tracing.cc:683: pending_dns_ = dns_request;
On 2013/01/31 21:29:49, eroman wrote:
> On 2013/01/31 21:11:23, Matt Menke wrote:
> > This isn't correct.  If we're doing a blocking call and complete
> synchronously,
> > we won't set pending_dns_ here, and then we'll run into the DCHECK in
> > OnDnsOperationComplete.
> 
> Actually it is the DCHECK in OnDnsOperationComplete() which is wrong. If the
> request completed synchronously, then pending_dns_ should be NULL. Adding the
> assignment shouldn't change that catch.
> 
> Good catch! This is a left-over from one of my prior refactorings.
> 
> For now I have removed the DCHECK. I will follow-up with a test to exercise
this
> situation, and see if I can't clarify the code in that as well.

You're right.  Was thinking that we could just always set it, but then we'd be
relying on it being non-NULL on synchronous completion.

[commit-bot: I haz the power, 2013-01-31 21:39:29]

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

[commit-bot: I haz the power, 2013-02-01 00:42:57]

Change committed as 180021