Flush the blocking pool in TestBrowserThreadBundle.

content/public/test/test_browser_thread_bundle.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/public/test/test_browser_thread_bundle.h"
 
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
+#include "content/browser/browser_thread_impl.h"
 #include "content/public/test/test_browser_thread.h"
 
 namespace content {
 
 TestBrowserThreadBundle::TestBrowserThreadBundle() {
   Init(DEFAULT);
 }
 
 TestBrowserThreadBundle::TestBrowserThreadBundle(int options) {
   Init(options);
 }
 
 TestBrowserThreadBundle::~TestBrowserThreadBundle() {
+  // To avoid memory leaks, we must ensure that any tasks posted to the blocking
+  // pool via PostTaskAndReply are able to reply back to the originating thread.
+  // Thus we must flush the blocking pool while the browser threads still exist.
+  base::RunLoop().RunUntilIdle();
+  BrowserThreadImpl::FlushThreadPoolHelper();

[awong, 2013/08/12 18:45:53]

I'm thinking this should be done after db_thread_ destruction, but before
ui_thread_ destruction so we simulate production thread "validity" as much as
possible:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

Also, can we add documentation about when/why we flush the blocking pool the way
we do?

Lastly, is FlushThreadPoolHelper() or ShutdownThreadPool more appropriate here?

[earthdok, 2013/08/12 19:18:31]

Note that our goal here is to be tidy, not to simulate the production
environment. In production, shutdown leaks are acceptable.

Here we definitely must flush the thread pool while all threads still exist, to
deal with PostTaskAndReply tasks posted from non-main threads. But I agree that
we should also flush it at the point where we normally shut down the thread
pool. (And perhaps at other points in the destruction sequence, too...) 
Any suggestions for where that would go?
ShutdownThreadPool seems to leave the thread pool in an unusable state for the
next test. Another difference is that ShutdownThreadPool may skip tasks
according to their specified shutdown behavior, whereas FlushThreadPoolHelper
runs all non-delayed tasks.

[awong, 2013/08/12 19:29:10]

I actually disagree with this strongly...the further we diverge from production
thread ordering, the most likely we have hidden dependencies in unittests.
Let's do it in both places then.  Where you have it now is fair game because the
sequence is indeterminate.

Having it happen again after db_thread_ destruction seems important too.
The class comment in the header file is where I'd put it.

 
Gotcha. Sounds good. Thanks!

+
   // To ensure a clean teardown, each thread's message loop must be flushed
   // just before the thread is destroyed. But destroying a fake thread does not
   // automatically flush the message loop, so we have to do it manually.
   // See http://crbug.com/247525 for discussion.
   base::RunLoop().RunUntilIdle();
   io_thread_.reset();
   base::RunLoop().RunUntilIdle();
   cache_thread_.reset();
   base::RunLoop().RunUntilIdle();
   process_launcher_thread_.reset();
@@ skipping 65 matching lines @@
   if (options & REAL_IO_THREAD) {
     io_thread_.reset(new TestBrowserThread(BrowserThread::IO));
     io_thread_->StartIOThread();
   } else {
     io_thread_.reset(
         new TestBrowserThread(BrowserThread::IO, message_loop_.get()));
   }
 }
 
 }  // namespace content