Adds a way to associate key/value pairs with the environment

Adds a way to associate key/value pairs with the environment

This is to fix a deadlock during shutdown. Specifically
WatcherThreadManager was a LazyInstance. This means it would be
shutdown by LazyInstance, which holds a lock. If the thread attempted
to grab the lazyinstance lock (which it does), then we deadlock.

Making the WatcherTheadManager owned by Environment makes for saner
lifetime management and easier shutdown in tests.

BUG=none
TEST=none
R=darin@chromium.org

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

[darin (slow to review), 2014-03-31 23:21:55]

So, the underlying issue here is that you want to make sure the
WatcherThreadManager instance gets deleted earlier during shutdown? Waiting for
AtExitManager to delete it was too late?

https://codereview.chromium.org/218583009/diff/20001/content/app/mojo/mojo_in...
File content/app/mojo/mojo_init.cc (right):

https://codereview.chromium.org/218583009/diff/20001/content/app/mojo/mojo_in...
content/app/mojo/mojo_init.cc:22: // instance of the ServiceManager.
nit: "instance of" -> "Instantiate" ?

https://codereview.chromium.org/218583009/diff/20001/content/app/mojo/mojo_in...
content/app/mojo/mojo_init.cc:24: mojo::embedder::Init();
Maybe we should initialize mojo core before initializing services that use it?
ServiceManager initialization should probably come second.

https://codereview.chromium.org/218583009/diff/20001/content/app/mojo/mojo_in...
content/app/mojo/mojo_init.cc:28: delete environment;
it is not clear why there is a benefit to storing EnvironmentData on
Environment. since EnvironmentData is reached via
EnvironmentData::GetInstance(), it seems like it doesn't need to have anything
to do with the lifetime of Environment. You could just manually create and
destroy EnvironmentData here from InitializeMojo and ShutdownMojo.

https://codereview.chromium.org/218583009/diff/20001/mojo/common/handle_watch...
File mojo/common/handle_watcher.cc (right):

https://codereview.chromium.org/218583009/diff/20001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:197: WatcherThreadManagerData* data =
static_cast<WatcherThreadManagerData*>(
it looks like you have to worry about multiple threads racing to call
WatcherThreadManager::GetInstance() here, and this code does not protect against
that case.

https://codereview.chromium.org/218583009/diff/20001/mojo/public/cpp/environm...
File mojo/public/cpp/environment/environment.h (right):

https://codereview.chromium.org/218583009/diff/20001/mojo/public/cpp/environm...
mojo/public/cpp/environment/environment.h:15: class Impl {
another way to do this is to just forward declare "class Impl;". you can put
that in the private section. then, in environment.cc you can define the Impl
class.

https://codereview.chromium.org/218583009/diff/20001/mojo/public/cpp/environm...
mojo/public/cpp/environment/environment.h:26: Impl* impl_;
you should probably at least set this to null in the standalone environment
impl, right? i'm referring to mojo/public/cpp/environment/lib/environment.cc

[darin (slow to review), 2014-03-31 23:23:33]

On 2014/03/31 23:21:55, darin wrote:
> So, the underlying issue here is that you want to make sure the
> WatcherThreadManager instance gets deleted earlier during shutdown? Waiting
for
> AtExitManager to delete it was too late?

I should have read your CL description more carefully.


>
https://codereview.chromium.org/218583009/diff/20001/content/app/mojo/mojo_in...
> content/app/mojo/mojo_init.cc:28: delete environment;
> it is not clear why there is a benefit to storing EnvironmentData on
> Environment. since EnvironmentData is reached via
> EnvironmentData::GetInstance(), it seems like it doesn't need to have anything
> to do with the lifetime of Environment. You could just manually create and
> destroy EnvironmentData here from InitializeMojo and ShutdownMojo.

Ah, in the CL description you wrote:
"Making the WatcherTheadManager owned by Environment makes for saner
lifetime management and easier shutdown in tests."

I see. That makes sense.

[darin (slow to review), 2014-03-31 23:24:46]

https://codereview.chromium.org/218583009/diff/20001/mojo/public/cpp/environm...
File mojo/public/cpp/environment/environment.h (right):

https://codereview.chromium.org/218583009/diff/20001/mojo/public/cpp/environm...
mojo/public/cpp/environment/environment.h:15: class Impl {
I might use a term other than Impl here since it might be confusing since we
also have a library named mojo_environment_chromium_impl. Maybe just "Data"
would do?

[sky, 2014-03-31 23:54:39]

https://codereview.chromium.org/218583009/diff/20001/content/app/mojo/mojo_in...
File content/app/mojo/mojo_init.cc (right):

https://codereview.chromium.org/218583009/diff/20001/content/app/mojo/mojo_in...
content/app/mojo/mojo_init.cc:22: // instance of the ServiceManager.
On 2014/03/31 23:21:55, darin wrote:
> nit: "instance of" -> "Instantiate" ?

This was part of a comment on 18 (old). Makes no sense here. Nuked.

https://codereview.chromium.org/218583009/diff/20001/content/app/mojo/mojo_in...
content/app/mojo/mojo_init.cc:24: mojo::embedder::Init();
On 2014/03/31 23:21:55, darin wrote:
> Maybe we should initialize mojo core before initializing services that use it?
> ServiceManager initialization should probably come second.

I copied this code. Dave says order shouldn't matter, so I changed it.

https://codereview.chromium.org/218583009/diff/20001/mojo/common/handle_watch...
File mojo/common/handle_watcher.cc (right):

https://codereview.chromium.org/218583009/diff/20001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:197: WatcherThreadManagerData* data =
static_cast<WatcherThreadManagerData*>(
On 2014/03/31 23:21:55, darin wrote:
> it looks like you have to worry about multiple threads racing to call
> WatcherThreadManager::GetInstance() here, and this code does not protect
against
> that case.

Done.

https://codereview.chromium.org/218583009/diff/20001/mojo/public/cpp/environm...
File mojo/public/cpp/environment/environment.h (right):

https://codereview.chromium.org/218583009/diff/20001/mojo/public/cpp/environm...
mojo/public/cpp/environment/environment.h:15: class Impl {
On 2014/03/31 23:24:47, darin wrote:
> I might use a term other than Impl here since it might be confusing since we
> also have a library named mojo_environment_chromium_impl. Maybe just "Data"
> would do?

Done.

https://codereview.chromium.org/218583009/diff/20001/mojo/public/cpp/environm...
mojo/public/cpp/environment/environment.h:15: class Impl {
On 2014/03/31 23:21:55, darin wrote:
> another way to do this is to just forward declare "class Impl;". you can put
> that in the private section. then, in environment.cc you can define the Impl
> class.

Done.

https://codereview.chromium.org/218583009/diff/20001/mojo/public/cpp/environm...
mojo/public/cpp/environment/environment.h:26: Impl* impl_;
On 2014/03/31 23:21:55, darin wrote:
> you should probably at least set this to null in the standalone environment
> impl, right? i'm referring to mojo/public/cpp/environment/lib/environment.cc

Done.

[darin (slow to review), 2014-04-01 00:14:18]

LGTM

[sky, 2014-04-01 00:23:31]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-04-01 00:25:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/218583009/40001

[commit-bot: I haz the power, 2014-04-01 00:57:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 00:57:36]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[sky, 2014-04-01 03:25:00]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-04-01 03:25:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/218583009/40001

[commit-bot: I haz the power, 2014-04-01 04:27:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 04:27:06]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[sky, 2014-04-01 15:35:36]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-04-01 15:36:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/218583009/60001

[commit-bot: I haz the power, 2014-04-01 16:41:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 16:41:49]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[sky, 2014-04-01 18:21:48]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-04-01 18:23:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/218583009/80001

[commit-bot: I haz the power, 2014-04-01 18:41:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 18:41:06]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[sky, 2014-04-01 20:02:57]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-04-01 20:03:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/218583009/80001

[commit-bot: I haz the power, 2014-04-01 20:53:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 20:53:07]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[sky, 2014-04-01 20:55:33]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-04-01 20:55:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/218583009/80001

[commit-bot: I haz the power, 2014-04-01 22:52:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/218583009/80001

[commit-bot: I haz the power, 2014-04-02 02:04:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/218583009/80001

[commit-bot: I haz the power, 2014-04-02 03:40:06]

Change committed as 261045