Add checks to DEBUG mode that no instance of URLRequest or URLFetcher survive...

Add checks to DEBUG mode that no instance of URLRequest or URLFetcher survives the destruction of the IO thread.

This checking is done by introducing a new helper class to base called LeakTracker. Classes that you want to check for leaks just need to extend LeakTracker.

The reason I am picking on URLFetcher / URLRequest, is I believe we have a bug that is making an instance of URLFetcher to outlive the IO thread.

This causes various sorts of badness.

For example:
If URLFetcher survives the IO thread, then URLRequestContext remains referenced and therefore also survives IO thread. In turn HostResolverImpl survives the IO thread, so any outstanding resolve requests are NOT cancelled before the IO thread is decomissioned. So now, when the worker thread doing the DNS resolve finally finishes (assuming it finishes before the rogue URLRequest is destroyed), it post the result to a defunct message loop. KAB00m! (http://crbug.com/15513)

Moreover, I believe we hit this same problem sporadically in AutomationProxyTest.AutocompleteGetSetText -- the test is flaky on the buildbots, and I've seen DCHECKs which suggest it is related to this issue.

BUG=http://crbug.com/18372

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

[darin (slow to review), 2009-07-31 17:03:11]

more comments soon...

http://codereview.chromium.org/160447/diff/31/39
File chrome/browser/net/url_fetcher.cc (right):

http://codereview.chromium.org/160447/diff/31/39#newcode245
Line 245: request_context_ = NULL;
I don't think the lifetime of the URLFetcher is what matters.  What matters is
if we reach this line _or_ destroy the Core object before the IO thread
disappears.

[eroman, 2009-07-31 18:41:43]

http://codereview.chromium.org/160447/diff/31/39
File chrome/browser/net/url_fetcher.cc (right):

http://codereview.chromium.org/160447/diff/31/39#newcode245
Line 245: request_context_ = NULL;
On 2009/07/31 17:03:11, darin wrote:
> I don't think the lifetime of the URLFetcher is what matters.  What matters is
> if we reach this line _or_ destroy the Core object before the IO thread
> disappears.

Truly. Note that URLFetcher's destructor calls |core_->Stop()|, so we reach this
line either way as a consequent of deleting URLFetcher.

(It posts a task to run CancelURLRequest on the IO thread. When the IO thread is
stopped, any already pending tasks are necessarily run).

So I believe the check that URLFetchers have been destroyed is sufficient (the
header has a similar warning that they should not outlive the IO thread, so we
checking an already established invariant).

[darin (slow to review), 2009-08-06 06:11:48]

Very sorry for not reviewing sooner.  This looks great.  I have some suggestions
for possible improvements:

http://codereview.chromium.org/160447/diff/31/35
File base/leak_tracker.h (right):

http://codereview.chromium.org/160447/diff/31/35#newcode79
Line 79: static LeakTrackerImpl tracker;
do you care about this lazy initialization being threadsafe?

http://codereview.chromium.org/160447/diff/31/35#newcode81
Line 81: }
you could also make a LeakTracker object have "next" and "prev" pointers as well
as a StackTrace member variable.  the overhead for the leak tracker becomes much
more lightweight this way.  Find is O(1) and Add involves no allocation.

Making this class lighter might be nice in case we wish to use it for classes
that are allocated much more frequently than URLRequest and URLFetcher.

http://codereview.chromium.org/160447/diff/31/38
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/160447/diff/31/38#newcode317
Line 317: io_thread_.reset();
More tasks could get posted to the IO thread between CleanupOnIOThread and IO
thread destruction.  Maybe it would be better to run CleanupOnIOThread from
Thread::CleanUp instead.

http://codereview.chromium.org/160447/diff/31/39
File chrome/browser/net/url_fetcher.cc (right):

http://codereview.chromium.org/160447/diff/31/39#newcode315
Line 315: void CheckIOThreadIsReadyToDie() {
this doesn't seem like the best file to implement this function in.  how about
just implementing it in BrowserProcessImpl?  that class is intended to know
about everything in the system, whereas URLFetcher really isn't.

http://codereview.chromium.org/160447/diff/31/32
File net/url_request/url_request.h (right):

http://codereview.chromium.org/160447/diff/31/32#newcode53
Line 53: class URLRequest : public base::LeakTracker<URLRequest> {
how about just declaring the LeakTracker as a member variable?  the address of a
LeakTracker member variable is still going to be unique.

[eroman, 2009-08-06 06:35:20]

Thanks for the review comments!

I will be out vacationing tomorrow, but will try to respond soon.

(RE: Thread::CleanUp() -- sweetness! I was afraid I'd have to accept the race;
didn't realize we had this gizmo).

[eroman, 2009-08-07 19:05:12]

http://codereview.chromium.org/160447/diff/31/35
File base/leak_tracker.h (right):

http://codereview.chromium.org/160447/diff/31/35#newcode79
Line 79: static LeakTrackerImpl tracker;
On 2009/08/06 06:11:48, darin wrote:
> do you care about this lazy initialization being threadsafe? 

No, I am not concerned about the initialization being threadsafe (in theory
these should only live on one thread anyway...)

http://codereview.chromium.org/160447/diff/31/35#newcode81
Line 81: }
On 2009/08/06 06:11:48, darin wrote:
> you could also make a LeakTracker object have "next" and "prev" pointers as
well
> as a StackTrace member variable.  the overhead for the leak tracker becomes
much
> more lightweight this way.  Find is O(1) and Add involves no allocation.
> 
> Making this class lighter might be nice in case we wish to use it for classes
> that are allocated much more frequently than URLRequest and URLFetcher.

Done.
Much better!

http://codereview.chromium.org/160447/diff/31/32
File net/url_request/url_request.h (right):

http://codereview.chromium.org/160447/diff/31/32#newcode53
Line 53: class URLRequest : public base::LeakTracker<URLRequest> {
On 2009/08/06 06:11:48, darin wrote:
> how about just declaring the LeakTracker as a member variable?  the address of
a
> LeakTracker member variable is still going to be unique.

I am fond of the method where *all* you have to do is inherit LeakTracker, since
it keeps the debug-only hacks hidden.

If I instead go the member variable route, then debug logic spills into the
class's implementation. In particular:

  1. Need to call an "add" method in each subclass's ctor(s)
  2. Need to call a "remove" method in each subclass's dtor
  3. Need to expose the member variable for calls to CheckForLeaks().

[darin (slow to review), 2009-08-08 03:52:15]

On Fri, Aug 7, 2009 at 12:05 PM, <eroman@chromium.org> wrote:

>
> http://codereview.chromium.org/160447/diff/31/35
> File base/leak_tracker.h (right):
>
> http://codereview.chromium.org/160447/diff/31/35#newcode79
> Line 79: static LeakTrackerImpl tracker;
> On 2009/08/06 06:11:48, darin wrote:
>
>> do you care about this lazy initialization being threadsafe?
>>
>
> No, I am not concerned about the initialization being threadsafe (in
> theory these should only live on one thread anyway...)
>

OK


>
>
> http://codereview.chromium.org/160447/diff/31/35#newcode81
> Line 81: }
> On 2009/08/06 06:11:48, darin wrote:
>
>> you could also make a LeakTracker object have "next" and "prev"
>>
> pointers as well
>
>> as a StackTrace member variable.  the overhead for the leak tracker
>>
> becomes much
>
>> more lightweight this way.  Find is O(1) and Add involves no
>>
> allocation.
>
>  Making this class lighter might be nice in case we wish to use it for
>>
> classes
>
>> that are allocated much more frequently than URLRequest and
>>
> URLFetcher.
>
> Done.
> Much better!
>


Great!


>
>
> http://codereview.chromium.org/160447/diff/31/32
> File net/url_request/url_request.h (right):
>
> http://codereview.chromium.org/160447/diff/31/32#newcode53
> Line 53: class URLRequest : public base::LeakTracker<URLRequest> {
> On 2009/08/06 06:11:48, darin wrote:
>
>> how about just declaring the LeakTracker as a member variable?  the
>>
> address of a
>
>> LeakTracker member variable is still going to be unique.
>>
>
> I am fond of the method where *all* you have to do is inherit
> LeakTracker, since it keeps the debug-only hacks hidden.
>
> If I instead go the member variable route, then debug logic spills into
> the class's implementation. In particular:
>
>  1. Need to call an "add" method in each subclass's ctor(s)
>  2. Need to call a "remove" method in each subclass's dtor
>  3. Need to expose the member variable for calls to CheckForLeaks().
>

No, I was suggesting not doing any of that.  Just let the member variable
self initialize.  The address passed to "add" would be the address of the
LeakChecker variable, which should lie within the allocation of the
containing class, so it is just as unique as the address of the containing
class instance.

Exposing CheckForLeaks is an interesting one.  Can't you get by with that
being a static function on LeakChecker<Foo>?  So to check for leaks of
classes of type Foo, you just call LeakChecker<Foo>::CheckForLeaks().

-Darin



>
> http://codereview.chromium.org/160447
>

[darin (slow to review), 2009-08-10 06:18:01]

http://codereview.chromium.org/160447/diff/3055/2046
File base/leak_tracker.h (right):

http://codereview.chromium.org/160447/diff/3055/2046#newcode89
Line 89: class LinkedList {
not necessarily fodder for this CL, but it might be cool to define a LinkNode<T>
class that LeakTracker could inherit from.  LinkedList could also just be an
instance of LinkNode<T>... if configured as a circular linked list, the check
for !head_ goes away.  see
http://mxr.mozilla.org/mozilla/source/nsprpub/pr/include/prclist.h

[eroman, 2009-08-10 21:34:59]

> No, I was suggesting not doing any of that.

Oh, now I get what you were suggesting.
Yes, I like that!

[eroman, 2009-08-11 02:16:41]

http://codereview.chromium.org/160447/diff/3055/2046
File base/leak_tracker.h (right):

http://codereview.chromium.org/160447/diff/3055/2046#newcode89
Line 89: class LinkedList {
On 2009/08/10 06:18:01, darin wrote:
> not necessarily fodder for this CL, but it might be cool to define a
LinkNode<T>
> class that LeakTracker could inherit from.  LinkedList could also just be an
> instance of LinkNode<T>... if configured as a circular linked list, the check
> for !head_ goes away.  see
> http://mxr.mozilla.org/mozilla/source/nsprpub/pr/include/prclist.h

Done.

Added {linked_list.h, linked_list_unittest.cc} to this code review. 

Hopefully I am not re-inventing the wheel.

[darin (slow to review), 2009-08-11 03:55:43]

beautiful!  thanks for going the extra distance here.  just a couple trivial
snags and then LGTM:

http://codereview.chromium.org/160447/diff/2069/3072
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/160447/diff/2069/3072#newcode109
Line 109: URLFetcher::CheckForLeaks();
these won't compile right?  they need to use the LeakChecker<T>::CheckForLeaks()
form i think.

http://codereview.chromium.org/160447/diff/2069/3072#newcode343
Line 343: chrome_browser_net::EnsureDnsPrefetchShutdown();
maybe this can happen from the IOThread::CleanUp function as well?

[eroman, 2009-08-11 19:10:12]

http://codereview.chromium.org/160447/diff/2069/3072
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/160447/diff/2069/3072#newcode109
Line 109: URLFetcher::CheckForLeaks();
On 2009/08/11 03:55:43, darin wrote:
> these won't compile right?  they need to use the
LeakChecker<T>::CheckForLeaks()
> form i think.

Done.

(I avoid building chrome_dll like the plague, since it tortures my system. I
mostly been using the try-server to test iterative compiles, but since it was
broken last night on a webkit error I hadn't caught this yet)

http://codereview.chromium.org/160447/diff/2069/3072#newcode343
Line 343: chrome_browser_net::EnsureDnsPrefetchShutdown();
On 2009/08/11 03:55:43, darin wrote:
> maybe this can happen from the IOThread::CleanUp function as well?

That is probably correct.

I have put a TODO since I want to spend some more time verifying before I make
such a change.

[darin (slow to review), 2009-08-11 19:11:58]

LGTM