dbus::ObjectManager: Add a match rule for properties before GetManagedObjects.

dbus::ObjectManager: Add a match rule for properties before GetManagedObjects.

There is a race condition in the way that match rules get set up for object
proxies created in response to GetManagedObjects that may cause us the miss
PropertiesChanged signals if they're received before the match rule and filter
function get added by ObjectProxy.

This patch changes this to work the "intended" way: ObjectManager now adds a
single match rule for its corresponding service name, and specifically for the
org.freedesktop.DBus.Properties.PropertiesChanged signal. Once it receives the
signal, ObjectManager dispatches the signal to the corresponding PropertySet.

BUG=407109, 400768
TEST=dbus_unittests

Committed: https://crrev.com/ebff093d22cd5c0613f3493acdbc1af1cfd5d31f
Cr-Commit-Position: refs/heads/master@{#293551}

[armansito, 2014-08-27 17:45:56]

armansito@chromium.org changed reviewers:
+ keybuk@chromium.org, satorux@chromium.org

[armansito, 2014-08-27 17:45:56]

Ended up not using the path_namespace rule.

keybuk@, satorux@: PTAL.
zeuthen@: FYI.

[satorux1, 2014-08-28 04:00:01]

Could you add a test that passes with the fix but fails without the fix?

https://codereview.chromium.org/510863002/diff/1/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/510863002/diff/1/dbus/bus.cc#newcode356
dbus/bus.cc:356: AssertOnOriginThread();
add: DCHECK(!callback.is_null()); to make it clear that the callback must not be
null

https://codereview.chromium.org/510863002/diff/1/dbus/bus.cc#newcode363
dbus/bus.cc:363: // ObjectManager is present. Remove it now and CleanUp in the
DBus thread.
in -> on ?

https://codereview.chromium.org/510863002/diff/1/dbus/object_manager.cc
File dbus/object_manager.cc (right):

https://codereview.chromium.org/510863002/diff/1/dbus/object_manager.cc#newco...
dbus/object_manager.cc:30: }
this is anotner one in object_proxy.cc. I think it's time to create util.h/cc
that contains these functions.

https://codereview.chromium.org/510863002/diff/1/dbus/object_manager.cc#newco...
dbus/object_manager.cc:312: Signal* signal) {
add   bus_->AssertOnOriginThread(); to be consistent with other functions.

[armansito, 2014-08-28 04:30:50]

https://codereview.chromium.org/510863002/diff/1/dbus/object_manager.cc
File dbus/object_manager.cc (right):

https://codereview.chromium.org/510863002/diff/1/dbus/object_manager.cc#newco...
dbus/object_manager.cc:30: }
On 2014/08/28 04:00:01, satorux1 wrote:
> this is anotner one in object_proxy.cc. I think it's time to create util.h/cc
> that contains these functions.

Ack. I'll add a TODO for this and do it in another CL. This should probably be
an inline function then. In fact, I only use it in one place in this file, so I
can just do the concatenation in place and remove this function entirely.

[armansito, 2014-08-28 15:37:59]

Added a unit test which fails 90% of the time without the fix; it matches the
expected behavior of the race condition.

https://codereview.chromium.org/510863002/diff/1/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/510863002/diff/1/dbus/bus.cc#newcode356
dbus/bus.cc:356: AssertOnOriginThread();
On 2014/08/28 04:00:01, satorux1 wrote:
> add: DCHECK(!callback.is_null()); to make it clear that the callback must not
be
> null

Done.

https://codereview.chromium.org/510863002/diff/1/dbus/bus.cc#newcode363
dbus/bus.cc:363: // ObjectManager is present. Remove it now and CleanUp in the
DBus thread.
On 2014/08/28 04:00:01, satorux1 wrote:
> in -> on ?

Done.

https://codereview.chromium.org/510863002/diff/1/dbus/object_manager.cc
File dbus/object_manager.cc (right):

https://codereview.chromium.org/510863002/diff/1/dbus/object_manager.cc#newco...
dbus/object_manager.cc:312: Signal* signal) {
On 2014/08/28 04:00:01, satorux1 wrote:
> add   bus_->AssertOnOriginThread(); to be consistent with other functions.

Done.

[satorux1, 2014-08-29 05:20:53]

Please also have this reviewed by keybuk@.

https://codereview.chromium.org/510863002/diff/1/dbus/object_manager.cc
File dbus/object_manager.cc (right):

https://codereview.chromium.org/510863002/diff/1/dbus/object_manager.cc#newco...
dbus/object_manager.cc:30: }
On 2014/08/28 04:30:49, armansito wrote:
> On 2014/08/28 04:00:01, satorux1 wrote:
> > this is anotner one in object_proxy.cc. I think it's time to create
util.h/cc
> > that contains these functions.
> 
> Ack. I'll add a TODO for this and do it in another CL. This should probably be
> an inline function then. In fact, I only use it in one place in this file, so
I
> can just do the concatenation in place and remove this function entirely.

It's fine to do it in a separate patch, but that cleanup is trivial enough to be
included in this patch. It's up to you.

https://codereview.chromium.org/510863002/diff/1/dbus/object_manager.cc#newco...
dbus/object_manager.cc:325: bus_->AssertOnOriginThread();
let's keep them. a bit verbose, but good to have them for clarity.

https://codereview.chromium.org/510863002/diff/20001/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/510863002/diff/20001/dbus/bus.h#newcode363
dbus/bus.h:363: virtual bool RemoveObjectManager(const std::string&
service_name,
Please document the return value.

https://codereview.chromium.org/510863002/diff/20001/dbus/test_service.cc
File dbus/test_service.cc (right):

https://codereview.chromium.org/510863002/diff/20001/dbus/test_service.cc#new...
dbus/test_service.cc:493: SetSendImmediatePropertiesChanged();
please add {} and while you are at it, please add {} elsewhere in the if-else
blocks.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditionals

However, if one part of an if-else statement uses curly braces, the other part
must too:

[armansito, 2014-08-29 16:17:08]

https://codereview.chromium.org/510863002/diff/1/dbus/object_manager.cc
File dbus/object_manager.cc (right):

https://codereview.chromium.org/510863002/diff/1/dbus/object_manager.cc#newco...
dbus/object_manager.cc:30: }
On 2014/08/29 05:20:53, satorux1 wrote:
> On 2014/08/28 04:30:49, armansito wrote:
> > On 2014/08/28 04:00:01, satorux1 wrote:
> > > this is anotner one in object_proxy.cc. I think it's time to create
> util.h/cc
> > > that contains these functions.
> > 
> > Ack. I'll add a TODO for this and do it in another CL. This should probably
be
> > an inline function then. In fact, I only use it in one place in this file,
so
> I
> > can just do the concatenation in place and remove this function entirely.
> 
> It's fine to do it in a separate patch, but that cleanup is trivial enough to
be
> included in this patch. It's up to you.

Done.

https://codereview.chromium.org/510863002/diff/1/dbus/object_manager.cc#newco...
dbus/object_manager.cc:325: bus_->AssertOnOriginThread();
On 2014/08/29 05:20:53, satorux1 wrote:
> let's keep them. a bit verbose, but good to have them for clarity.

Done.

https://codereview.chromium.org/510863002/diff/20001/dbus/test_service.cc
File dbus/test_service.cc (right):

https://codereview.chromium.org/510863002/diff/20001/dbus/test_service.cc#new...
dbus/test_service.cc:493: SetSendImmediatePropertiesChanged();
On 2014/08/29 05:20:53, satorux1 wrote:
> please add {} and while you are at it, please add {} elsewhere in the if-else
> blocks.
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditionals
> 
> However, if one part of an if-else statement uses curly braces, the other part
> must too:

Done.

[armansito, 2014-09-03 05:34:45]

ping

[armansito, 2014-09-03 21:49:52]

On 2014/09/03 05:34:45, armansito wrote:
> ping

PTAL whenever you can. This fix is needed for 38.

[satorux1, 2014-09-04 04:22:29]

LGTM (sorry I thought I sent LGTM already).

As mentioned, I'd like this patch to be reviewed by keybuk@. keybuk@, could you
take a look?

https://codereview.chromium.org/510863002/diff/40001/dbus/util.h
File dbus/util.h (right):

https://codereview.chromium.org/510863002/diff/40001/dbus/util.h#newcode22
dbus/util.h:22: CHROME_DBUS_EXPORT std::string GetAbsoluteMemberName(
Thank you for creating this!

[satorux1, 2014-09-04 04:25:33]

https://codereview.chromium.org/510863002/diff/40001/dbus/util.cc
File dbus/util.cc (right):

https://codereview.chromium.org/510863002/diff/40001/dbus/util.cc#newcode11
dbus/util.cc:11: return interface_name + "." + member_name;
It may sound a bit silly to have a unit test for this function, but I'd be happy
if you could create a util_unittest.cc and add a test for this. Having
util_unittest.cc makes it easier for us to add new tests when new functions are
added to this file.

[armansito, 2014-09-05 00:47:12]

https://codereview.chromium.org/510863002/diff/40001/dbus/util.cc
File dbus/util.cc (right):

https://codereview.chromium.org/510863002/diff/40001/dbus/util.cc#newcode11
dbus/util.cc:11: return interface_name + "." + member_name;
On 2014/09/04 04:25:33, satorux1 wrote:
> It may sound a bit silly to have a unit test for this function, but I'd be
happy
> if you could create a util_unittest.cc and add a test for this. Having
> util_unittest.cc makes it easier for us to add new tests when new functions
are
> added to this file.

I'd rather not get this CL any larger, since I intend to merge this to 38 asap.
I'll follow this up with another CL that adds a util_unittest on ToT.

[armansito, 2014-09-05 00:47:16]

The CQ bit was checked by armansito@chromium.org

[commit-bot: I haz the power, 2014-09-05 00:47:58]

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

[satorux1, 2014-09-05 00:54:43]

The CQ bit was unchecked by satorux@chromium.org

[satorux1, 2014-09-05 00:54:47]

Before submitting. Could you ping keybuk@? If he's ooo, then it's fine, but
otherwise, I'd like keybuk@ to take a look at this patch.

[armansito, 2014-09-05 00:58:21]

The CQ bit was checked by armansito@chromium.org

[commit-bot: I haz the power, 2014-09-05 00:59:03]

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

[commit-bot: I haz the power, 2014-09-05 04:07:59]

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

[commit-bot: I haz the power, 2014-09-05 04:08:00]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/51955)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[armansito, 2014-09-05 04:59:43]

The CQ bit was checked by armansito@chromium.org

[commit-bot: I haz the power, 2014-09-05 05:00:32]

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

[commit-bot: I haz the power, 2014-09-05 05:22:27]

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

[commit-bot: I haz the power, 2014-09-05 05:22:28]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[armansito, 2014-09-05 16:26:03]

The CQ bit was checked by armansito@chromium.org

[commit-bot: I haz the power, 2014-09-05 16:27:49]

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

[commit-bot: I haz the power, 2014-09-05 17:50:21]

Committed patchset #5 (id:80001) as 0c7f357634a3b58d6017462edde98b6e6a8e2cd5

[commit-bot: I haz the power, 2014-09-10 03:40:37]

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