mash: Allow a subset of D-Bus clients to be created in DBusThreadManager

chromeos: Allow DBusThreadManager to create a subset of the D-Bus clients

On mus the ash process only needs to initialize a subset of the available
D-Bus clients. Most clients are only used by chrome, and some of those clients
expect to have only a single user of their chrome-facing API.

* Use DBusClientTypeMask to specify which clients to create
* Keep the existing DBusThreadManager::Initialize() constructor, as it is used
extensively in tests
* Continue to support the --dbus-unstub-clients command line flag, which
requires DBusClientBundle to support a mix of real and fake clients
* Rename "stub" to "fake" in locals and methods

Existing chrome and ash code continues to use DBusThreadManager::Get()->GetFoo()
and will function in both classic ash (with ash running in chrome browser) and
mus ash (with ash in its own process).

BUG=644414
TEST=added to chromeos_unittests

[James Cook, 2016-09-07 18:45:18]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 18:45:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 19:36:52]

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

[commit-bot: I haz the power, 2016-09-07 19:36:53]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-09-07 19:52:51]

Description was changed from

==========
mash: Allow a subset of D-Bus clients to be created in DBusThreadManager

format


rename stub to fake


Add unit tests, partially init in mash


finish partial init with masks


Split out fake and real clients

BUG=
==========

to

==========
chromeos: Allow DBusThreadManager to create a subset of the D-Bus clients

On mus the ash process only needs to initialize a subset of the available
D-Bus clients. Most clients are only used by chrome, and some of those clients
expect to have only a single user of their chrome-facing API.

* Use DBusClientTypeMask to specify which clients to create
* Keep the existing DBusThreadManager::Initialize() constructor, as it is used
extensively in tests
* Continue to support the --dbus-unstub-clients command line flag, which
requires DBusClientBundle to support a mix of real and fake clients
* Rename "stub" to "fake" in locals and methods

Existing chrome and ash code continues to use DBusThreadManager::Get()->GetFoo()
and will function in both classic ash (with ash running in chrome browser) and
mus ash (with ash in its own process).

BUG=644414
TEST=added to chromeos_unittests
==========

[James Cook, 2016-09-07 19:52:56]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 19:53:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-09-07 19:56:06]

jamescook@chromium.org changed reviewers:
+ stevenjb@chromium.org

[James Cook, 2016-09-07 19:56:07]

stevenjb, please take a look.

hashimoto, FYI.

https://codereview.chromium.org/2319783002/diff/20001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.h (right):

https://codereview.chromium.org/2319783002/diff/20001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.h:94: static DBusClientTypeMask
ParseUnstubList(const std::string& unstub_list);
I did not rename the --dbus-unstub-clients flag because I don't know who is
using it and I don't want to break their workflow. I also didn't rename any of
these functions, since I thought it would be useful to have it match the flag.

[Daniel Erat, 2016-09-07 20:31:58]

derat@chromium.org changed reviewers:
+ derat@chromium.org

[Daniel Erat, 2016-09-07 20:31:59]

https://codereview.chromium.org/2319783002/diff/20001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/2319783002/diff/20001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:331: CreateGlobalInstance(fake_clients,
real_clients);
the parameters here are swapped

https://codereview.chromium.org/2319783002/diff/20001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.h (right):

https://codereview.chromium.org/2319783002/diff/20001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.h:150: // the comma-separated
|force_real_clients_list|.
do you mind updating this comment to describe the format of the string?
specifically, what are the identifiers that go between the commas?

[commit-bot: I haz the power, 2016-09-07 20:54:26]

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

[commit-bot: I haz the power, 2016-09-07 20:54:27]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-09-07 20:57:43]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[James Cook, 2016-09-07 20:58:12]

stevenjb / derat, please take another look

hashimoto, still FYI

https://codereview.chromium.org/2319783002/diff/20001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/2319783002/diff/20001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:331: CreateGlobalInstance(fake_clients,
real_clients);
On 2016/09/07 20:31:59, Daniel Erat wrote:
> the parameters here are swapped

Argh. Good catch. Done.

I tried to add a test for this, but it's hard because we don't want to create
real service implementations in tests. :-(

https://codereview.chromium.org/2319783002/diff/20001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.h (right):

https://codereview.chromium.org/2319783002/diff/20001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.h:150: // the comma-separated
|force_real_clients_list|.
On 2016/09/07 20:31:59, Daniel Erat wrote:
> do you mind updating this comment to describe the format of the string?
> specifically, what are the identifiers that go between the commas?

Done.

[commit-bot: I haz the power, 2016-09-07 20:58:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Daniel Erat, 2016-09-07 21:00:10]

thanks! lgtm, but i'll let steven take a look too

[commit-bot: I haz the power, 2016-09-07 21:49:26]

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

[commit-bot: I haz the power, 2016-09-07 21:49:27]

Dry run: This issue passed the CQ dry run.

[hashimoto, 2016-09-08 06:11:43]

hashimoto@chromium.org changed reviewers:
+ hashimoto@chromium.org

[hashimoto, 2016-09-08 06:11:43]

Existing code is written with an assumption that DBusThreadManager never returns
null for getter methods.
After this change, DBusThreadManager may return nullptr for any client getter
methods, and I think it's a drastic API contract change.

To disable SMS code in chrome, how about not initializing NetworkSmsHandler in
chromeos::NetworkHandler::NetworkHandler()?
I think in general it's better to disable the user code than returning null
D-Bus client.

[stevenjb, 2016-09-08 13:50:30]

On 2016/09/08 06:11:43, hashimoto wrote:
> Existing code is written with an assumption that DBusThreadManager never
returns
> null for getter methods.
> After this change, DBusThreadManager may return nullptr for any client getter
> methods, and I think it's a drastic API contract change.
> 
> To disable SMS code in chrome, how about not initializing NetworkSmsHandler in
> chromeos::NetworkHandler::NetworkHandler()?
> I think in general it's better to disable the user code than returning null
> D-Bus client.

I like the explicit initialization and I like how this approach works for
ash/mus/window_manager_application.cc. We are going to have to initialize a
subset there regardless, not just disable SMS, and this seems like a good
approach. I expect there will be other services than SMS that till eventually be
removed from chrome entirely and only used in Ash as we separate the two over
time.

Also, we might already return null before or after initialization, and iirc
Get() already does an assert, so the API change seems reasonable to me. We may
wish to be sure to document that a getter might return null if not initialized
(if we do not already).

[James Cook, 2016-09-08 17:03:28]

On 2016/09/08 13:50:30, stevenjb wrote:
> On 2016/09/08 06:11:43, hashimoto wrote:
> > Existing code is written with an assumption that DBusThreadManager never
> returns
> > null for getter methods.
> > After this change, DBusThreadManager may return nullptr for any client
getter
> > methods, and I think it's a drastic API contract change.
> > 
> > To disable SMS code in chrome, how about not initializing NetworkSmsHandler
in
> > chromeos::NetworkHandler::NetworkHandler()?
> > I think in general it's better to disable the user code than returning null
> > D-Bus client.
> 
> I like the explicit initialization and I like how this approach works for
> ash/mus/window_manager_application.cc. We are going to have to initialize a
> subset there regardless, not just disable SMS, and this seems like a good
> approach. I expect there will be other services than SMS that till eventually
be
> removed from chrome entirely and only used in Ash as we separate the two over
> time.
> 
> Also, we might already return null before or after initialization, and iirc
> Get() already does an assert, so the API change seems reasonable to me. We may
> wish to be sure to document that a getter might return null if not initialized
> (if we do not already).

So I need to do a couple things in mustash:

1) Disable SMS in chrome. In particular, I don't want chrome to respond to D-Bus
signals on the org.freedesktop.ModemManager.Modem.Gsm.SMS interface.

2) Disable a large number of services in ash. For example, I don't want ash to
respond to CrosDisks mount complete signals or cryptohome low-disk-space
signals.

It might work to initialize all the clients in both chrome and in ash, then rely
on the higher level wrappers not to use them (for example, rely on the fact that
no one calls DiskMountManager::Initialize() in ash, so it won't call
CrosDisksClient::SetMountCompletedHandler, so nothing will respond to the mount
completed event). However, it seems wasteful to initialize a bunch of clients
that are never used. It's also hard to me to determine by inspecting the code
that nothing will go wrong if ash receives a signal from a service it doesn't
use. My primary concern is that both chrome and ash will respond to a signal and
confuse the underlying service.

So I would prefer to skip initializing services I don't use.

I also need to make sure that things work properly when running chrome without
mustash. In that case I need to make sure all clients are initialized, and each
one initialized only once.

I can see a few approaches:
1) Do this. Keep all the thread and fake management centralized, but sometimes
return null for getters. This risks crashing if a piece of code accesses an
unexpected service. However, it should be possible to find all callers of
DBusThreadManager::GetFooClient() and ensure they are called in the right
process. Also change ash_unittests to only init a subset of clients, so maybe
tests will find callers returning null.
2) Lazily create clients when methods like
DBusThreadManager::GetCrosDisksClient() are called. This will never return null
and each process only gets the clients it needs. However, we may need to
explicitly initialize some clients at startup -- I'm not sure if there are cases
where a client needs to exist to receive a signal before GetFooClient() is
called.
3) Subclass DBusThreadManager into DBusThreadManagerAsh and
DBusThreadManagerChrome. Only provide getters for the clients expected to be
used by that process. This involves changing ~250 call sites in ash and chrome
to use the right manager. It also is a little tricky when running with ash and
chrome in the same process, as some clients are used by both (e.g.
PowerManagerClient) and we only want one copy of the client.

What do you guys want me to do?

[Daniel Erat, 2016-09-09 00:16:05]

On 2016/09/08 17:03:28, James Cook wrote:
> On 2016/09/08 13:50:30, stevenjb wrote:
> > On 2016/09/08 06:11:43, hashimoto wrote:
> > > Existing code is written with an assumption that DBusThreadManager never
> > returns
> > > null for getter methods.
> > > After this change, DBusThreadManager may return nullptr for any client
> getter
> > > methods, and I think it's a drastic API contract change.
> > > 
> > > To disable SMS code in chrome, how about not initializing
NetworkSmsHandler
> in
> > > chromeos::NetworkHandler::NetworkHandler()?
> > > I think in general it's better to disable the user code than returning
null
> > > D-Bus client.
> > 
> > I like the explicit initialization and I like how this approach works for
> > ash/mus/window_manager_application.cc. We are going to have to initialize a
> > subset there regardless, not just disable SMS, and this seems like a good
> > approach. I expect there will be other services than SMS that till
eventually
> be
> > removed from chrome entirely and only used in Ash as we separate the two
over
> > time.
> > 
> > Also, we might already return null before or after initialization, and iirc
> > Get() already does an assert, so the API change seems reasonable to me. We
may
> > wish to be sure to document that a getter might return null if not
initialized
> > (if we do not already).
> 
> So I need to do a couple things in mustash:
> 
> 1) Disable SMS in chrome. In particular, I don't want chrome to respond to
D-Bus
> signals on the org.freedesktop.ModemManager.Modem.Gsm.SMS interface.
> 
> 2) Disable a large number of services in ash. For example, I don't want ash to
> respond to CrosDisks mount complete signals or cryptohome low-disk-space
> signals.
> 
> It might work to initialize all the clients in both chrome and in ash, then
rely
> on the higher level wrappers not to use them (for example, rely on the fact
that
> no one calls DiskMountManager::Initialize() in ash, so it won't call
> CrosDisksClient::SetMountCompletedHandler, so nothing will respond to the
mount
> completed event). However, it seems wasteful to initialize a bunch of clients
> that are never used. It's also hard to me to determine by inspecting the code
> that nothing will go wrong if ash receives a signal from a service it doesn't
> use. My primary concern is that both chrome and ash will respond to a signal
and
> confuse the underlying service.
> 
> So I would prefer to skip initializing services I don't use.
> 
> I also need to make sure that things work properly when running chrome without
> mustash. In that case I need to make sure all clients are initialized, and
each
> one initialized only once.
> 
> I can see a few approaches:
> 1) Do this. Keep all the thread and fake management centralized, but sometimes
> return null for getters. This risks crashing if a piece of code accesses an
> unexpected service. However, it should be possible to find all callers of
> DBusThreadManager::GetFooClient() and ensure they are called in the right
> process. Also change ash_unittests to only init a subset of clients, so maybe
> tests will find callers returning null.
> 2) Lazily create clients when methods like
> DBusThreadManager::GetCrosDisksClient() are called. This will never return
null
> and each process only gets the clients it needs. However, we may need to
> explicitly initialize some clients at startup -- I'm not sure if there are
cases
> where a client needs to exist to receive a signal before GetFooClient() is
> called.
> 3) Subclass DBusThreadManager into DBusThreadManagerAsh and
> DBusThreadManagerChrome. Only provide getters for the clients expected to be
> used by that process. This involves changing ~250 call sites in ash and chrome
> to use the right manager. It also is a little tricky when running with ash and
> chrome in the same process, as some clients are used by both (e.g.
> PowerManagerClient) and we only want one copy of the client.
> 
> What do you guys want me to do?

my strong preference is for 1), only creating and initializing clients in the
processes where they should be used. yes, we'll crash if we try to dereference a
missing service, but that indicates a bug that we need to fix anyway.

[hashimoto, 2016-09-09 01:50:41]

On 2016/09/09 00:16:05, Daniel Erat wrote:
> On 2016/09/08 17:03:28, James Cook wrote:
> > On 2016/09/08 13:50:30, stevenjb wrote:
> > > On 2016/09/08 06:11:43, hashimoto wrote:
> > > > Existing code is written with an assumption that DBusThreadManager never
> > > returns
> > > > null for getter methods.
> > > > After this change, DBusThreadManager may return nullptr for any client
> > getter
> > > > methods, and I think it's a drastic API contract change.
> > > > 
> > > > To disable SMS code in chrome, how about not initializing
> NetworkSmsHandler
> > in
> > > > chromeos::NetworkHandler::NetworkHandler()?
> > > > I think in general it's better to disable the user code than returning
> null
> > > > D-Bus client.
> > > 
> > > I like the explicit initialization and I like how this approach works for
> > > ash/mus/window_manager_application.cc. We are going to have to initialize
a
> > > subset there regardless, not just disable SMS, and this seems like a good
> > > approach. I expect there will be other services than SMS that till
> eventually
> > be
> > > removed from chrome entirely and only used in Ash as we separate the two
> over
> > > time.
> > > 
> > > Also, we might already return null before or after initialization, and
iirc
> > > Get() already does an assert, so the API change seems reasonable to me. We
> may
> > > wish to be sure to document that a getter might return null if not
> initialized
> > > (if we do not already).
DBusThreadManager::Get() and client getters are different.
DBusThreadManager::Get() may return null only when you call it during browser
startup or shutdown (i.e. in most cases, you don't have to worry about
DBusThreadManager::Get()'s return value).
This change makes client getters return null even when DBusThreadManager::Get()
returns a valid object (i.e. always you have to worry about client getters'
return values).
> > 
> > So I need to do a couple things in mustash:
> > 
> > 1) Disable SMS in chrome. In particular, I don't want chrome to respond to
> D-Bus
> > signals on the org.freedesktop.ModemManager.Modem.Gsm.SMS interface.
> > 
> > 2) Disable a large number of services in ash. For example, I don't want ash
to
> > respond to CrosDisks mount complete signals or cryptohome low-disk-space
> > signals.
> > 
> > It might work to initialize all the clients in both chrome and in ash, then
> rely
> > on the higher level wrappers not to use them (for example, rely on the fact
> that
> > no one calls DiskMountManager::Initialize() in ash, so it won't call
> > CrosDisksClient::SetMountCompletedHandler, so nothing will respond to the
> mount
> > completed event). However, it seems wasteful to initialize a bunch of
clients
> > that are never used. It's also hard to me to determine by inspecting the
code
> > that nothing will go wrong if ash receives a signal from a service it
doesn't
> > use. My primary concern is that both chrome and ash will respond to a signal
> and
> > confuse the underlying service.
> > 
> > So I would prefer to skip initializing services I don't use.
> > 
> > I also need to make sure that things work properly when running chrome
without
> > mustash. In that case I need to make sure all clients are initialized, and
> each
> > one initialized only once.
> > 
> > I can see a few approaches:
> > 1) Do this. Keep all the thread and fake management centralized, but
sometimes
> > return null for getters. This risks crashing if a piece of code accesses an
> > unexpected service. However, it should be possible to find all callers of
> > DBusThreadManager::GetFooClient() and ensure they are called in the right
> > process. Also change ash_unittests to only init a subset of clients, so
maybe
> > tests will find callers returning null.
> > 2) Lazily create clients when methods like
> > DBusThreadManager::GetCrosDisksClient() are called. This will never return
> null
> > and each process only gets the clients it needs. However, we may need to
> > explicitly initialize some clients at startup -- I'm not sure if there are
> cases
> > where a client needs to exist to receive a signal before GetFooClient() is
> > called.
> > 3) Subclass DBusThreadManager into DBusThreadManagerAsh and
> > DBusThreadManagerChrome. Only provide getters for the clients expected to be
> > used by that process. This involves changing ~250 call sites in ash and
chrome
> > to use the right manager. It also is a little tricky when running with ash
and
> > chrome in the same process, as some clients are used by both (e.g.
> > PowerManagerClient) and we only want one copy of the client.
Or 4) Keep using DBusThreadManager in Chrome, and add DBusThreadManagerAsh for
Ash.
This way you don't have to fix all 250 call sites.
You can start by moving the ownership of GsmSMSClient and SMSClient from
DBusThreadManager to DBusThreadManagerAsh,
and you can extract reusable parts from DBusThreadManager to write
DBusThreadManagerAsh.

For clients used by both Chrome and Ash (e.g. PowerManagerClient),
DBusThreadManagerAsh can return a pointer of an object which is actually owned
by DBusThreadManager to avoid duplicated clients when mash is not used.

The benefit of this approach is that we can utilize compilers, linkers, and DEPS
checker during build time to avoid accessing D-Bus clients from a wrong process.
I think it's much better than hitting random SIGSEGV at runtime.

Of course, this approach requires some amount of effort before getting started,
so that might be a reason to prefer 1).
However, I think 4) is good for all developers' productivity for long term.
WDYT?
> > 
> > What do you guys want me to do?
> 
> my strong preference is for 1), only creating and initializing clients in the
> processes where they should be used. yes, we'll crash if we try to dereference
a
> missing service, but that indicates a bug that we need to fix anyway.

[James Cook, 2016-09-09 05:30:59]

On 2016/09/09 01:50:41, hashimoto wrote:
> On 2016/09/09 00:16:05, Daniel Erat wrote:
> > On 2016/09/08 17:03:28, James Cook wrote:
> > > On 2016/09/08 13:50:30, stevenjb wrote:
> > > > On 2016/09/08 06:11:43, hashimoto wrote:
> > > > > Existing code is written with an assumption that DBusThreadManager
never
> > > > returns
> > > > > null for getter methods.
> > > > > After this change, DBusThreadManager may return nullptr for any client
> > > getter
> > > > > methods, and I think it's a drastic API contract change.
> > > > > 
> > > > > To disable SMS code in chrome, how about not initializing
> > NetworkSmsHandler
> > > in
> > > > > chromeos::NetworkHandler::NetworkHandler()?
> > > > > I think in general it's better to disable the user code than returning
> > null
> > > > > D-Bus client.
> > > > 
> > > > I like the explicit initialization and I like how this approach works
for
> > > > ash/mus/window_manager_application.cc. We are going to have to
initialize
> a
> > > > subset there regardless, not just disable SMS, and this seems like a
good
> > > > approach. I expect there will be other services than SMS that till
> > eventually
> > > be
> > > > removed from chrome entirely and only used in Ash as we separate the two
> > over
> > > > time.
> > > > 
> > > > Also, we might already return null before or after initialization, and
> iirc
> > > > Get() already does an assert, so the API change seems reasonable to me.
We
> > may
> > > > wish to be sure to document that a getter might return null if not
> > initialized
> > > > (if we do not already).
> DBusThreadManager::Get() and client getters are different.
> DBusThreadManager::Get() may return null only when you call it during browser
> startup or shutdown (i.e. in most cases, you don't have to worry about
> DBusThreadManager::Get()'s return value).
> This change makes client getters return null even when
DBusThreadManager::Get()
> returns a valid object (i.e. always you have to worry about client getters'
> return values).
> > > 
> > > So I need to do a couple things in mustash:
> > > 
> > > 1) Disable SMS in chrome. In particular, I don't want chrome to respond to
> > D-Bus
> > > signals on the org.freedesktop.ModemManager.Modem.Gsm.SMS interface.
> > > 
> > > 2) Disable a large number of services in ash. For example, I don't want
ash
> to
> > > respond to CrosDisks mount complete signals or cryptohome low-disk-space
> > > signals.
> > > 
> > > It might work to initialize all the clients in both chrome and in ash,
then
> > rely
> > > on the higher level wrappers not to use them (for example, rely on the
fact
> > that
> > > no one calls DiskMountManager::Initialize() in ash, so it won't call
> > > CrosDisksClient::SetMountCompletedHandler, so nothing will respond to the
> > mount
> > > completed event). However, it seems wasteful to initialize a bunch of
> clients
> > > that are never used. It's also hard to me to determine by inspecting the
> code
> > > that nothing will go wrong if ash receives a signal from a service it
> doesn't
> > > use. My primary concern is that both chrome and ash will respond to a
signal
> > and
> > > confuse the underlying service.
> > > 
> > > So I would prefer to skip initializing services I don't use.
> > > 
> > > I also need to make sure that things work properly when running chrome
> without
> > > mustash. In that case I need to make sure all clients are initialized, and
> > each
> > > one initialized only once.
> > > 
> > > I can see a few approaches:
> > > 1) Do this. Keep all the thread and fake management centralized, but
> sometimes
> > > return null for getters. This risks crashing if a piece of code accesses
an
> > > unexpected service. However, it should be possible to find all callers of
> > > DBusThreadManager::GetFooClient() and ensure they are called in the right
> > > process. Also change ash_unittests to only init a subset of clients, so
> maybe
> > > tests will find callers returning null.
> > > 2) Lazily create clients when methods like
> > > DBusThreadManager::GetCrosDisksClient() are called. This will never return
> > null
> > > and each process only gets the clients it needs. However, we may need to
> > > explicitly initialize some clients at startup -- I'm not sure if there are
> > cases
> > > where a client needs to exist to receive a signal before GetFooClient() is
> > > called.
> > > 3) Subclass DBusThreadManager into DBusThreadManagerAsh and
> > > DBusThreadManagerChrome. Only provide getters for the clients expected to
be
> > > used by that process. This involves changing ~250 call sites in ash and
> chrome
> > > to use the right manager. It also is a little tricky when running with ash
> and
> > > chrome in the same process, as some clients are used by both (e.g.
> > > PowerManagerClient) and we only want one copy of the client.
> Or 4) Keep using DBusThreadManager in Chrome, and add DBusThreadManagerAsh for
> Ash.
> This way you don't have to fix all 250 call sites.
> You can start by moving the ownership of GsmSMSClient and SMSClient from
> DBusThreadManager to DBusThreadManagerAsh,
> and you can extract reusable parts from DBusThreadManager to write
> DBusThreadManagerAsh.
> 
> For clients used by both Chrome and Ash (e.g. PowerManagerClient),
> DBusThreadManagerAsh can return a pointer of an object which is actually owned
> by DBusThreadManager to avoid duplicated clients when mash is not used.
> 
> The benefit of this approach is that we can utilize compilers, linkers, and
DEPS
> checker during build time to avoid accessing D-Bus clients from a wrong
process.
> I think it's much better than hitting random SIGSEGV at runtime.
> 
> Of course, this approach requires some amount of effort before getting
started,
> so that might be a reason to prefer 1).
> However, I think 4) is good for all developers' productivity for long term.
> WDYT?
> > > 
> > > What do you guys want me to do?
> > 
> > my strong preference is for 1), only creating and initializing clients in
the
> > processes where they should be used. yes, we'll crash if we try to
dereference
> a
> > missing service, but that indicates a bug that we need to fix anyway.

Chatted with hashimoto@. I plan to do something like this:

A. Introduce DBusThreadManagerAsh. It lives in //ash. It provides GetFooClient()
for all clients used in //ash. Some, like SMS, will be ash-only. Others, like
PowerManagerClient, will also be used in //chrome.
B. DBusThreadManagerAsh::GetFooClient() will never return null, because it will
not provide getters for clients only used in //chrome.
C. DBusThreadManagerAsh is not a subclass of DBusThreadManager, but a sibling.
Shared code (like for thread setup) will go into a DBusThreadHelper that can be
used by both.
D. DBusThreadManager should really be called DBusThreadManagerChrome, but there
are too many call sites to rename, so we won't do that in this CL.
E. DBusThreadManagerChrome should live in //chrome/browser/chromeos, but it
can't due to usage in //chromeos/network. We could fix that by injecting the
needed clients into ctors. I probably won't do that in this CL.
F. I might continue to use DBusClientBundle, like in this CL, with masks to
select the real and fake clients to initialize. This allows us to have a list of
all dbus clients in one place, and all the real vs. fake init code in one place.

Thanks for the advice. I'll start looking at this tomorrow.

[hashimoto, 2016-09-09 07:54:11]

On 2016/09/09 05:30:59, James Cook wrote:
> On 2016/09/09 01:50:41, hashimoto wrote:
> > On 2016/09/09 00:16:05, Daniel Erat wrote:
> > > On 2016/09/08 17:03:28, James Cook wrote:
> > > > On 2016/09/08 13:50:30, stevenjb wrote:
> > > > > On 2016/09/08 06:11:43, hashimoto wrote:
> > > > > > Existing code is written with an assumption that DBusThreadManager
> never
> > > > > returns
> > > > > > null for getter methods.
> > > > > > After this change, DBusThreadManager may return nullptr for any
client
> > > > getter
> > > > > > methods, and I think it's a drastic API contract change.
> > > > > > 
> > > > > > To disable SMS code in chrome, how about not initializing
> > > NetworkSmsHandler
> > > > in
> > > > > > chromeos::NetworkHandler::NetworkHandler()?
> > > > > > I think in general it's better to disable the user code than
returning
> > > null
> > > > > > D-Bus client.
> > > > > 
> > > > > I like the explicit initialization and I like how this approach works
> for
> > > > > ash/mus/window_manager_application.cc. We are going to have to
> initialize
> > a
> > > > > subset there regardless, not just disable SMS, and this seems like a
> good
> > > > > approach. I expect there will be other services than SMS that till
> > > eventually
> > > > be
> > > > > removed from chrome entirely and only used in Ash as we separate the
two
> > > over
> > > > > time.
> > > > > 
> > > > > Also, we might already return null before or after initialization, and
> > iirc
> > > > > Get() already does an assert, so the API change seems reasonable to
me.
> We
> > > may
> > > > > wish to be sure to document that a getter might return null if not
> > > initialized
> > > > > (if we do not already).
> > DBusThreadManager::Get() and client getters are different.
> > DBusThreadManager::Get() may return null only when you call it during
browser
> > startup or shutdown (i.e. in most cases, you don't have to worry about
> > DBusThreadManager::Get()'s return value).
> > This change makes client getters return null even when
> DBusThreadManager::Get()
> > returns a valid object (i.e. always you have to worry about client getters'
> > return values).
> > > > 
> > > > So I need to do a couple things in mustash:
> > > > 
> > > > 1) Disable SMS in chrome. In particular, I don't want chrome to respond
to
> > > D-Bus
> > > > signals on the org.freedesktop.ModemManager.Modem.Gsm.SMS interface.
> > > > 
> > > > 2) Disable a large number of services in ash. For example, I don't want
> ash
> > to
> > > > respond to CrosDisks mount complete signals or cryptohome low-disk-space
> > > > signals.
> > > > 
> > > > It might work to initialize all the clients in both chrome and in ash,
> then
> > > rely
> > > > on the higher level wrappers not to use them (for example, rely on the
> fact
> > > that
> > > > no one calls DiskMountManager::Initialize() in ash, so it won't call
> > > > CrosDisksClient::SetMountCompletedHandler, so nothing will respond to
the
> > > mount
> > > > completed event). However, it seems wasteful to initialize a bunch of
> > clients
> > > > that are never used. It's also hard to me to determine by inspecting the
> > code
> > > > that nothing will go wrong if ash receives a signal from a service it
> > doesn't
> > > > use. My primary concern is that both chrome and ash will respond to a
> signal
> > > and
> > > > confuse the underlying service.
> > > > 
> > > > So I would prefer to skip initializing services I don't use.
> > > > 
> > > > I also need to make sure that things work properly when running chrome
> > without
> > > > mustash. In that case I need to make sure all clients are initialized,
and
> > > each
> > > > one initialized only once.
> > > > 
> > > > I can see a few approaches:
> > > > 1) Do this. Keep all the thread and fake management centralized, but
> > sometimes
> > > > return null for getters. This risks crashing if a piece of code accesses
> an
> > > > unexpected service. However, it should be possible to find all callers
of
> > > > DBusThreadManager::GetFooClient() and ensure they are called in the
right
> > > > process. Also change ash_unittests to only init a subset of clients, so
> > maybe
> > > > tests will find callers returning null.
> > > > 2) Lazily create clients when methods like
> > > > DBusThreadManager::GetCrosDisksClient() are called. This will never
return
> > > null
> > > > and each process only gets the clients it needs. However, we may need to
> > > > explicitly initialize some clients at startup -- I'm not sure if there
are
> > > cases
> > > > where a client needs to exist to receive a signal before GetFooClient()
is
> > > > called.
> > > > 3) Subclass DBusThreadManager into DBusThreadManagerAsh and
> > > > DBusThreadManagerChrome. Only provide getters for the clients expected
to
> be
> > > > used by that process. This involves changing ~250 call sites in ash and
> > chrome
> > > > to use the right manager. It also is a little tricky when running with
ash
> > and
> > > > chrome in the same process, as some clients are used by both (e.g.
> > > > PowerManagerClient) and we only want one copy of the client.
> > Or 4) Keep using DBusThreadManager in Chrome, and add DBusThreadManagerAsh
for
> > Ash.
> > This way you don't have to fix all 250 call sites.
> > You can start by moving the ownership of GsmSMSClient and SMSClient from
> > DBusThreadManager to DBusThreadManagerAsh,
> > and you can extract reusable parts from DBusThreadManager to write
> > DBusThreadManagerAsh.
> > 
> > For clients used by both Chrome and Ash (e.g. PowerManagerClient),
> > DBusThreadManagerAsh can return a pointer of an object which is actually
owned
> > by DBusThreadManager to avoid duplicated clients when mash is not used.
> > 
> > The benefit of this approach is that we can utilize compilers, linkers, and
> DEPS
> > checker during build time to avoid accessing D-Bus clients from a wrong
> process.
> > I think it's much better than hitting random SIGSEGV at runtime.
> > 
> > Of course, this approach requires some amount of effort before getting
> started,
> > so that might be a reason to prefer 1).
> > However, I think 4) is good for all developers' productivity for long term.
> > WDYT?
> > > > 
> > > > What do you guys want me to do?
> > > 
> > > my strong preference is for 1), only creating and initializing clients in
> the
> > > processes where they should be used. yes, we'll crash if we try to
> dereference
> > a
> > > missing service, but that indicates a bug that we need to fix anyway.
> 
> Chatted with hashimoto@. I plan to do something like this:
> 
> A. Introduce DBusThreadManagerAsh. It lives in //ash. It provides
GetFooClient()
> for all clients used in //ash. Some, like SMS, will be ash-only. Others, like
> PowerManagerClient, will also be used in //chrome.
> B. DBusThreadManagerAsh::GetFooClient() will never return null, because it
will
> not provide getters for clients only used in //chrome.
> C. DBusThreadManagerAsh is not a subclass of DBusThreadManager, but a sibling.
> Shared code (like for thread setup) will go into a DBusThreadHelper that can
be
> used by both.
Probably we need to have 3 classes, DBusThreadManager (which owns client classes
needed by both ash and chrome), DBusThreadManagerAsh (owns clients needed only
by ash), and DBusThreadManagerChrome (owns clients needed only by chrome).
This way DBusThreadManagerChrome can be put in //chrome/browser/chromeos and
DBusThreadManagerAsh can be in //ash while keeping most DBusThreadManager user
code unchanged.
> D. DBusThreadManager should really be called DBusThreadManagerChrome, but
there
> are too many call sites to rename, so we won't do that in this CL.
> E. DBusThreadManagerChrome should live in //chrome/browser/chromeos, but it
> can't due to usage in //chromeos/network. We could fix that by injecting the
> needed clients into ctors. I probably won't do that in this CL.
> F. I might continue to use DBusClientBundle, like in this CL, with masks to
> select the real and fake clients to initialize. This allows us to have a list
of
> all dbus clients in one place, and all the real vs. fake init code in one
place.
> 
> Thanks for the advice. I'll start looking at this tomorrow.

[James Cook, 2016-09-09 15:55:26]

On 2016/09/09 07:54:11, hashimoto wrote:
> On 2016/09/09 05:30:59, James Cook wrote:
> > On 2016/09/09 01:50:41, hashimoto wrote:
> > > On 2016/09/09 00:16:05, Daniel Erat wrote:
> > > > On 2016/09/08 17:03:28, James Cook wrote:
> > > > > On 2016/09/08 13:50:30, stevenjb wrote:
> > > > > > On 2016/09/08 06:11:43, hashimoto wrote:
> > > > > > > Existing code is written with an assumption that DBusThreadManager
> > never
> > > > > > returns
> > > > > > > null for getter methods.
> > > > > > > After this change, DBusThreadManager may return nullptr for any
> client
> > > > > getter
> > > > > > > methods, and I think it's a drastic API contract change.
> > > > > > > 
> > > > > > > To disable SMS code in chrome, how about not initializing
> > > > NetworkSmsHandler
> > > > > in
> > > > > > > chromeos::NetworkHandler::NetworkHandler()?
> > > > > > > I think in general it's better to disable the user code than
> returning
> > > > null
> > > > > > > D-Bus client.
> > > > > > 
> > > > > > I like the explicit initialization and I like how this approach
works
> > for
> > > > > > ash/mus/window_manager_application.cc. We are going to have to
> > initialize
> > > a
> > > > > > subset there regardless, not just disable SMS, and this seems like a
> > good
> > > > > > approach. I expect there will be other services than SMS that till
> > > > eventually
> > > > > be
> > > > > > removed from chrome entirely and only used in Ash as we separate the
> two
> > > > over
> > > > > > time.
> > > > > > 
> > > > > > Also, we might already return null before or after initialization,
and
> > > iirc
> > > > > > Get() already does an assert, so the API change seems reasonable to
> me.
> > We
> > > > may
> > > > > > wish to be sure to document that a getter might return null if not
> > > > initialized
> > > > > > (if we do not already).
> > > DBusThreadManager::Get() and client getters are different.
> > > DBusThreadManager::Get() may return null only when you call it during
> browser
> > > startup or shutdown (i.e. in most cases, you don't have to worry about
> > > DBusThreadManager::Get()'s return value).
> > > This change makes client getters return null even when
> > DBusThreadManager::Get()
> > > returns a valid object (i.e. always you have to worry about client
getters'
> > > return values).
> > > > > 
> > > > > So I need to do a couple things in mustash:
> > > > > 
> > > > > 1) Disable SMS in chrome. In particular, I don't want chrome to
respond
> to
> > > > D-Bus
> > > > > signals on the org.freedesktop.ModemManager.Modem.Gsm.SMS interface.
> > > > > 
> > > > > 2) Disable a large number of services in ash. For example, I don't
want
> > ash
> > > to
> > > > > respond to CrosDisks mount complete signals or cryptohome
low-disk-space
> > > > > signals.
> > > > > 
> > > > > It might work to initialize all the clients in both chrome and in ash,
> > then
> > > > rely
> > > > > on the higher level wrappers not to use them (for example, rely on the
> > fact
> > > > that
> > > > > no one calls DiskMountManager::Initialize() in ash, so it won't call
> > > > > CrosDisksClient::SetMountCompletedHandler, so nothing will respond to
> the
> > > > mount
> > > > > completed event). However, it seems wasteful to initialize a bunch of
> > > clients
> > > > > that are never used. It's also hard to me to determine by inspecting
the
> > > code
> > > > > that nothing will go wrong if ash receives a signal from a service it
> > > doesn't
> > > > > use. My primary concern is that both chrome and ash will respond to a
> > signal
> > > > and
> > > > > confuse the underlying service.
> > > > > 
> > > > > So I would prefer to skip initializing services I don't use.
> > > > > 
> > > > > I also need to make sure that things work properly when running chrome
> > > without
> > > > > mustash. In that case I need to make sure all clients are initialized,
> and
> > > > each
> > > > > one initialized only once.
> > > > > 
> > > > > I can see a few approaches:
> > > > > 1) Do this. Keep all the thread and fake management centralized, but
> > > sometimes
> > > > > return null for getters. This risks crashing if a piece of code
accesses
> > an
> > > > > unexpected service. However, it should be possible to find all callers
> of
> > > > > DBusThreadManager::GetFooClient() and ensure they are called in the
> right
> > > > > process. Also change ash_unittests to only init a subset of clients,
so
> > > maybe
> > > > > tests will find callers returning null.
> > > > > 2) Lazily create clients when methods like
> > > > > DBusThreadManager::GetCrosDisksClient() are called. This will never
> return
> > > > null
> > > > > and each process only gets the clients it needs. However, we may need
to
> > > > > explicitly initialize some clients at startup -- I'm not sure if there
> are
> > > > cases
> > > > > where a client needs to exist to receive a signal before
GetFooClient()
> is
> > > > > called.
> > > > > 3) Subclass DBusThreadManager into DBusThreadManagerAsh and
> > > > > DBusThreadManagerChrome. Only provide getters for the clients expected
> to
> > be
> > > > > used by that process. This involves changing ~250 call sites in ash
and
> > > chrome
> > > > > to use the right manager. It also is a little tricky when running with
> ash
> > > and
> > > > > chrome in the same process, as some clients are used by both (e.g.
> > > > > PowerManagerClient) and we only want one copy of the client.
> > > Or 4) Keep using DBusThreadManager in Chrome, and add DBusThreadManagerAsh
> for
> > > Ash.
> > > This way you don't have to fix all 250 call sites.
> > > You can start by moving the ownership of GsmSMSClient and SMSClient from
> > > DBusThreadManager to DBusThreadManagerAsh,
> > > and you can extract reusable parts from DBusThreadManager to write
> > > DBusThreadManagerAsh.
> > > 
> > > For clients used by both Chrome and Ash (e.g. PowerManagerClient),
> > > DBusThreadManagerAsh can return a pointer of an object which is actually
> owned
> > > by DBusThreadManager to avoid duplicated clients when mash is not used.
> > > 
> > > The benefit of this approach is that we can utilize compilers, linkers,
and
> > DEPS
> > > checker during build time to avoid accessing D-Bus clients from a wrong
> > process.
> > > I think it's much better than hitting random SIGSEGV at runtime.
> > > 
> > > Of course, this approach requires some amount of effort before getting
> > started,
> > > so that might be a reason to prefer 1).
> > > However, I think 4) is good for all developers' productivity for long
term.
> > > WDYT?
> > > > > 
> > > > > What do you guys want me to do?
> > > > 
> > > > my strong preference is for 1), only creating and initializing clients
in
> > the
> > > > processes where they should be used. yes, we'll crash if we try to
> > dereference
> > > a
> > > > missing service, but that indicates a bug that we need to fix anyway.
> > 
> > Chatted with hashimoto@. I plan to do something like this:
> > 
> > A. Introduce DBusThreadManagerAsh. It lives in //ash. It provides
> GetFooClient()
> > for all clients used in //ash. Some, like SMS, will be ash-only. Others,
like
> > PowerManagerClient, will also be used in //chrome.
> > B. DBusThreadManagerAsh::GetFooClient() will never return null, because it
> will
> > not provide getters for clients only used in //chrome.
> > C. DBusThreadManagerAsh is not a subclass of DBusThreadManager, but a
sibling.
> > Shared code (like for thread setup) will go into a DBusThreadHelper that can
> be
> > used by both.
> Probably we need to have 3 classes, DBusThreadManager (which owns client
classes
> needed by both ash and chrome), DBusThreadManagerAsh (owns clients needed only
> by ash), and DBusThreadManagerChrome (owns clients needed only by chrome).
> This way DBusThreadManagerChrome can be put in //chrome/browser/chromeos and
> DBusThreadManagerAsh can be in //ash while keeping most DBusThreadManager user
> code unchanged.
> > D. DBusThreadManager should really be called DBusThreadManagerChrome, but
> there
> > are too many call sites to rename, so we won't do that in this CL.
> > E. DBusThreadManagerChrome should live in //chrome/browser/chromeos, but it
> > can't due to usage in //chromeos/network. We could fix that by injecting the
> > needed clients into ctors. I probably won't do that in this CL.
> > F. I might continue to use DBusClientBundle, like in this CL, with masks to
> > select the real and fake clients to initialize. This allows us to have a
list
> of
> > all dbus clients in one place, and all the real vs. fake init code in one
> place.
> > 
> > Thanks for the advice. I'll start looking at this tomorrow.

Hmm. This is going to be ugly. Many of the chrome-only clients are accessed by
handler code that lives in //chromeos, for example in //chromeos/cryptohome or
//chromeos/network.

The ash-only SMS code is accessed in //chromeos/network as well.

This means that either:
1) DBusThreadManagerAsh and DBusThreadManagerChrome both live in
//chromeos/dbus, which is easy to understand but a layering violation.
2) All the "handler" code in //chromeos/cryptohome and similar places moves into
//chrome/browser/chromeos. Shared //chromeos/network code takes shill client
classes by injection in their constructors.
3) Every handler class takes its clients by injection

I'm going to start refactoring things now and see how it looks. If anyone has
suggestions please ping me.

[James Cook, 2016-09-09 20:21:54]

I've uploaded a WIP patch, but it's pretty ugly.

Let's move discussion to the bug (http://crbug.com/644414) since I may have to
abandon this patch.