dbus: Make Bus::GetManagedObjects skip unavailable services.

dbus: Make ObjectManager get objects after service is ready.

Make dbus::ObjectManager automatically fetch managed objects
after the remote service becomes available. Also remove
Bus::GetManagedObjects(), which produces a large amount of
log spam due to some services not yet being ready. (It was
called by chromeos::DBusThreadManager and
bluez::BluezDBusManager at startup.)

BUG=636554
Committed: https://crrev.com/2ef7d0eda3fe8cb8a73c3ce977b8f1768cd32ece
Cr-Commit-Position: refs/heads/master@{#417405}

[Daniel Erat, 2016-08-11 22:51:56]

derat@chromium.org changed reviewers:
+ hashimoto@chromium.org, satorux@chromium.org, stevenjb@chromium.org

[Daniel Erat, 2016-08-11 22:51:56]

i still need to write tests for this, but i wanted to get your thoughts on this
approach. this looks like it's responsible for fourteen long lines of log spam
at boot. :-)

ObjectManager already calls ObjectProxy::SetNameOwnerChangedCallback() and
automatically invokes GetManagedObjects() when a new owner shows up, so another
approach might be to just make ObjectManager's c'tor call
ObjectProxy::WaitForServiceToBeAvailable() and use that to automatically fetch
the objects if the service is initially available.

then we could presumably just remove the Bus::GetManagedObjects() call in
DBusThreadManager::InitializeClients().

[Daniel Erat, 2016-08-11 22:52:01]

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

[commit-bot: I haz the power, 2016-08-11 22:53:04]

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-08-12 00:22:20]

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

[commit-bot: I haz the power, 2016-08-12 00:22:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hashimoto, 2016-08-15 07:23:22]

https://codereview.chromium.org/2239123002/diff/1/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

https://codereview.chromium.org/2239123002/diff/1/dbus/object_proxy.cc#newcod...
dbus/object_proxy.cc:212: return !service_name_owner_.empty();
This member should be accessed only on the D-Bus thread.

[hashimoto, 2016-08-15 07:24:56]

On 2016/08/11 22:51:56, Daniel Erat (OOO 8-12 to 8-22) wrote:
> i still need to write tests for this, but i wanted to get your thoughts on
this
> approach. this looks like it's responsible for fourteen long lines of log spam
> at boot. :-)
> 
> ObjectManager already calls ObjectProxy::SetNameOwnerChangedCallback() and
> automatically invokes GetManagedObjects() when a new owner shows up, so
another
> approach might be to just make ObjectManager's c'tor call
> ObjectProxy::WaitForServiceToBeAvailable() and use that to automatically fetch
> the objects if the service is initially available.
> 
> then we could presumably just remove the Bus::GetManagedObjects() call in
> DBusThreadManager::InitializeClients().
This approach sounds better.
Relying on both SetNameOwnerChangedCallback() and Bus::GetManagedObjects() looks
fragile to me.

[Daniel Erat, 2016-08-31 16:45:53]

Description was changed from

==========
dbus: Make Bus::GetManagedObjects skip unavailable services.

Expose a service's current availability via a new
ObjectProxy::ServiceIsAvailable() method, and make
Bus::GetManagedObjects() use it to only update objects
backed by usable proxies.

This greatly cuts down on log spam when Chrome OS boots --
chromeos::DBusThreadManager() calls Bus::GetManagedObjects()
before many D-Bus services have started.

BUG=636554
==========

to

==========
dbus: Make ObjectManager get objects after service is ready.

Make dbus::ObjectManager automatically fetch managed objects
after the remote service becomes available. Also remove
chromeos::DBusThreadManager's call to
Bus::GetManagedObjects(), which produces a large amount of
log spam due to some services not yet being ready.

BUG=636554
==========

[Daniel Erat, 2016-08-31 16:46:22]

thanks for the comment! i've updated the approach (but haven't yet added tests).
does this look better to you?

[Daniel Erat, 2016-08-31 16:46:33]

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

[commit-bot: I haz the power, 2016-08-31 16:47:06]

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-08-31 16:58:41]

Description was changed from

==========
dbus: Make ObjectManager get objects after service is ready.

Make dbus::ObjectManager automatically fetch managed objects
after the remote service becomes available. Also remove
chromeos::DBusThreadManager's call to
Bus::GetManagedObjects(), which produces a large amount of
log spam due to some services not yet being ready.

BUG=636554
==========

to

==========
dbus: Make ObjectManager get objects after service is ready.

Make dbus::ObjectManager automatically fetch managed objects
after the remote service becomes available. Also remove
Bus::GetManagedObjects(), which produces a large amount of
log spam due to some services not yet being ready. (It was
called by chromeos::DBusThreadManager and
bluez::BluezDBusManager at startup.)

BUG=636554
==========

[Daniel Erat, 2016-08-31 16:58:48]

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

[commit-bot: I haz the power, 2016-08-31 16:59:14]

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-08-31 18:25:38]

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

[commit-bot: I haz the power, 2016-08-31 18:25:39]

Dry run: This issue passed the CQ dry run.

[hashimoto, 2016-09-01 03:26:08]

Thanks, this looks promising.

https://codereview.chromium.org/2239123002/diff/40001/dbus/object_manager.cc
File dbus/object_manager.cc (right):

https://codereview.chromium.org/2239123002/diff/40001/dbus/object_manager.cc#...
dbus/object_manager.cc:215: GetManagedObjects();
How about calling WaitForServiceToBeAvailable() here instead of in the ctor so
that we don't need to manage signals_are_connected_ and
service_initially_available_?
Are SetupMatchRuleAndFilter() and WaitForServiceToBeAvailable() too slow to
perform them sequentially?

[Daniel Erat, 2016-09-01 04:15:55]

https://codereview.chromium.org/2239123002/diff/40001/dbus/object_manager.cc
File dbus/object_manager.cc (right):

https://codereview.chromium.org/2239123002/diff/40001/dbus/object_manager.cc#...
dbus/object_manager.cc:215: GetManagedObjects();
On 2016/09/01 03:26:07, hashimoto wrote:
> How about calling WaitForServiceToBeAvailable() here instead of in the ctor so
> that we don't need to manage signals_are_connected_ and
> service_initially_available_?
> Are SetupMatchRuleAndFilter() and WaitForServiceToBeAvailable() too slow to
> perform them sequentially?

thanks, that seems like a good suggestion! and no, i'd expect that the delay
wouldn't cause any issues. i'll ping you when i've uploaded a new version.

[Daniel Erat, 2016-09-01 17:06:15]

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

[Daniel Erat, 2016-09-01 17:06:25]

https://codereview.chromium.org/2239123002/diff/60001/dbus/object_manager.h
File dbus/object_manager.h (left):

https://codereview.chromium.org/2239123002/diff/60001/dbus/object_manager.h#o...
dbus/object_manager.h:243: void InitializeObjects();
i moved this into OnSetupMatchRuleAndFilterComplete since the flow through these
methods is linear.

[commit-bot: I haz the power, 2016-09-01 17:06:44]

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-01 18:19:45]

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

[commit-bot: I haz the power, 2016-09-01 18:19:46]

Dry run: This issue passed the CQ dry run.

[hashimoto, 2016-09-02 08:33:07]

Sorry for not noticing this before.
With this code, GetManagedObjects() will get called twice by
OnServiceInitiallyAvailable and NameOwnerChanged when the service becomes
available, and it'll just waste CPU and other resources.

But the good news is that SetupMatchRuleAndFilter sets service_name_owner_. So
if service_name_owner_ is not empty, that means the service is available and
it's OK to call GetManagedObjects in OnSetupMatchRuleAndFilterComplete without
using WaitForServiceToBeAvailable.

BTW this class is badly written in terms of thread safety (crbug.com/643512). I
will fix it after you land this change.

https://codereview.chromium.org/2239123002/diff/60001/dbus/object_manager.cc
File dbus/object_manager.cc (right):

https://codereview.chromium.org/2239123002/diff/60001/dbus/object_manager.cc#...
dbus/object_manager.cc:242: weak_ptr_factory_.GetWeakPtr()));
You can just do this here:

if (!service_name_owner_.empty())
  GetManagedObjects();

If service_name_owner_ is empty, GetManagedObjects() will get called by
ObjectManager::NameOwnerChanged when the service becomes available.

[Daniel Erat, 2016-09-02 14:39:27]

https://codereview.chromium.org/2239123002/diff/60001/dbus/object_manager.cc
File dbus/object_manager.cc (right):

https://codereview.chromium.org/2239123002/diff/60001/dbus/object_manager.cc#...
dbus/object_manager.cc:242: weak_ptr_factory_.GetWeakPtr()));
On 2016/09/02 08:33:06, hashimoto wrote:
> You can just do this here:
> 
> if (!service_name_owner_.empty())
>   GetManagedObjects();
> 
> If service_name_owner_ is empty, GetManagedObjects() will get called by
> ObjectManager::NameOwnerChanged when the service becomes available.

thanks, i hadn't seen the earlier call to GetServiceOwnerAndBlock and didn't
realize that this was already available here. i'll update this (probably next
week since i'm out today). :-)

[Daniel Erat, 2016-09-07 17:33:38]

thanks again for the suggestions. mind taking another look?

[Daniel Erat, 2016-09-07 17:33:45]

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

[commit-bot: I haz the power, 2016-09-07 17:34:12]

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 18:49:47]

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

[commit-bot: I haz the power, 2016-09-07 18:49:48]

Dry run: This issue passed the CQ dry run.

[hashimoto, 2016-09-08 03:37:10]

thanks, lgtm

[Daniel Erat, 2016-09-08 15:34:54]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2016-09-08 15:35:11]

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-08 15:41:50]

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

[commit-bot: I haz the power, 2016-09-08 15:41:51]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Daniel Erat, 2016-09-08 15:50:33]

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

[Daniel Erat, 2016-09-08 15:50:33]

rahul, please take a look at device/bluetooth/dbus. thanks!

[Rahul Chaturvedi, 2016-09-08 20:02:47]

lgtm

[Daniel Erat, 2016-09-08 20:15:43]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2016-09-08 20:16:16]

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-08 20:18:21]

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

[commit-bot: I haz the power, 2016-09-08 20:18:22]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Daniel Erat, 2016-09-08 20:50:31]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2016-09-08 20:51:13]

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-08 21:36:11]

Description was changed from

==========
dbus: Make ObjectManager get objects after service is ready.

Make dbus::ObjectManager automatically fetch managed objects
after the remote service becomes available. Also remove
Bus::GetManagedObjects(), which produces a large amount of
log spam due to some services not yet being ready. (It was
called by chromeos::DBusThreadManager and
bluez::BluezDBusManager at startup.)

BUG=636554
==========

to

==========
dbus: Make ObjectManager get objects after service is ready.

Make dbus::ObjectManager automatically fetch managed objects
after the remote service becomes available. Also remove
Bus::GetManagedObjects(), which produces a large amount of
log spam due to some services not yet being ready. (It was
called by chromeos::DBusThreadManager and
bluez::BluezDBusManager at startup.)

BUG=636554
==========

[commit-bot: I haz the power, 2016-09-08 21:36:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-08 21:39:51]

Description was changed from

==========
dbus: Make ObjectManager get objects after service is ready.

Make dbus::ObjectManager automatically fetch managed objects
after the remote service becomes available. Also remove
Bus::GetManagedObjects(), which produces a large amount of
log spam due to some services not yet being ready. (It was
called by chromeos::DBusThreadManager and
bluez::BluezDBusManager at startup.)

BUG=636554
==========

to

==========
dbus: Make ObjectManager get objects after service is ready.

Make dbus::ObjectManager automatically fetch managed objects
after the remote service becomes available. Also remove
Bus::GetManagedObjects(), which produces a large amount of
log spam due to some services not yet being ready. (It was
called by chromeos::DBusThreadManager and
bluez::BluezDBusManager at startup.)

BUG=636554

Committed: https://crrev.com/2ef7d0eda3fe8cb8a73c3ce977b8f1768cd32ece
Cr-Commit-Position: refs/heads/master@{#417405}
==========

[commit-bot: I haz the power, 2016-09-08 21:39:53]

Patchset 5 (id:??) landed as
https://crrev.com/2ef7d0eda3fe8cb8a73c3ce977b8f1768cd32ece
Cr-Commit-Position: refs/heads/master@{#417405}