Fixes bad URLRequest leak detection. nss_ocsp creates a

URLRequest. This URLRequest, if valid, is deleted when the IO thread
is being shutdown by way of a DestructionObserver attached to the IO
thread. Prior to this patch this proves problematic as the LeakTracker
is run before DestructionObservers, which means we were errorenously
detecting a leak and causing random UI failures (and perhaps other
tests).

To fix this I've moved the LeakTracker to run after the IO thread
MessageLoop has been deleted (which means DestructionObservers have
been notified). Doing this triggered a DCHECK in
ChromeNetLog::AddEntry:

  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));

Here's the full stack:

	StackTrace::StackTrace() [0xf247c1]
	logging::LogMessage::~LogMessage() [0xf3ef4f]
	ChromeNetLog::AddEntry() [0x5c9639]
	net::BoundNetLog::AddEntry() [0x147d8a3]
	net::BoundNetLog::AddEvent() [0x147d92a]
	net::internal::ClientSocketPoolBaseHelper::CancelRequest() [0x132d032]
	net::ClientSocketPoolBase<>::CancelRequest() [0x134aabd]
	net::TCPClientSocketPool::CancelRequest() [0x1348fdb]
	net::ClientSocketHandle::ResetInternal() [0x13273b7]
	net::ClientSocketHandle::Reset() [0x1327874]
	net::ClientSocketHandle::~ClientSocketHandle() [0x1327895]
	scoped_ptr<>::~scoped_ptr() [0x1304b97]
	net::HttpStreamRequest::~HttpStreamRequest() [0x1300933]
	base::RefCounted<>::Release() [0xa15b83]
	scoped_refptr<>::operator=() [0x12f2da4]
	net::HttpNetworkTransaction::~HttpNetworkTransaction() [0x12ee377]
	scoped_ptr<>::~scoped_ptr() [0x12e2724]
	net::HttpCache::Transaction::~Transaction() [0x12e10a6]
	scoped_ptr<>::reset() [0x12d1f0b]
	URLRequestHttpJob::DestroyTransaction() [0x142229e]
	URLRequestHttpJob::Kill() [0x14252e5]
	URLRequest::DoCancel() [0x137af1e]
	URLRequest::Cancel() [0x137b081]
	(anonymous namespace)::OCSPRequestSession::CancelURLRequest() [0x13142a7]
	(anonymous namespace)::OCSPRequestSession::WillDestroyCurrentMessageLoop() [0x1314b30]
	MessageLoop::~MessageLoop() [0xf421ab]
	base::Thread::ThreadMain() [0xf71395]
	ThreadFunc() [0xf54338]
	start_thread [0x7ff66c6d83f7]
	0x7ff669d5cbbd

This is still on the IO thread, but the DCHECK fails because at the
time the DestructionObservers are run Thread::message_loop() returns
NULL, which means the DCHECK fails. I've fixed this by changing the
DCHECK to pass if the current thread's message loop is NULL. I feel a
bit quesy about this as it seems a bit fragile (well, all this code is
fragile). I would be inclined to make Thread::message_loop() return
the MessageLoop until after the destructor has run, but this seems
equally risky. Let me know what you prefer.

BUG=52022
TEST=none

Review URL: http://codereview.chromium.org/3402006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@59566 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 26 insertions, 5 deletions

diff --git a/chrome/browser/browser_process.h b/chrome/browser/browser_process.h
index 0eeb08d..ff9599a 100644
--- a/chrome/browser/browser_process.h
+++ b/chrome/browser/browser_process.h
@@ -76,7 +76,7 @@ class BrowserProcess {
   // need a MessageLoop*.  If you just want to post a task, use
   // ChromeThread::PostTask (or other variants) as they take care of checking
   // that a thread is still alive, race conditions, lifetime differences etc.
-  // If you still must use this, need to check the return value for NULL.
+  // If you still must use this check the return value for NULL.
   virtual IOThread* io_thread() = 0;
 
   // Returns the thread that we perform random file operations on. For code
diff --git a/chrome/browser/chrome_thread.cc b/chrome/browser/chrome_thread.cc
index 984d188..dc3febd 100644
--- a/chrome/browser/chrome_thread.cc
+++ b/chrome/browser/chrome_thread.cc
@@ -119,6 +119,14 @@ bool ChromeThread::CurrentlyOn(ID identifier) {
 }
 
 // static
+bool ChromeThread::IsMessageLoopValid(ID identifier) {
+  AutoLock lock(lock_);
+  DCHECK(identifier >= 0 && identifier < ID_COUNT);
+  return chrome_threads_[identifier] &&
+         chrome_threads_[identifier]->message_loop();
+}
+
+// static
 bool ChromeThread::PostTask(ID identifier,
                             const tracked_objects::Location& from_here,
                             Task* task) {
diff --git a/chrome/browser/chrome_thread.h b/chrome/browser/chrome_thread.h
index 3831b7c..4bb9387 100644
--- a/chrome/browser/chrome_thread.h
+++ b/chrome/browser/chrome_thread.h
@@ -130,6 +130,11 @@ class ChromeThread : public base::Thread {
   // thread.
   static bool CurrentlyOn(ID identifier);
 
+  // Callable on any thread.  Returns whether the threads message loop is valid.
+  // If this returns false it means the thread is in the process of shutting
+  // down.
+  static bool IsMessageLoopValid(ID identifier);
+
   // If the current message loop is one of the known threads, returns true and
   // sets identifier to its ID.  Otherwise returns false.
   static bool GetCurrentThreadIdentifier(ID* identifier);
diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc
index 45362b5..12bb00e 100644
--- a/chrome/browser/io_thread.cc
+++ b/chrome/browser/io_thread.cc
@@ -236,9 +236,6 @@ void IOThread::CleanUp() {
   delete globals_;
   globals_ = NULL;
 
-  // URLRequest instances must NOT outlive the IO thread.
-  base::LeakTracker<URLRequest>::CheckForLeaks();
-
   BrowserProcessSubThread::CleanUp();
 }
 
@@ -249,6 +246,13 @@ void IOThread::CleanUpAfterMessageLoopDestruction() {
   // combine the two cleanups.
   deferred_net_log_to_delete_.reset();
   BrowserProcessSubThread::CleanUpAfterMessageLoopDestruction();
+
+  // URLRequest instances must NOT outlive the IO thread.
+  //
+  // To allow for URLRequests to be deleted from
+  // MessageLoop::DestructionObserver this check has to happen after CleanUp
+  // (which runs before DestructionObservers).
+  base::LeakTracker<URLRequest>::CheckForLeaks();
 }
 
 net::HttpAuthHandlerFactory* IOThread::CreateDefaultAuthHandlerFactory(
diff --git a/chrome/browser/net/chrome_net_log.cc b/chrome/browser/net/chrome_net_log.cc
index dd9c62f..1003dfa 100644
--- a/chrome/browser/net/chrome_net_log.cc
+++ b/chrome/browser/net/chrome_net_log.cc
@@ -46,7 +46,11 @@ void ChromeNetLog::AddEntry(EventType type,
                             const Source& source,
                             EventPhase phase,
                             EventParameters* params) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
+  // This must be invoked when we're on the IO thread, or if the IO thread's
+  // message loop isn't valid. The later can happen if this is invoked when the
+  // IOThread is shuting down the MessageLoop.
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO) ||
+         !ChromeThread::IsMessageLoopValid(ChromeThread::IO));
 
   // Notify all of the log observers.
   FOR_EACH_OBSERVER(Observer, observers_,