Flush the blocking pool in TestBrowserThreadBundle.

Flush the blocking pool in TestBrowserThreadBundle.

While we already flush the blocking pool in a OnTestEnd() hook (installed in
ContentTestSuiteBase), that is too late for tests that create their own threads.
The test (and the TestBrowserThreadBundle that it contains) will have been
destroyed by the time the hook runs, and any tasks posted to the blocking pool
via PostTaskAndReply will not be able to reply back to the originating thread.
This causes the task and reply Closures to be leaked. Flushing the blocking pool
before TestBrowserThreadBundle is destroyed should fix those leaks.

BUG=168415, 270673, 270658
TBR=ajwong@chromium.org, jochen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217268

[earthdok, 2013-08-12 15:06:16]

please take a look

sky for content/browser/
jochen for content/public/test/

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

https://codereview.chromium.org/22799005/diff/4001/content/public/test/test_b...
File content/public/test/test_browser_thread_bundle.cc (right):

https://codereview.chromium.org/22799005/diff/4001/content/public/test/test_b...
content/public/test/test_browser_thread_bundle.cc:27:
BrowserThreadImpl::FlushThreadPoolHelper();
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]

https://codereview.chromium.org/22799005/diff/4001/content/public/test/test_b...
File content/public/test/test_browser_thread_bundle.cc (right):

https://codereview.chromium.org/22799005/diff/4001/content/public/test/test_b...
content/public/test/test_browser_thread_bundle.cc:27:
BrowserThreadImpl::FlushThreadPoolHelper();
On 2013/08/12 18:45:53, awong wrote:
> 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...
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...) 

> Also, can we add documentation about when/why we flush the blocking pool the
way
> we do?
Any suggestions for where that would go?

> Lastly, is FlushThreadPoolHelper() or ShutdownThreadPool more appropriate
here?
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.

[jochen (gone - plz use gerrit), 2013-08-12 19:23:31]

i don't really know about this code. Albert seems to have some clue, so I'm
deferring to him.

[earthdok, 2013-08-12 19:25:37]

Updated as per Albert's suggestion.

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

https://codereview.chromium.org/22799005/diff/4001/content/public/test/test_b...
File content/public/test/test_browser_thread_bundle.cc (right):

https://codereview.chromium.org/22799005/diff/4001/content/public/test/test_b...
content/public/test/test_browser_thread_bundle.cc:27:
BrowserThreadImpl::FlushThreadPoolHelper();
On 2013/08/12 19:18:31, earthdok wrote:
> On 2013/08/12 18:45:53, awong wrote:
> > 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...
> Note that our goal here is to be tidy, not to simulate the production
> environment. In production, shutdown leaks are acceptable.

I actually disagree with this strongly...the further we diverge from production
thread ordering, the most likely we have hidden dependencies in unittests.


> 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...) 

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.


> > Also, can we add documentation about when/why we flush the blocking pool the
> way
> > we do?
> Any suggestions for where that would go?

The class comment in the header file is where I'd put it.

 
> > Lastly, is FlushThreadPoolHelper() or ShutdownThreadPool more appropriate
> here?
> 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.

Gotcha. Sounds good. Thanks!

[earthdok, 2013-08-12 19:53:40]

On 2013/08/12 19:29:10, awong wrote:
> The class comment in the header file is where I'd put it.

Ok, updated. It essentially repeats what I said in the comments to the
destructor. Not sure if that's what you had in mind...

[awong, 2013-08-12 21:05:44]

LGTM

[earthdok, 2013-08-13 08:48:26]

Jochen, please approve.

[jochen (gone - plz use gerrit), 2013-08-13 09:06:10]

lgtm

[commit-bot: I haz the power, 2013-08-13 10:30:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/earthdok@chromium.org/22799005/15001

[commit-bot: I haz the power, 2013-08-13 11:46:24]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-08-13 12:00:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/earthdok@chromium.org/22799005/15001

[commit-bot: I haz the power, 2013-08-13 12:56:15]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-08-13 15:32:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/earthdok@chromium.org/22799005/15001

[earthdok, 2013-08-13 16:10:13]

Committed patchset #4 manually as r217268 (presubmit successful).