Add more unit tests for net/base/host_resolver.

Add more unit tests for net/base/host_resolver.

BUG=6661

[darin (slow to review), 2009-01-26 20:01:31]

LGTM!

[jar (doing other things), 2009-01-26 21:21:07]

See suggestion below.  Other than that, LGTM.

http://codereview.chromium.org/18775/diff/1/2
File net/base/host_resolver_unittest.cc (right):

http://codereview.chromium.org/18775/diff/1/2#newcode81
Line 81: 3000);
Several folks have argued that rather than using a timer to quit, we should
assume the test will work, and let the automatic hang-time-out kick in when we
really break (which should be pretty much never).  When we use a time constant 
(sadly), we always run the risk that the system will on rare accaisions go slow,
and we'll get a flaky failure.

Suggestion: Don't bother with this PostDelayedTask().  You don't speed up the
test is the common case, and you can induce a flaky failure.

[Paweł Hajdan Jr., 2009-01-27 21:59:18]

jar: thanks for suggestion. Applied.

Testing uncovered weird failures with memory corruption. After adding reference
counting they disappeared. Summary: the code was trying to access HostMapper
that was out of scope and get stupid results. With reference counting all still
relevant HostMappers are kept in memory.

What do you think?

[darin (slow to review), 2009-01-27 22:26:17]

http://codereview.chromium.org/18775/diff/7/219
File net/base/scoped_host_mapper.h (right):

http://codereview.chromium.org/18775/diff/7/219#newcode95
Line 95: // This class sets the HostMapper for a particular scope. It completely
i'm having a hard time understand why the reference counting is necessary.  can
you describe the issue you saw in more detail?

[Paweł Hajdan Jr., 2009-01-28 08:13:46]

On 2009/01/27 22:26:17, darin wrote:
> http://codereview.chromium.org/18775/diff/7/219
> File net/base/scoped_host_mapper.h (right):
> 
> http://codereview.chromium.org/18775/diff/7/219#newcode95
> Line 95: // This class sets the HostMapper for a particular scope. It
completely
> i'm having a hard time understand why the reference counting is necessary. 
can
> you describe the issue you saw in more detail?

Sure. The problem appears when HostResolver is destroyed, but simulated long
asynchronous resolution is pending (for example in CanceledAsynchronousLookup).
I couldn't catch it on Linux in Debug mode, but it reproduces reliably on Mac,
Debug mode and probably Release too. I had similar problem with dns porting
change in Windows Release mode (reproduces reliably), and I think this change
will fix it.

So, here's what you see: After running net_unittests, HostResolver test all
pass, and then it crashes later with a stacktrace like this (it always crashes,
usually when running IDNToUnicode because it's long):

[ RUN      ] NetUtilTest.IDNToUnicode
 
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x6e6b6e51
[Switching to process 13556 thread 0x170b]
0x960f12e5 in std::string::_Rep::_M_grab ()
(gdb) bt
#0  0x960f12e5 in std::string::_Rep::_M_grab ()
#1  0x960f240e in std::basic_string<char, std::char_traits<char>,
std::allocator<char> >::basic_string ()
#2  0x0009e4e0 in net::ScopedHostMapper::Map (this=0xbffff5d8, host=@0x1436d88)
at scoped_host_mapper.h:81
#3  0x0042fb7b in net::ResolveAddrInfo (host=@0x1436d88, port=@0x1436d8c,
out=0x1436dd0) at /Users/admin/chromium-root/src/net/base/host_resolver.cc:53
#4  0x00430bb7 in net::HostResolver::Request::DoLookup (this=0x1436d80) at
/Users/admin/chromium-root/src/net/base/host_resolver.cc:87
#5  0x004301a6 in DispatchToMethod<net::HostResolver::Request, void
(net::HostResolver::Request::*)()> (obj=0x1436d80, method={__pfn = 0x430b7c
<net::HostResolver::Request::DoLookup()>, __delta = 0}, arg=@0x1436d7c) at
tuple.h:383
#6  0x00430205 in RunnableMethod<net::HostResolver::Request, void
(net::HostResolver::Request::*)(), Tuple0>::Run (this=0x1436d60) at task.h:312
#7  0x002344e4 in -[TaskOperation main] (self=0x142bfa0, _cmd=0x92b3fc00) at
/Users/admin/chromium-root/src/base/worker_pool_mac.mm:48
#8  0x91ab8064 in -[NSOperation start] ()
#9  0x91ab7d52 in __runop ()
#10 0x9105640f in _pthread_wqthread ()
#11 0x910562c2 in start_wqthread ()

The problem is that the HostMapper does not exist at this point, but we still
use it. To simulate long lookup, I run PlatformThread::Sleep in HostMapper's Map
method. By the time this call completes, original HostResolver, HostMapper,
test, etc. goes out of scope and is destroyed. The problem is not with
HostResolver's canceling of requests. This crash happens before that.

So, reference counting. In case you wonder why I can't use HostMapper in
HostResolver's Resolve method... well, if not the sleep, I could. But if I want
to sleep, I have to do so in other thread, and it results in code like this.
Please note that HostMapper is only used by tests.

[darin (slow to review), 2009-01-28 08:33:13]

OK... I see now.  Thanks for the detailed explanation.

The core problem is that the worker thread where we call ResolveAddrInfo may
outlive the HostResolver instance, and it may therefore also outlive the
ScopedHostMapper instance.

I don't even think reference counting is a perfect solution here.  From
~ScopedHostMapper we are still calling SetHostMapper(NULL), so there is still in
ResolverAddrInfo involving the testing and usage of host_mapper.  An interleaved
SetHostMapper(NULL) could trigger a crash!

So, we may need a more involved solution...  if there were a way to wait for all
threads in the worker pool to finish that might be nice, but that is not a
trivial change :-/

[Paweł Hajdan Jr., 2009-01-28 17:37:37]

On 2009/01/28 08:33:13, darin wrote:
> OK... I see now.  Thanks for the detailed explanation.
> 
> The core problem is that the worker thread where we call ResolveAddrInfo may
> outlive the HostResolver instance, and it may therefore also outlive the
> ScopedHostMapper instance.
> 
> I don't even think reference counting is a perfect solution here.

That's right, but what do you think after my latest change ("try to fix...")?

> From ~ScopedHostMapper we are still calling SetHostMapper(NULL), so there is
still in
> ResolverAddrInfo involving the testing and usage of host_mapper.  An
interleaved
> SetHostMapper(NULL) could trigger a crash!

Ah, I forgot. Easily fixable, and fixed.

> So, we may need a more involved solution...  if there were a way to wait for
all
> threads in the worker pool to finish that might be nice, but that is not a
> trivial change :-/

What do you think about WaitingHostMapper?

[darin (slow to review), 2009-01-28 18:11:41]

I like this approach...

http://codereview.chromium.org/18775/diff/611/612
File net/base/host_resolver.cc (right):

http://codereview.chromium.org/18775/diff/611/612#newcode84
Line 84: original_host_mapper_(host_mapper),
nit: it seems OK to just name this host_mapper_ since it is the HostMapper that
this Request will use.

http://codereview.chromium.org/18775/diff/611/612#newcode156
Line 156: // Hold reference to the host mapper to avoid crash.
"Hold an owning reference to the host mapper that we are going to use.  This may
not be the current host mapper by the time we call ResolveAddrInfo, but that's
OK... we'll use it anyways, and the owning reference ensures that it remains
valid until we are done."

http://codereview.chromium.org/18775/diff/611/613
File net/base/host_resolver.h (right):

http://codereview.chromium.org/18775/diff/611/613#newcode60
Line 60: class HostMapper : public base::RefCounted<HostMapper> {
I think this needs to be RefCountedThreadSafe because the Request class may die
on a background thread and therefore call Release on the HostMapper on a
background thread.

http://codereview.chromium.org/18775/diff/611/613#newcode73
Line 73: void SetPreviousHostMapper(HostMapper* mapper);
nit: perhaps this can just be an inline set_previous_mapper method?

http://codereview.chromium.org/18775/diff/611/615
File net/base/host_resolver_unittest.h (right):

http://codereview.chromium.org/18775/diff/611/615#newcode131
Line 131: }
it might be nice to add a .get() method here that returns the
current_host_mapper_.  that way in run_all_unittests.cc you wouldn't need to
hold an extra reference to the HostMapper.

http://codereview.chromium.org/18775/diff/611/616
File net/base/run_all_unittests.cc (right):

http://codereview.chromium.org/18775/diff/611/616#newcode39
Line 39: host_mapper_(new net::RuleBasedHostMapper()),
it seems like you can do away with host_mapper_ by adding a way to access the
HostMapper held by a ScopedHostMapper.

http://codereview.chromium.org/18775/diff/611/618
File net/base/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/18775/diff/611/618#newcode25
Line 25: : host_mapper_(new net::RuleBasedHostMapper()),
ditto.

http://codereview.chromium.org/18775/diff/611/619
File net/http/http_network_layer_unittest.cc (right):

http://codereview.chromium.org/18775/diff/611/619#newcode16
Line 16: : host_mapper_(new net::RuleBasedHostMapper()),
ditto

[Paweł Hajdan Jr., 2009-01-28 19:16:39]

Fixed, except get() because it would require a down_cast.

I also added set_previous_mapper call which I forgot before.

[darin (slow to review), 2009-01-28 19:26:02]

LGTM

I wish there were a simpler way for the host_mapper_ business, but OK.