Allow Chromium's DBus service ownership to be stealable

Allow Chromium's DBus service ownership to be stealable

We've seen some cases in tests where a Chromium process winds up in a
temporarily unkillable state, causing the dbus-daemon to believe that
it still actively owns org.chromium.LibCrosService.  This makes
attempts to restart the UI fail, as the browser dies when it cannot
take ownership of this service name. The reason it can't is because
Chromium currently doesn't allow other processes to steal ownership --
and the unkillable process is holding onto the token.

This can be remedied by providing certain options when ownership of
the service name is taken, options that allow other processes to seize
ownership if they so choose. The ramifications of this are discussed
further in the bug.

BUG=chromium:261381
TEST=new unit test in dbus_unittest
TEST=run the following as chronos on a device: "gdbus call --system --dest org.freedesktop.DBus --object-path /org/freedesktop/DBus --method org.freedesktop.DBus.RequestName org.chromium.LibCrosService 7"
TEST=This should return (uint32 1,)

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

[Chris Masone, 2013-07-25 23:38:54]

Satoru, I still need to test this out, but I've uploaded it here for your early
perusal.

[satorux1, 2013-07-26 00:50:12]

Thank you for working on this!

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

https://codereview.chromium.org/20555003/diff/1/dbus/bus.cc#newcode575
dbus/bus.cc:575: << (error.is_set() ? error.message() : res_str);
Maybe we should always print the error code:

LOG(ERROR) << "Failed to release the ownership of " << service_name << ": "
           << "error code: " << result << ": "
           <<  (error.is_set() ? error.message() : "");

https://codereview.chromium.org/20555003/diff/1/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/20555003/diff/1/dbus/bus.h#newcode183
dbus/bus.h:183: ALLOW_REPLACEMENT = DBUS_NAME_FLAG_ALLOW_REPLACEMENT,
Don't we need DBUS_NAME_FLAG_DO_NOT_QUEUE for this? I think we rarely want to
wait in the queue.

That said, simply copying the libdbus constants would be more straightforward:

enum ServiceOwnershipOptions {
  ALLOW_REPLACEMENT = DBUS_NAME_FLAG_ALLOW_REPLACEMENT,
  REPLACE_EXISTING = DBUS_NAME_FLAG_REPLACE_EXISTING,
  DO_NOT_QUEUE = DBUS_NAME_FLAG_DO_NOT_QUEUE,
}

Then RequestOwnership() would take 'int options' which is an OR'ed value of
ServiceOwnershipOptions.

https://codereview.chromium.org/20555003/diff/1/dbus/signal_sender_verificati...
File dbus/signal_sender_verification_unittest.cc (right):

https://codereview.chromium.org/20555003/diff/1/dbus/signal_sender_verificati...
dbus/signal_sender_verification_unittest.cc:272: LOG(INFO) << "Spun up new test
service";
please remove this.

[Chris Masone, 2013-07-26 01:00:24]

https://codereview.chromium.org/20555003/diff/1/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/20555003/diff/1/dbus/bus.h#newcode183
dbus/bus.h:183: ALLOW_REPLACEMENT = DBUS_NAME_FLAG_ALLOW_REPLACEMENT,
On 2013/07/26 00:50:12, satorux1 wrote:
> Don't we need DBUS_NAME_FLAG_DO_NOT_QUEUE for this? I think we rarely want to
> wait in the queue.
> 
> That said, simply copying the libdbus constants would be more straightforward:
> 
> enum ServiceOwnershipOptions {
>   ALLOW_REPLACEMENT = DBUS_NAME_FLAG_ALLOW_REPLACEMENT,
>   REPLACE_EXISTING = DBUS_NAME_FLAG_REPLACE_EXISTING,
>   DO_NOT_QUEUE = DBUS_NAME_FLAG_DO_NOT_QUEUE,
> }
> 
> Then RequestOwnership() would take 'int options' which is an OR'ed value of
> ServiceOwnershipOptions.

I thought about doing that, but wasn't sure if it made sense to have callers be
OR'ing together enum values.  That only works when the values are 0x1, 0x2, 0x4
etc (which they are here), but that's sort of an implementation detail.  That
was why I went with these semantically higher-level enum's.  I'm not married to
this approach, though, if you think that the other way is better.

[satorux1, 2013-07-26 01:14:45]

https://codereview.chromium.org/20555003/diff/1/chrome/browser/chromeos/dbus/...
File chrome/browser/chromeos/dbus/cros_dbus_service.cc (right):

https://codereview.chromium.org/20555003/diff/1/chrome/browser/chromeos/dbus/...
chrome/browser/chromeos/dbus/cros_dbus_service.cc:53:
dbus::Bus::REQUIRE_PRIMARY_ALLOW_REPLACEMENT,
Please add some comment about why we are allowing replacement here as it's not
obvious. Having a pointer to crbug.com URL would be nice.

https://codereview.chromium.org/20555003/diff/1/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/20555003/diff/1/dbus/bus.h#newcode183
dbus/bus.h:183: ALLOW_REPLACEMENT = DBUS_NAME_FLAG_ALLOW_REPLACEMENT,
On 2013/07/26 01:00:24, Chris Masone wrote:
> On 2013/07/26 00:50:12, satorux1 wrote:
> > Don't we need DBUS_NAME_FLAG_DO_NOT_QUEUE for this? I think we rarely want
to
> > wait in the queue.
> > 
> > That said, simply copying the libdbus constants would be more
straightforward:
> > 
> > enum ServiceOwnershipOptions {
> >   ALLOW_REPLACEMENT = DBUS_NAME_FLAG_ALLOW_REPLACEMENT,
> >   REPLACE_EXISTING = DBUS_NAME_FLAG_REPLACE_EXISTING,
> >   DO_NOT_QUEUE = DBUS_NAME_FLAG_DO_NOT_QUEUE,
> > }
> > 
> > Then RequestOwnership() would take 'int options' which is an OR'ed value of
> > ServiceOwnershipOptions.
> 
> I thought about doing that, but wasn't sure if it made sense to have callers
be
> OR'ing together enum values.  That only works when the values are 0x1, 0x2,
0x4
> etc (which they are here), but that's sort of an implementation detail.  That
> was why I went with these semantically higher-level enum's.  I'm not married
to
> this approach, though, if you think that the other way is better.

That's a good point. I think ALLOW_REPLACEMENT alone won't be used in the
caller, so what about just providing the two options:

enum ServiceOwnershipOptions {
  REQUIRE_PRIMARY = (DBUS_NAME_FLAG_DO_NOT_QUEUE |
DBUS_NAME_FLAG_REPLACE_EXISTING),
  REQUIRE_PRIMARY_ALLOW_REPLACEMENT = (REQUIRE_PRIMARY |
DBUS_NAME_FLAG_ALLOW_REPLACEMENT)
}

This would suffice for now. We can switch to the OR-ed value approach if needed
at a later time.

[Chris Masone, 2013-07-26 17:37:24]

https://codereview.chromium.org/20555003/diff/1/chrome/browser/chromeos/dbus/...
File chrome/browser/chromeos/dbus/cros_dbus_service.cc (right):

https://codereview.chromium.org/20555003/diff/1/chrome/browser/chromeos/dbus/...
chrome/browser/chromeos/dbus/cros_dbus_service.cc:53:
dbus::Bus::REQUIRE_PRIMARY_ALLOW_REPLACEMENT,
On 2013/07/26 01:14:45, satorux1 wrote:
> Please add some comment about why we are allowing replacement here as it's not
> obvious. Having a pointer to http://crbug.com URL would be nice.

Done.

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

https://codereview.chromium.org/20555003/diff/1/dbus/bus.cc#newcode575
dbus/bus.cc:575: << (error.is_set() ? error.message() : res_str);
On 2013/07/26 00:50:12, satorux1 wrote:
> Maybe we should always print the error code:
> 
> LOG(ERROR) << "Failed to release the ownership of " << service_name << ": "
>            << "error code: " << result << ": "
>            <<  (error.is_set() ? error.message() : "");

Done.

https://codereview.chromium.org/20555003/diff/1/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/20555003/diff/1/dbus/bus.h#newcode183
dbus/bus.h:183: ALLOW_REPLACEMENT = DBUS_NAME_FLAG_ALLOW_REPLACEMENT,
On 2013/07/26 01:14:45, satorux1 wrote:
> On 2013/07/26 01:00:24, Chris Masone wrote:
> > On 2013/07/26 00:50:12, satorux1 wrote:
> > > Don't we need DBUS_NAME_FLAG_DO_NOT_QUEUE for this? I think we rarely want
> to
> > > wait in the queue.
> > > 
> > > That said, simply copying the libdbus constants would be more
> straightforward:
> > > 
> > > enum ServiceOwnershipOptions {
> > >   ALLOW_REPLACEMENT = DBUS_NAME_FLAG_ALLOW_REPLACEMENT,
> > >   REPLACE_EXISTING = DBUS_NAME_FLAG_REPLACE_EXISTING,
> > >   DO_NOT_QUEUE = DBUS_NAME_FLAG_DO_NOT_QUEUE,
> > > }
> > > 
> > > Then RequestOwnership() would take 'int options' which is an OR'ed value
of
> > > ServiceOwnershipOptions.
> > 
> > I thought about doing that, but wasn't sure if it made sense to have callers
> be
> > OR'ing together enum values.  That only works when the values are 0x1, 0x2,
> 0x4
> > etc (which they are here), but that's sort of an implementation detail. 
That
> > was why I went with these semantically higher-level enum's.  I'm not married
> to
> > this approach, though, if you think that the other way is better.
> 
> That's a good point. I think ALLOW_REPLACEMENT alone won't be used in the
> caller, so what about just providing the two options:
> 
> enum ServiceOwnershipOptions {
>   REQUIRE_PRIMARY = (DBUS_NAME_FLAG_DO_NOT_QUEUE |
> DBUS_NAME_FLAG_REPLACE_EXISTING),
>   REQUIRE_PRIMARY_ALLOW_REPLACEMENT = (REQUIRE_PRIMARY |
> DBUS_NAME_FLAG_ALLOW_REPLACEMENT)
> }
> 
> This would suffice for now. We can switch to the OR-ed value approach if
needed
> at a later time.
> 
> 
> 

Done.

https://codereview.chromium.org/20555003/diff/1/dbus/signal_sender_verificati...
File dbus/signal_sender_verification_unittest.cc (right):

https://codereview.chromium.org/20555003/diff/1/dbus/signal_sender_verificati...
dbus/signal_sender_verification_unittest.cc:272: LOG(INFO) << "Spun up new test
service";
On 2013/07/26 00:50:12, satorux1 wrote:
> please remove this.

Done.

[satorux1, 2013-07-29 05:50:00]

LGTM with nits:

https://codereview.chromium.org/20555003/diff/26001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/dbus/cros_dbus_service.cc (right):

https://codereview.chromium.org/20555003/diff/26001/chrome/browser/chromeos/d...
chrome/browser/chromeos/dbus/cros_dbus_service.cc:53: // where processes on
Linux can wind up stuck in an uninterruptible state
stack?

https://codereview.chromium.org/20555003/diff/26001/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/20555003/diff/26001/dbus/bus.cc#newcode575
dbus/bus.cc:575: << (error.is_set() ? error.message() : "") << ", result: "
result -> result code to be a bit more descriptive?

https://codereview.chromium.org/20555003/diff/26001/dbus/bus.cc#newcode576
dbus/bus.cc:576: << res_str;
No need to use StringPrintf() to print |result| with LOG()

https://codereview.chromium.org/20555003/diff/26001/dbus/signal_sender_verifi...
File dbus/signal_sender_verification_unittest.cc (right):

https://codereview.chromium.org/20555003/diff/26001/dbus/signal_sender_verifi...
dbus/signal_sender_verification_unittest.cc:313:
SafeServiceStop(&stealable_test_service);
Maybe just:

base::ThreadRestrictions::ScopedAllowIO allow_io;
stealable_test_service->Stop();

[Chris Masone, 2013-07-29 16:39:27]

https://codereview.chromium.org/20555003/diff/26001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/dbus/cros_dbus_service.cc (right):

https://codereview.chromium.org/20555003/diff/26001/chrome/browser/chromeos/d...
chrome/browser/chromeos/dbus/cros_dbus_service.cc:53: // where processes on
Linux can wind up stuck in an uninterruptible state
On 2013/07/29 05:50:01, satorux1 wrote:
> stack?

We don't have any stack traces, as the processes do not respond to signals in
this situation, so we can't get any kind of crash report.  We have been able to
record process state, though, and they're in the 'D' state.

https://codereview.chromium.org/20555003/diff/26001/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/20555003/diff/26001/dbus/bus.cc#newcode575
dbus/bus.cc:575: << (error.is_set() ? error.message() : "") << ", result: "
On 2013/07/29 05:50:01, satorux1 wrote:
> result -> result code to be a bit more descriptive?

Done.

https://codereview.chromium.org/20555003/diff/26001/dbus/bus.cc#newcode576
dbus/bus.cc:576: << res_str;
On 2013/07/29 05:50:01, satorux1 wrote:
> No need to use StringPrintf() to print |result| with LOG()

Done.

https://codereview.chromium.org/20555003/diff/26001/dbus/signal_sender_verifi...
File dbus/signal_sender_verification_unittest.cc (right):

https://codereview.chromium.org/20555003/diff/26001/dbus/signal_sender_verifi...
dbus/signal_sender_verification_unittest.cc:313:
SafeServiceStop(&stealable_test_service);
On 2013/07/29 05:50:01, satorux1 wrote:
> Maybe just:
> 
> base::ThreadRestrictions::ScopedAllowIO allow_io;
> stealable_test_service->Stop();

The presubmit check indicates that is a banned function

[commit-bot: I haz the power, 2013-07-29 16:44:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cmasone@chromium.org/20555003/39001

[commit-bot: I haz the power, 2013-07-30 03:14:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cmasone@chromium.org/20555003/39001

[commit-bot: I haz the power, 2013-07-30 14:36:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cmasone@chromium.org/20555003/39001

[commit-bot: I haz the power, 2013-07-31 06:35:03]

Change committed as 214589