Cancel when InitProxyResolver cancels, so we don't use callback later.

Cancel when InitProxyResolver cancels, so we don't use callback later.

Also fix the invariant of data, i.e. null client_callback_ once we
enter state DONE.

BUG=84496
TEST=net_unittests --gtest_filter=Dhcp*:InitProxy*

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

[Jói, 2011-06-08 20:13:07]

Hi Eric,

This should fix all, or at least most of, the crashes related to issue 84496.
There was one variant I saw that isn't a bad client_callback_ that might be
different, although I still suspect it's the same, just a side effect of
corruption.

After this fix goes in, I'm planning to revert http://crrev.com/87047, to make
the DHCP WPAD on by default again on trunk.  This will let me continue to
iterate on the performance aspects with meaningful data, and give meaningful
verification that the crashes are fixed (I don't believe we have a consistent
repro case).  Let me know if you agree with this plan.

Cheers,
Jói

[eroman, 2011-06-08 20:38:01]

LGTM.

Is there a unit-test which tests this behavior?

[Jói, 2011-06-08 20:54:55]

The test would basically just assert that
DhcpProxyScriptFetcherWin::Cancel is called when
InitProxyResolver::Cancel is called, which is what the code now says
:)

The only thing I could think of would be to add a DCHECK to the
InitProxyResolver::Cancel function that DhcpProxyScriptFetcherWin is
in state DONE, but this DCHECK would be no less likely to get removed
in future changes than the call to DhcpProxyScriptFetcherWin::Cancel.
What do you think?

Cheers,
Jói


On Wed, Jun 8, 2011 at 4:38 PM,  <eroman@chromium.org> wrote:
> LGTM.
>
> Is there a unit-test which tests this behavior?
>
> http://codereview.chromium.org/7044058/
>

[eroman, 2011-06-08 21:29:49]

ok fine as is then.

On Wed, Jun 8, 2011 at 1:54 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> The test would basically just assert that
> DhcpProxyScriptFetcherWin::Cancel is called when
> InitProxyResolver::Cancel is called, which is what the code now says
> :)
>
> The only thing I could think of would be to add a DCHECK to the
> InitProxyResolver::Cancel function that DhcpProxyScriptFetcherWin is
> in state DONE, but this DCHECK would be no less likely to get removed
> in future changes than the call to DhcpProxyScriptFetcherWin::Cancel.
> What do you think?
>
> Cheers,
> Jói
>
>
> On Wed, Jun 8, 2011 at 4:38 PM,  <eroman@chromium.org> wrote:
>> LGTM.
>>
>> Is there a unit-test which tests this behavior?
>>
>> http://codereview.chromium.org/7044058/
>>
>

[Jói, 2011-06-09 01:45:49]

Actually I just thought of a test that would most often have crashed
before the fix, will add that tomorrow.

Cheers,
Jói


2011/6/8 Eric Roman <eroman@chromium.org>:
> ok fine as is then.
>
> On Wed, Jun 8, 2011 at 1:54 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> The test would basically just assert that
>> DhcpProxyScriptFetcherWin::Cancel is called when
>> InitProxyResolver::Cancel is called, which is what the code now says
>> :)
>>
>> The only thing I could think of would be to add a DCHECK to the
>> InitProxyResolver::Cancel function that DhcpProxyScriptFetcherWin is
>> in state DONE, but this DCHECK would be no less likely to get removed
>> in future changes than the call to DhcpProxyScriptFetcherWin::Cancel.
>> What do you think?
>>
>> Cheers,
>> Jói
>>
>>
>> On Wed, Jun 8, 2011 at 4:38 PM,  <eroman@chromium.org> wrote:
>>> LGTM.
>>>
>>> Is there a unit-test which tests this behavior?
>>>
>>> http://codereview.chromium.org/7044058/
>>>
>>
>

[Jói, 2011-06-09 21:07:36]

PTAL, I've added a regression test and fixed a check-before-use issue
that an existing test caught on the trybots.

Cheers,
Jói


2011/6/8 Jói Sigurðsson <joi@chromium.org>:
> Actually I just thought of a test that would most often have crashed
> before the fix, will add that tomorrow.
>
> Cheers,
> Jói
>
>
> 2011/6/8 Eric Roman <eroman@chromium.org>:
>> ok fine as is then.
>>
>> On Wed, Jun 8, 2011 at 1:54 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>> The test would basically just assert that
>>> DhcpProxyScriptFetcherWin::Cancel is called when
>>> InitProxyResolver::Cancel is called, which is what the code now says
>>> :)
>>>
>>> The only thing I could think of would be to add a DCHECK to the
>>> InitProxyResolver::Cancel function that DhcpProxyScriptFetcherWin is
>>> in state DONE, but this DCHECK would be no less likely to get removed
>>> in future changes than the call to DhcpProxyScriptFetcherWin::Cancel.
>>> What do you think?
>>>
>>> Cheers,
>>> Jói
>>>
>>>
>>> On Wed, Jun 8, 2011 at 4:38 PM,  <eroman@chromium.org> wrote:
>>>> LGTM.
>>>>
>>>> Is there a unit-test which tests this behavior?
>>>>
>>>> http://codereview.chromium.org/7044058/
>>>>
>>>
>>
>

[eroman, 2011-06-10 21:27:41]

lgtm