Do not spawn a thread in browser/interactive ui tests before spawning sandbox host process

Do not spawn a thread in browser/interactive ui tests before spawning sandbox host process
* Introduced DBThreadManager::SetInstanceForTesting to specify the instance to be used when DBThreadManager::Initialize is called in the browser setup rocess.
* Temporarily stop the thread in EmbeddedTestServer to fork/exec sandbox host process properly.

BUG=322732
TBR=sky@chromium.org

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

[satorux1, 2013-11-26 01:22:57]

https://codereview.chromium.org/83633004/diff/170001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/83633004/diff/170001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/kiosk_browsertest.cc:252:
embedded_test_server()->StopThread();
this is not obvious. could you add some comment about why we need to do this?
why do we need to stop the thread here, and restart in SetUpOnMainThread()?

https://codereview.chromium.org/83633004/diff/170001/chromeos/dbus/dbus_threa...
File chromeos/dbus/dbus_thread_manager.h (right):

https://codereview.chromium.org/83633004/diff/170001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.h:90: static void
SetInstanceForTesting(DBusThreadManager* dbus_thread_manager);
It wasn't clear to me why this needed to be introduced. Could you explain when
to use this and when to use InitializeForTesting() in the comment?

https://codereview.chromium.org/83633004/diff/170001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.h:93: // DBusThreadManager using
SetInstanceForTest first.
maybe add:

// The injected object will be owned by the internal pointer and deleted
// by Shutdown().

https://codereview.chromium.org/83633004/diff/170001/net/test/embedded_test_s...
File net/test/embedded_test_server/embedded_test_server.h (right):

https://codereview.chromium.org/83633004/diff/170001/net/test/embedded_test_s...
net/test/embedded_test_server/embedded_test_server.h:43: void
DetachFromThread();
a function comment is missing.

[oshima, 2013-11-26 02:18:42]

https://codereview.chromium.org/83633004/diff/170001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/83633004/diff/170001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/kiosk_browsertest.cc:252:
embedded_test_server()->StopThread();
On 2013/11/26 01:22:58, satorux1 wrote:
> this is not obvious. could you add some comment about why we need to do this?
> why do we need to stop the thread here, and restart in SetUpOnMainThread()?

I added comment.

https://codereview.chromium.org/83633004/diff/170001/chromeos/dbus/dbus_threa...
File chromeos/dbus/dbus_thread_manager.h (right):

https://codereview.chromium.org/83633004/diff/170001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.h:90: static void
SetInstanceForTesting(DBusThreadManager* dbus_thread_manager);
On 2013/11/26 01:22:58, satorux1 wrote:
> It wasn't clear to me why this needed to be introduced. Could you explain when
> to use this and when to use InitializeForTesting() in the comment?

This should be used in the test that creates a browser, which calls
DBusThreadManager::Initialize inside. The problem is that calling
InitializeForTesting creates a thread before sandbox host process is created
(which happens only with a test that creates a browser).

This makes the execution order of DBusThreadManager identical to one in real
scenario, so we can catch the change that will break this assumption.

https://codereview.chromium.org/83633004/diff/170001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.h:93: // DBusThreadManager using
SetInstanceForTest first.
On 2013/11/26 01:22:58, satorux1 wrote:
> maybe add:
> 
> // The injected object will be owned by the internal pointer and deleted
> // by Shutdown().

Do you want to switch to use scoped_ptr instead?
I'm fine with either way. just asking.

https://codereview.chromium.org/83633004/diff/170001/net/test/embedded_test_s...
File net/test/embedded_test_server/embedded_test_server.h (right):

https://codereview.chromium.org/83633004/diff/170001/net/test/embedded_test_s...
net/test/embedded_test_server/embedded_test_server.h:43: void
DetachFromThread();
On 2013/11/26 01:22:58, satorux1 wrote:
> a function comment is missing.

Done.

[oshima, 2013-11-26 02:26:02]

https://codereview.chromium.org/83633004/diff/210001/net/test/embedded_test_s...
File net/test/embedded_test_server/embedded_test_server.h (right):

https://codereview.chromium.org/83633004/diff/210001/net/test/embedded_test_s...
net/test/embedded_test_server/embedded_test_server.h:55: // The common use case
for unit tests is below:
Forgot to mention. I'll update the comment and add another examples for browser
test case.

[satorux1, 2013-11-26 06:52:07]

LGTM

https://codereview.chromium.org/83633004/diff/170001/chromeos/dbus/dbus_threa...
File chromeos/dbus/dbus_thread_manager.h (right):

https://codereview.chromium.org/83633004/diff/170001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.h:90: static void
SetInstanceForTesting(DBusThreadManager* dbus_thread_manager);
On 2013/11/26 02:18:43, oshima wrote:
> On 2013/11/26 01:22:58, satorux1 wrote:
> > It wasn't clear to me why this needed to be introduced. Could you explain
when
> > to use this and when to use InitializeForTesting() in the comment?
> 
> This should be used in the test that creates a browser, which calls
> DBusThreadManager::Initialize inside. The problem is that calling
> InitializeForTesting creates a thread before sandbox host process is created
> (which happens only with a test that creates a browser).
> 
> This makes the execution order of DBusThreadManager identical to one in real
> scenario, so we can catch the change that will break this assumption.
> 

Please add the explanation in the comment.

https://codereview.chromium.org/83633004/diff/170001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.h:93: // DBusThreadManager using
SetInstanceForTest first.
On 2013/11/26 02:18:43, oshima wrote:
> On 2013/11/26 01:22:58, satorux1 wrote:
> > maybe add:
> > 
> > // The injected object will be owned by the internal pointer and deleted
> > // by Shutdown().
> 
> Do you want to switch to use scoped_ptr instead?
> I'm fine with either way. just asking.

Let's keep it as-is for now.

[oshima, 2013-11-26 20:46:44]

I needed one more change in EmbeddedTestServer to listen on the socket using new
thread. (I thought I tested the change locally, but I probably have missed
something) Please take another look.

https://codereview.chromium.org/83633004/diff/170001/chromeos/dbus/dbus_threa...
File chromeos/dbus/dbus_thread_manager.h (right):

https://codereview.chromium.org/83633004/diff/170001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.h:90: static void
SetInstanceForTesting(DBusThreadManager* dbus_thread_manager);
On 2013/11/26 06:52:07, satorux1 wrote:
> On 2013/11/26 02:18:43, oshima wrote:
> > On 2013/11/26 01:22:58, satorux1 wrote:
> > > It wasn't clear to me why this needed to be introduced. Could you explain
> when
> > > to use this and when to use InitializeForTesting() in the comment?
> > 
> > This should be used in the test that creates a browser, which calls
> > DBusThreadManager::Initialize inside. The problem is that calling
> > InitializeForTesting creates a thread before sandbox host process is created
> > (which happens only with a test that creates a browser).
> > 
> > This makes the execution order of DBusThreadManager identical to one in real
> > scenario, so we can catch the change that will break this assumption.
> > 
> 
> Please add the explanation in the comment.

Done.

https://codereview.chromium.org/83633004/diff/170001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.h:93: // DBusThreadManager using
SetInstanceForTest first.
On 2013/11/26 06:52:07, satorux1 wrote:
> On 2013/11/26 02:18:43, oshima wrote:
> > On 2013/11/26 01:22:58, satorux1 wrote:
> > > maybe add:
> > > 
> > > // The injected object will be owned by the internal pointer and deleted
> > > // by Shutdown().
> > 
> > Do you want to switch to use scoped_ptr instead?
> > I'm fine with either way. just asking.
> 
> Let's keep it as-is for now.

Done.

[satorux1, 2013-11-27 03:27:17]

https://codereview.chromium.org/83633004/diff/280001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest.cc (left):

https://codereview.chromium.org/83633004/diff/280001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:1276:
chromeos::DBusThreadManager::Shutdown();
Is it ok to remove this? I thought FakeDBusThreadManager was deleted by
Shutdown()

https://codereview.chromium.org/83633004/diff/280001/chromeos/dbus/dbus_threa...
File chromeos/dbus/dbus_thread_manager.h (right):

https://codereview.chromium.org/83633004/diff/280001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.h:87: // to be used in |Initialize()| for
testing. Tests that calls calls
Tests that calls calls -> Tests that call

https://codereview.chromium.org/83633004/diff/280001/net/test/embedded_test_s...
File net/test/embedded_test_server/embedded_test_server.cc (right):

https://codereview.chromium.org/83633004/diff/280001/net/test/embedded_test_s...
net/test/embedded_test_server/embedded_test_server.cc:140:
&EmbeddedTestServer::ListenOnIOThread, base::Unretained(this)))) {
did you meant to add LOG(ERROR) here? Otherwise the if block doesn't mean
anything.

[oshima, 2013-11-27 07:41:54]

https://codereview.chromium.org/83633004/diff/280001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest.cc (left):

https://codereview.chromium.org/83633004/diff/280001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:1276:
chromeos::DBusThreadManager::Shutdown();
On 2013/11/27 03:27:17, satorux1 wrote:
> Is it ok to remove this? I thought FakeDBusThreadManager was deleted by
> Shutdown()

Yes, this wasn't necessary because Shutdown is called in regular shutdown
process. (Browser tests launches and shutdown browser, which calls Initialize
and Shutdown)

https://codereview.chromium.org/83633004/diff/280001/chromeos/dbus/dbus_threa...
File chromeos/dbus/dbus_thread_manager.h (right):

https://codereview.chromium.org/83633004/diff/280001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.h:87: // to be used in |Initialize()| for
testing. Tests that calls calls
On 2013/11/27 03:27:17, satorux1 wrote:
> Tests that calls calls -> Tests that call

Done.

https://codereview.chromium.org/83633004/diff/280001/net/test/embedded_test_s...
File net/test/embedded_test_server/embedded_test_server.cc (right):

https://codereview.chromium.org/83633004/diff/280001/net/test/embedded_test_s...
net/test/embedded_test_server/embedded_test_server.cc:140:
&EmbeddedTestServer::ListenOnIOThread, base::Unretained(this)))) {
On 2013/11/27 03:27:17, satorux1 wrote:
> did you meant to add LOG(ERROR) here? Otherwise the if block doesn't mean
> anything.

changed to CHECK. This should never fail in tests (and this failure leads to
test failure anyway)

[satorux1, 2013-11-28 06:29:47]

lgtm. sorry for the belated response

[commit-bot: I haz the power, 2013-11-28 09:15:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/83633004/290001

[commit-bot: I haz the power, 2013-11-28 09:28:33]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[oshima, 2013-12-02 18:40:41]

kbr -> content/browser/renderer_host/render_sandbox_host_linux.cc

[Ken Russell (switch to Gerrit), 2013-12-03 22:02:11]

piman is a better reviewer for the
content/browser/renderer_host/render_sandbox_host_linux.cc change than me.

+jln as FYI

[piman, 2013-12-03 22:51:52]

lgtm

[oshima, 2013-12-03 23:46:50]

tbr'ing sky for trivial changes in chrome/browser content/browser

[commit-bot: I haz the power, 2013-12-04 00:13:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/83633004/290001

[commit-bot: I haz the power, 2013-12-04 00:35:49]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-12-04 00:47:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/83633004/310001

[commit-bot: I haz the power, 2013-12-04 04:03:01]

Change committed as 238554