Linux/CrOS: Retry connecting to mtpd.

Linux/CrOS: Listen for mtpd service owner change events and communicate with the new service owner.

The mtpd dbus service may not start right away. Any attempts to use it may be racy due to the lack of a service owner. Listening for service owner changes fixes this race.

BUG=241302
TEST=Manual, see bug for repro case.

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

[Lei Zhang, 2013-05-29 21:23:21]

We probably need to do something like this, or have a way on ChromeOS to ensure
mtpd starts before the browser process.

[Lei Zhang, 2013-05-29 23:28:33]

On 2013/05/29 21:23:21, Lei Zhang wrote:
> We probably need to do something like this, or have a way on ChromeOS to
ensure
> mtpd starts before the browser process.

FWIW, the second attempt (5 second delay) connected on my test Pixel.

[satorux1, 2013-05-30 00:58:10]

Could you explain the root cause of this problem (BTW, would be nice to explain
this in the patch description).

Maybe the MTP client code was calling methods on MTPD before MTPD has started? I
think solving such a problem by polling is not correct. We should instead do
something like:

1) set up a signal to detect name owner change
2) once the signal is set connected, get the current name owner (which is what
we do now with GetServiceOwner)
3) if the current name owner is there, do the initialization. otherwise, do
nothing.
4) once the name owner appears (we can tell by the signal set at the step 1), do
the initializaiton


Note that we don't have this service start-up race problem with signals (i.e.
connecting to signals don't require the sender to be already running).

Re name owner change, we have some code in dbus/object_proxy.cc, which may be
reusable.

[satorux1, 2013-05-30 01:01:14]

Another way to solve this problem is to ensure that Chrome starts after MTPD is
ready. IIRC, we can do something like that with 'upstart' but it'd slow down the
boot time, hence it's probably not a good idea.

[Lei Zhang, 2013-05-30 01:18:51]

On 2013/05/30 00:58:10, satorux1 wrote:
> Could you explain the root cause of this problem (BTW, would be nice to
explain
> this in the patch description).
> 
> Maybe the MTP client code was calling methods on MTPD before MTPD has started?
I
> think solving such a problem by polling is not correct. We should instead do
> something like:
> 
> 1) set up a signal to detect name owner change
> 2) once the signal is set connected, get the current name owner (which is what
> we do now with GetServiceOwner)
> 3) if the current name owner is there, do the initialization. otherwise, do
> nothing.
> 4) once the name owner appears (we can tell by the signal set at the step 1),
do
> the initializaiton
> 
> 
> Note that we don't have this service start-up race problem with signals (i.e.
> connecting to signals don't require the sender to be already running).
> 
> Re name owner change, we have some code in dbus/object_proxy.cc, which may be
> reusable.

Thanks for the feedback. I believe mtpd is not started right away when we first
try to connect. Thus the retry mechanism works. I wouldn't want to slow down the
startup by enforcing mtpd starting first either. I'll try out your suggestion
and ping you when I have an updated CL.

[satorux1, 2013-05-30 01:24:16]

On 2013/05/30 01:18:51, Lei Zhang wrote:
> On 2013/05/30 00:58:10, satorux1 wrote:
> > Could you explain the root cause of this problem (BTW, would be nice to
> explain
> > this in the patch description).
> > 
> > Maybe the MTP client code was calling methods on MTPD before MTPD has
started?
> I
> > think solving such a problem by polling is not correct. We should instead do
> > something like:
> > 
> > 1) set up a signal to detect name owner change
> > 2) once the signal is set connected, get the current name owner (which is
what
> > we do now with GetServiceOwner)
> > 3) if the current name owner is there, do the initialization. otherwise, do
> > nothing.
> > 4) once the name owner appears (we can tell by the signal set at the step
1),
> do
> > the initializaiton
> > 
> > 
> > Note that we don't have this service start-up race problem with signals
(i.e.
> > connecting to signals don't require the sender to be already running).
> > 
> > Re name owner change, we have some code in dbus/object_proxy.cc, which may
be
> > reusable.
> 
> Thanks for the feedback. I believe mtpd is not started right away when we
first
> try to connect. Thus the retry mechanism works. I wouldn't want to slow down
the
> startup by enforcing mtpd starting first either. I'll try out your suggestion
> and ping you when I have an updated CL.

Just to clarify, Chrome does not connect to MTPD directly so "connect" may not
be the right word. Chrome talks to MTPD via D-Bus. As mentioned, it's fine to
prepare receiving of signals from MTPD before MTPD has started, but it's not
good to call methods of MTPD before MTPD has started (i.e. method calls just
fail). My guess is that the latter is the root cause of this problem.

[Lei Zhang, 2013-05-31 02:51:11]

PTAL at patch set 2. I have added the functionality to listen and un-listen for
service owner changes to the Bus object. I think ObjectProxy may be able to take
advantage of this code, but I am trying to keep this CL small so it is easier to
merge back to M28.

[Lei Zhang, 2013-05-31 02:57:41]

On 2013/05/30 01:24:16, satorux1 wrote:
> Just to clarify, Chrome does not connect to MTPD directly so "connect" may not
> be the right word. Chrome talks to MTPD via D-Bus. As mentioned, it's fine to
> prepare receiving of signals from MTPD before MTPD has started, but it's not
> good to call methods of MTPD before MTPD has started (i.e. method calls just
> fail). My guess is that the latter is the root cause of this problem.

I put "communicate" in the CL description, but it seems only marginally better
than "connect".

The specific issue in the bug is that MTPD has not started yet, but the browser
has, and it checks if there is a service owner. Since there is not one yet, it
does not even try to call any MTPD methods at all.

I'm not sure how it ever worked previously if this was the case. Chrome would
have called MTPD methods and should have failed. Maybe it has always been racy
and recently the outcome of the race changed?

[satorux1, 2013-06-03 02:26:06]

https://codereview.chromium.org/15741025/diff/17005/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/15741025/diff/17005/dbus/bus.cc#newcode33
dbus/bus.cc:33: const char kNameOwnerChangedSignal[] = "NameOwnerChanged";
I think we have this in object_proxy.cc. While you are at it, could you
introduce dbus/constants.h and consolidate constants there?

https://codereview.chromium.org/15741025/diff/17005/dbus/bus.cc#newcode941
dbus/bus.cc:941: CHECK(AddFilterFunction(Bus::OnServiceOwnerChangedFilter,
this));
CHECK -> DCHECK. CHECK is rarely used.

https://codereview.chromium.org/15741025/diff/17005/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/15741025/diff/17005/dbus/bus.h#newcode567
dbus/bus.h:567: // name of the new owner.
What's gonna happen if the service name just disappears. In that case will the
callback receive an empty string? Please document.

Please also document that it's OK to add multiple listeners (callbacks) for the
same service name (but duplicate listeners are ignored)

https://codereview.chromium.org/15741025/diff/17005/dbus/bus.h#newcode577
dbus/bus.h:577: const GetServiceOwnerCallback& callback);
Could you write unit tests for the new functions? 'dbus' maintains high test
coverage. :)

https://codereview.chromium.org/15741025/diff/17005/device/media_transfer_pro...
File device/media_transfer_protocol/media_transfer_protocol_manager.cc (right):

https://codereview.chromium.org/15741025/diff/17005/device/media_transfer_pro...
device/media_transfer_protocol/media_transfer_protocol_manager.cc:59:
mtpd_owner_changed_callback_);
You might want to add a comment why we are doing this, as it's not very obvious.

[satorux1, 2013-06-03 02:29:30]

On 2013/05/31 02:57:41, Lei Zhang wrote:
> On 2013/05/30 01:24:16, satorux1 wrote:
> > Just to clarify, Chrome does not connect to MTPD directly so "connect" may
not
> > be the right word. Chrome talks to MTPD via D-Bus. As mentioned, it's fine
to
> > prepare receiving of signals from MTPD before MTPD has started, but it's not
> > good to call methods of MTPD before MTPD has started (i.e. method calls just
> > fail). My guess is that the latter is the root cause of this problem.
> 
> I put "communicate" in the CL description, but it seems only marginally better
> than "connect".
> 
> The specific issue in the bug is that MTPD has not started yet, but the
browser
> has, and it checks if there is a service owner. Since there is not one yet, it
> does not even try to call any MTPD methods at all.
> 
> I'm not sure how it ever worked previously if this was the case. Chrome would
> have called MTPD methods and should have failed. Maybe it has always been racy
> and recently the outcome of the race changed?

That would probably be the case. This made me wonder if we had similar issues
with other D-Bus clients. I'm guessing we do have races (scary!), but these
races do not cause significant consequences so far. It seems MTPD is the first
to get hit by this. Is MTPD slow to boot?

BTW, could you add TEST= line? I'm interested in how you verified that this
change fixed the issue. Would be also nice to have some explanation about the
issue and the fix in the patch description, to record more context of this
change.

[Lei Zhang, 2013-06-04 00:35:53]

Looking at unit tests now...

https://codereview.chromium.org/15741025/diff/17005/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/15741025/diff/17005/dbus/bus.cc#newcode33
dbus/bus.cc:33: const char kNameOwnerChangedSignal[] = "NameOwnerChanged";
On 2013/06/03 02:26:06, satorux1 wrote:
> I think we have this in object_proxy.cc. While you are at it, could you
> introduce dbus/constants.h and consolidate constants there?

I can do this in a later cleanup CL. I'm trying to keep this CL small for
merging.

https://codereview.chromium.org/15741025/diff/17005/dbus/bus.cc#newcode941
dbus/bus.cc:941: CHECK(AddFilterFunction(Bus::OnServiceOwnerChangedFilter,
this));
On 2013/06/03 02:26:06, satorux1 wrote:
> CHECK -> DCHECK. CHECK is rarely used.

Done.

https://codereview.chromium.org/15741025/diff/17005/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/15741025/diff/17005/dbus/bus.h#newcode567
dbus/bus.h:567: // name of the new owner.
On 2013/06/03 02:26:06, satorux1 wrote:
> What's gonna happen if the service name just disappears. In that case will the
> callback receive an empty string? Please document.
> 
> Please also document that it's OK to add multiple listeners (callbacks) for
the
> same service name (but duplicate listeners are ignored)

Done.

https://codereview.chromium.org/15741025/diff/17005/device/media_transfer_pro...
File device/media_transfer_protocol/media_transfer_protocol_manager.cc (right):

https://codereview.chromium.org/15741025/diff/17005/device/media_transfer_pro...
device/media_transfer_protocol/media_transfer_protocol_manager.cc:59:
mtpd_owner_changed_callback_);
On 2013/06/03 02:26:06, satorux1 wrote:
> You might want to add a comment why we are doing this, as it's not very
obvious.

Done.

[satorux1, 2013-06-04 01:09:05]

https://codereview.chromium.org/15741025/diff/17005/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/15741025/diff/17005/dbus/bus.cc#newcode33
dbus/bus.cc:33: const char kNameOwnerChangedSignal[] = "NameOwnerChanged";
On 2013/06/04 00:35:53, Lei Zhang wrote:
> On 2013/06/03 02:26:06, satorux1 wrote:
> > I think we have this in object_proxy.cc. While you are at it, could you
> > introduce dbus/constants.h and consolidate constants there?
> 
> I can do this in a later cleanup CL. I'm trying to keep this CL small for
> merging.

I see. That makes sense.

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.cc#newcode927
dbus/bus.cc:927: if (service_name.empty() || callback.is_null())
Let's make them mandatory:

DCHECK(!service_name.empty());
DCHECK(!callback.is_null());

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.cc#newcode945
dbus/bus.cc:945: if (service_owner_changed_listener_map_.size() == 0) {
service_owner_changed_listener_map_.empty()

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.cc#newcode968
dbus/bus.cc:968: std::vector<GetServiceOwnerCallback>())).first;
Maybe simpler to do:

service_owner_changed_listener_map_[service_name].push_back(callback);
return;

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.cc#newcode1012
dbus/bus.cc:1012: if (callbacks.size() > 0)
!callbacks.empty() ?

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.cc#newcode1024
dbus/bus.cc:1024: if (service_owner_changed_listener_map_.size() == 0) {
service_owner_changed_listener_map_.empty()

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.cc#newcode1159
dbus/bus.cc:1159: return DBUS_HANDLER_RESULT_HANDLED;
I think we should return DBUS_HANDLER_RESULT_NOT_YET_HANDLED as some other code
may be interested in this owner change. See:

DBusHandlerResult ObjectProxy::HandleNameOwnerChanged(
   ...
  // Always return unhandled to let other object proxies handle the same        
  // signal.                                                                    
  return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
}

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.h#newcode571
dbus/bus.h:571: // and so are empty |service_name|s and null |callback|s.
Silently ignoring empty service name and callback doesn't seem to be correct
(i.e. may hide bugs in the client code). It'd be nicer to make them mandatory.
|service_name| and |callback| should not be empty/null.

[Lei Zhang, 2013-06-04 06:15:56]

Test added in patch set 4.

I cannot speak to how other dbus code work and how they are affected by
potential races, but the mtp client previous made the assumption that if mtpd is
not there, then don't bother doing anything. I didn't realize there was a race
condition on startup, which is what I'm fixing here.

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.cc#newcode927
dbus/bus.cc:927: if (service_name.empty() || callback.is_null())
On 2013/06/04 01:09:06, satorux1 wrote:
> Let's make them mandatory:
> 
> DCHECK(!service_name.empty());
> DCHECK(!callback.is_null());

Done.

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.cc#newcode945
dbus/bus.cc:945: if (service_owner_changed_listener_map_.size() == 0) {
On 2013/06/04 01:09:06, satorux1 wrote:
> service_owner_changed_listener_map_.empty()

Done.

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.cc#newcode968
dbus/bus.cc:968: std::vector<GetServiceOwnerCallback>())).first;
On 2013/06/04 01:09:06, satorux1 wrote:
> Maybe simpler to do:
> 
> service_owner_changed_listener_map_[service_name].push_back(callback);
> return;

Done.

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.cc#newcode1012
dbus/bus.cc:1012: if (callbacks.size() > 0)
On 2013/06/04 01:09:06, satorux1 wrote:
> !callbacks.empty() ?

Done.

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.cc#newcode1024
dbus/bus.cc:1024: if (service_owner_changed_listener_map_.size() == 0) {
On 2013/06/04 01:09:06, satorux1 wrote:
> service_owner_changed_listener_map_.empty()

Done.

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.cc#newcode1159
dbus/bus.cc:1159: return DBUS_HANDLER_RESULT_HANDLED;
On 2013/06/04 01:09:06, satorux1 wrote:
> I think we should return DBUS_HANDLER_RESULT_NOT_YET_HANDLED as some other
code
> may be interested in this owner change. See:
> 
> DBusHandlerResult ObjectProxy::HandleNameOwnerChanged(
>    ...
>   // Always return unhandled to let other object proxies handle the same      
 
>   // signal.                                                                  
 
>   return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> }

Done.

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/15741025/diff/25001/dbus/bus.h#newcode571
dbus/bus.h:571: // and so are empty |service_name|s and null |callback|s.
On 2013/06/04 01:09:06, satorux1 wrote:
> Silently ignoring empty service name and callback doesn't seem to be correct
> (i.e. may hide bugs in the client code). It'd be nicer to make them mandatory.
> |service_name| and |callback| should not be empty/null.

Done.

[satorux1, 2013-06-05 01:33:58]

Sorry for the belated response.

https://codereview.chromium.org/15741025/diff/30001/dbus/bus_unittest.cc
File dbus/bus_unittest.cc (right):

https://codereview.chromium.org/15741025/diff/30001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:397: base::Thread dbus_thread("D-Bus thread");
Do we need to create a D-Bus thread? Other test cases in this file don't create
a D-Bus thread. 

Lots of helper functions (ex. GetServiceOwner() and GetServiceOwnerHelper())
were added in order to emulate blocking calls. These are unnecessary, if the
D-Bus thread is not in use. You can just use blocking functions liek
Bus::GetServiceOwnerAndBlock().

https://codereview.chromium.org/15741025/diff/30001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:407: OwnershipChangeListener ownership_change_listener_1;
nit: ownership_change_listener1 ? having _ before number looked a bit unusual

https://codereview.chromium.org/15741025/diff/30001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:408:
ownership_change_listener_1.set_current_owner("dummy1");
At first, I was confused about why a dummy value was set here. This is necessary
to confirm that GetServiceOwner() returns an empty string, rather than the dummy
value, right?

Seems to me that this test case is testing GetServiceOwner() and
ListenForServiceOwnerChange(). I think it'd be nicer to have a separate test
case for  GetServiceOwner(). Then, we can just assume that GetServiceOwner()
works properly hence no need to set a dummy value here.

https://codereview.chromium.org/15741025/diff/30001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:410:
base::Bind(&OwnershipChangeListener::OnServiceOwnerChanged,
This class seems to be unnecessary. Can we just pass some parameters to a
callback bound to a free function like:

dbus::Bus::GetServiceOwnerCallback callback1 =
   base::Bind(&OnServiceOwnerChanged,
              &current_owner,
              &num_owner_changes));

That'd be simpler than having a class.

If we are to stop creating a D-Bus thread, you'd need to pass a quit closure to
that callback function to quit the message loop. The code would look like:

bus_->RequestOwnershipAndBlock(...);
run_loop.Run();  // OnServiceOwnerChanged() will quit the loop.

https://codereview.chromium.org/15741025/diff/30001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:426: service_owner = GetServiceOwner(bus,
"org.chromium.TestService");
How does the |service_owner_| look like? Could you add an example here as a
comment?

https://codereview.chromium.org/15741025/diff/30001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:440:
bus->ListenForServiceOwnerChange("org.chromium.FakeService", callback2);
Why do we listen to "org.chromium.FakeService"? Could you add a comment, or
remove the code?

[Lei Zhang, 2013-06-09 04:37:56]

https://codereview.chromium.org/15741025/diff/30001/dbus/bus_unittest.cc
File dbus/bus_unittest.cc (right):

https://codereview.chromium.org/15741025/diff/30001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:397: base::Thread dbus_thread("D-Bus thread");
On 2013/06/05 01:33:58, satorux1 wrote:
> Do we need to create a D-Bus thread? Other test cases in this file don't
create
> a D-Bus thread. 
> 
> Lots of helper functions (ex. GetServiceOwner() and GetServiceOwnerHelper())
> were added in order to emulate blocking calls. These are unnecessary, if the
> D-Bus thread is not in use. You can just use blocking functions liek
> Bus::GetServiceOwnerAndBlock().

I copied it from the test right above this one and it made things a lot more
complicated. The next patch set doesn't have the DBus thread and is a lot
simpler.

https://codereview.chromium.org/15741025/diff/30001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:407: OwnershipChangeListener ownership_change_listener_1;
On 2013/06/05 01:33:58, satorux1 wrote:
> nit: ownership_change_listener1 ? having _ before number looked a bit unusual

Done.

https://codereview.chromium.org/15741025/diff/30001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:410:
base::Bind(&OwnershipChangeListener::OnServiceOwnerChanged,
On 2013/06/05 01:33:58, satorux1 wrote:
> This class seems to be unnecessary. Can we just pass some parameters to a
> callback bound to a free function like:
> 
> dbus::Bus::GetServiceOwnerCallback callback1 =
>    base::Bind(&OnServiceOwnerChanged,
>               &current_owner,
>               &num_owner_changes));
> 
> That'd be simpler than having a class.

Done

> If we are to stop creating a D-Bus thread, you'd need to pass a quit closure
to
> that callback function to quit the message loop. The code would look like:
> 
> bus_->RequestOwnershipAndBlock(...);
> run_loop.Run();  // OnServiceOwnerChanged() will quit the loop.

But when I have 2 listeners, they will call Quit() 1 time too many. I used
RunUntilIdle() instead.

https://codereview.chromium.org/15741025/diff/30001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:440:
bus->ListenForServiceOwnerChange("org.chromium.FakeService", callback2);
On 2013/06/05 01:33:58, satorux1 wrote:
> Why do we listen to "org.chromium.FakeService"? Could you add a comment, or
> remove the code?

I guess it doesn't add too much value to the test, removed.

[satorux1, 2013-06-10 01:16:22]

https://codereview.chromium.org/15741025/diff/44001/dbus/bus_unittest.cc
File dbus/bus_unittest.cc (right):

https://codereview.chromium.org/15741025/diff/44001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:329: base::RunLoop().RunUntilIdle();
this call looks unnecessary.

https://codereview.chromium.org/15741025/diff/44001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:337: base::RunLoop().RunUntilIdle();
Not sure if this race-free (i.e. OnServiceOwnerChanged() may not be called here,
as the owner change signal may be delivered a bit later). This is why I
suggested to call Quit() inside the callback function.

> But when I have 2 listeners, they will call Quit() 1 time too many. I used
> RunUntilIdle() instead.

Can we solve this problem by simply calling Run() twice? If this doesn't work,
we could pass extra parameters to the callbacks to control when to call Quit(),
but it'd be a bit complicated.

[Lei Zhang, 2013-06-10 04:50:07]

On 2013/06/10 01:16:22, satorux1 wrote:
> https://codereview.chromium.org/15741025/diff/44001/dbus/bus_unittest.cc
> File dbus/bus_unittest.cc (right):
> 
>
https://codereview.chromium.org/15741025/diff/44001/dbus/bus_unittest.cc#newc...
> dbus/bus_unittest.cc:329: base::RunLoop().RunUntilIdle();
> this call looks unnecessary.
> 
>
https://codereview.chromium.org/15741025/diff/44001/dbus/bus_unittest.cc#newc...
> dbus/bus_unittest.cc:337: base::RunLoop().RunUntilIdle();
> Not sure if this race-free (i.e. OnServiceOwnerChanged() may not be called
here,
> as the owner change signal may be delivered a bit later). This is why I
> suggested to call Quit() inside the callback function.
> 
> > But when I have 2 listeners, they will call Quit() 1 time too many. I used
> > RunUntilIdle() instead.
> 
> Can we solve this problem by simply calling Run() twice? If this doesn't work,
> we could pass extra parameters to the callbacks to control when to call
Quit(),
> but it'd be a bit complicated.

With base::RunLoop, it can only Run() once. (See |run_called_| usage in
base/run_loop.cc.)

I made a RunLoop() wrapper that contains the state and just passed that to
OnServiceOwnerChanged(). See patch set 7.

[satorux1, 2013-06-10 04:58:46]

LGTM with nits. Thank you for writing the unit test!

https://codereview.chromium.org/15741025/diff/52001/dbus/bus_unittest.cc
File dbus/bus_unittest.cc (right):

https://codereview.chromium.org/15741025/diff/52001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:29: // test helpers for
BusTest.ListenForServiceOwnerChange.
This class does a non-trivial thing. Could you write a class comment?

https://codereview.chromium.org/15741025/diff/52001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:30: class RunLoopState {
RunLoopWithExpectedCount may be a bit more descriptive.

https://codereview.chromium.org/15741025/diff/52001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:43: void Quit() {
Quit() may be a misnomer as this may not quit. Maybe:

MaybeQuit()
QuitIfConditionIsSatisified()

https://codereview.chromium.org/15741025/diff/52001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:59: void OnServiceOwnerChanged(RunLoopState* run_loop,
run_loop_state ?

[Lei Zhang, 2013-06-10 05:20:32]

https://codereview.chromium.org/15741025/diff/52001/dbus/bus_unittest.cc
File dbus/bus_unittest.cc (right):

https://codereview.chromium.org/15741025/diff/52001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:29: // test helpers for
BusTest.ListenForServiceOwnerChange.
On 2013/06/10 04:58:46, satorux1 wrote:
> This class does a non-trivial thing. Could you write a class comment?

Done.

https://codereview.chromium.org/15741025/diff/52001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:30: class RunLoopState {
On 2013/06/10 04:58:46, satorux1 wrote:
> RunLoopWithExpectedCount may be a bit more descriptive.

Done.

https://codereview.chromium.org/15741025/diff/52001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:43: void Quit() {
On 2013/06/10 04:58:46, satorux1 wrote:
> Quit() may be a misnomer as this may not quit. Maybe:
> 
> MaybeQuit()
> QuitIfConditionIsSatisified()

Done.

https://codereview.chromium.org/15741025/diff/52001/dbus/bus_unittest.cc#newc...
dbus/bus_unittest.cc:59: void OnServiceOwnerChanged(RunLoopState* run_loop,
On 2013/06/10 04:58:46, satorux1 wrote:
> run_loop_state ?

Done.

[commit-bot: I haz the power, 2013-06-10 05:20:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/15741025/59001

[commit-bot: I haz the power, 2013-06-10 06:40:00]

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

[commit-bot: I haz the power, 2013-06-10 08:12:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/15741025/59001

[commit-bot: I haz the power, 2013-06-10 08:57:47]

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

[commit-bot: I haz the power, 2013-06-10 09:15:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/15741025/59001

[commit-bot: I haz the power, 2013-06-10 09:58:13]

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

[commit-bot: I haz the power, 2013-06-10 18:47:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/15741025/59001

[commit-bot: I haz the power, 2013-06-10 22:52:35]

Change committed as 205331