Fix content_browsertests on Android.

Fix content_browsertests on Android.

The content_browsertests on Android exit before IO thread is joined.
Some tasks on IO thread needs to access GetContentClient() which has
been reset by ContentShellTestSuiteInitializer::OnTestEnd when test
is terminated.

To keep closer to production code, this patch tries to not reset
ContentClient on Android when test is ended.

BUG=181069
R=jam@chromium.org

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

[Yaron, 2013-10-18 23:03:16]

+jam who reviewed the CL
+kristianm who recently modified this for android

https://codereview.chromium.org/27518013/diff/1/content/public/test/browser_t...
File content/public/test/browser_test_base.cc (left):

https://codereview.chromium.org/27518013/diff/1/content/public/test/browser_t...
content/public/test/browser_test_base.cc:221: // We are done running the test by
now. During teardown we
It looks like this was added in https://chromiumcodereview.appspot.com/12188016

Have you verified that this doesn't break any other tests? I kicked off
android_fyi_dbg which should run content_browsertests.

I'm not really familiar with this enough to know if it makes sense.

[wjia(left Chromium), 2013-10-20 03:16:29]

Please hold on review. I am looking into the crash in content_browsertests.

[Kristian Monsen, 2013-10-21 19:58:49]

I think in general tests do not use the java message loop as it does not support
nested message loops. Has this changed recently?

[wjia(left Chromium), 2013-11-01 17:54:26]

The patch is now ready for review. PTAL.

[jam, 2013-11-06 21:33:52]

+Nilesh who brought up content_browsertests on Android.

I don't understand why this is needed. I know that on desktop chrome,
ContentShellTestSuiteInitializer::OnTestEnd which deletes ContentClient is run
_after_ BrowserMainLoop::ShutdownThreadsAndCleanUp() is run so we don't have the
issue that you describe. Is this different in Android? Seems we need to fix that
if so.

[wjia(left Chromium), 2013-11-06 23:37:52]

On 2013/11/06 21:33:52, jam wrote:
> +Nilesh who brought up content_browsertests on Android.
> 
> I don't understand why this is needed. I know that on desktop chrome,
> ContentShellTestSuiteInitializer::OnTestEnd which deletes ContentClient is run
> _after_ BrowserMainLoop::ShutdownThreadsAndCleanUp() is run so we don't have
the
> issue that you describe. Is this different in Android? Seems we need to fix
that
> if so.

My debugging shows BrowserMainLoop::ShutdownThreadsAndCleanUp() is not
called (actuallly it's never called) before
ContentShellTestSuiteInitializer::OnTestEnd is called.
This patch is trying to correct that.

Nilesh might have more insight.

[nilesh, 2013-11-12 00:35:16]

On 2013/11/06 23:37:52, wjia wrote:
> On 2013/11/06 21:33:52, jam wrote:
> > +Nilesh who brought up content_browsertests on Android.
> > 
> > I don't understand why this is needed. I know that on desktop chrome,
> > ContentShellTestSuiteInitializer::OnTestEnd which deletes ContentClient is
run
> > _after_ BrowserMainLoop::ShutdownThreadsAndCleanUp() is run so we don't have
> the
> > issue that you describe. Is this different in Android? Seems we need to fix
> that
> > if so.
> 
> My debugging shows BrowserMainLoop::ShutdownThreadsAndCleanUp() is not
> called (actuallly it's never called) before
> ContentShellTestSuiteInitializer::OnTestEnd is called.
> This patch is trying to correct that.
> 
> Nilesh might have more insight.

AFAIK:
On Android (in tests and in production), only BrowserMainRunner::Create and
BrowserMainRunner::Initialize are ever called. I think this is because there is
no browser shutdown path supported/needed on Android. Trying to join threads
only for tests is one solution. Other way around this might be for your test to
wait for some notification/condition to allow for the IO thread to finish
processing. Another solution is to not reset the ContentClient for Android
(which is closer to what happens in production).

[jam, 2013-11-15 08:25:21]

On 2013/11/12 00:35:16, nilesh wrote:
> On 2013/11/06 23:37:52, wjia wrote:
> > On 2013/11/06 21:33:52, jam wrote:
> > > +Nilesh who brought up content_browsertests on Android.
> > > 
> > > I don't understand why this is needed. I know that on desktop chrome,
> > > ContentShellTestSuiteInitializer::OnTestEnd which deletes ContentClient is
> run
> > > _after_ BrowserMainLoop::ShutdownThreadsAndCleanUp() is run so we don't
have
> > the
> > > issue that you describe. Is this different in Android? Seems we need to
fix
> > that
> > > if so.
> > 
> > My debugging shows BrowserMainLoop::ShutdownThreadsAndCleanUp() is not
> > called (actuallly it's never called) before
> > ContentShellTestSuiteInitializer::OnTestEnd is called.
> > This patch is trying to correct that.
> > 
> > Nilesh might have more insight.
> 
> AFAIK:
> On Android (in tests and in production), only BrowserMainRunner::Create and
> BrowserMainRunner::Initialize are ever called. I think this is because there
is
> no browser shutdown path supported/needed on Android. Trying to join threads
> only for tests is one solution. Other way around this might be for your test
to
> wait for some notification/condition to allow for the IO thread to finish
> processing. Another solution is to not reset the ContentClient for Android
> (which is closer to what happens in production).

The closer we can make the browser test harness to what happens in production,
the better.

[wjia(left Chromium), 2013-11-17 17:10:09]

PTAL. Now the patch will not reset ContentClient in
ContentShellTestSuiteInitializer::OnTestEnd on Android. This should be closer to
production code.

The failure in content_browsertests on Android has been there before this patch.

[jam, 2013-11-18 05:41:10]

lgtm, thanks

https://codereview.chromium.org/27518013/diff/619001/content/test/content_tes...
File content/test/content_test_launcher.cc (right):

https://codereview.chromium.org/27518013/diff/619001/content/test/content_tes...
content/test/content_test_launcher.cc:47: #if !defined(OS_ANDROID)
nit: please add a comment about why this is done

[wjia(left Chromium), 2013-11-18 18:13:24]

Committed patchset #9 manually as r235761 (presubmit successful).