dbus: allow unregistering of exported objects

dbus: allow unregistering of exported objects

Not all objects are permanent, some are transient and if we ever
re-use the object path, we want a new instance of the exported object
to be created rather than re-use an existing one.
    
BUG=chromium-os:21320
TEST=dbus_unittests and included change to agent service provider

Change-Id: I09882bbe2f70356182ac301c4260473051333424


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126527

[satorux1, 2012-03-13 06:07:06]

http://codereview.chromium.org/9691025/diff/1/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/9691025/diff/1/dbus/bus.h#newcode236
dbus/bus.h:236: virtual void UnregisterExportedObject(const ObjectPath&
object_path);
This doesn't take a callback, so clients don't know when the object is
unregistered. is it ok?

[keybuk, 2012-03-13 16:01:04]

http://codereview.chromium.org/9691025/diff/1/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/9691025/diff/1/dbus/bus.h#newcode236
dbus/bus.h:236: virtual void UnregisterExportedObject(const ObjectPath&
object_path);
The most likely place for this to be called is from a service provider
destructor, so a callback doesn't make sense. Though you make a point that this
introduces potentials for races, I'll adjust it a little so it doesn't have that

[satorux1, 2012-03-13 18:27:25]

http://codereview.chromium.org/9691025/diff/5002/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/9691025/diff/5002/dbus/bus.h#newcode235
dbus/bus.h:235: // BLOCKING CALL. Must be called in the origin thread.
On what thread this function is called? For instance, we shouldn't block the UI
thread.

[keybuk, 2012-03-13 18:32:41]

http://codereview.chromium.org/9691025/diff/5002/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/9691025/diff/5002/dbus/bus.h#newcode235
dbus/bus.h:235: // BLOCKING CALL. Must be called in the origin thread.
probably the UI thread ... alternate suggestions welcome

[satorux1, 2012-03-13 18:42:01]

http://codereview.chromium.org/9691025/diff/5002/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/9691025/diff/5002/dbus/bus.h#newcode235
dbus/bus.h:235: // BLOCKING CALL. Must be called in the origin thread.
On 2012/03/13 18:32:41, keybuk wrote:
> probably the UI thread ... alternate suggestions welcome

On 2012/03/13 18:27:25, satorux1 wrote:
> http://codereview.chromium.org/9691025/diff/5002/dbus/bus.h
> File dbus/bus.h (right):
> 
> http://codereview.chromium.org/9691025/diff/5002/dbus/bus.h#newcode235
> dbus/bus.h:235: // BLOCKING CALL. Must be called in the origin thread.
> On what thread this function is called? For instance, we shouldn't block the
UI
> thread.

maybe:

1) add a callback parameter to UnregisterExportedObject, and let
BluetoothAgentServiceProviderImpl use it properly, before the destructor is
called. would it be difficult?

2) rather than block in UnregisterExportedObject, block in
Bus::ShutdownAndBlockOnDBusThreadInternal? we can probably do this by
dbus_thread_message_loop_->RunAllPendings()

[keybuk, 2012-03-13 18:53:31]

http://codereview.chromium.org/9691025/diff/5002/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/9691025/diff/5002/dbus/bus.h#newcode235
dbus/bus.h:235: // BLOCKING CALL. Must be called in the origin thread.
1) there's no place to call because the destructor is called as a result of the
scoped_ptr<> freeing the contained object - there isn't a sensible shutdown path
here

2) this isn't shutdown, this is unregistering an object without shutting down
the bus

[satorux1, 2012-03-13 18:56:35]

http://codereview.chromium.org/9691025/diff/5002/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/9691025/diff/5002/dbus/bus.h#newcode235
dbus/bus.h:235: // BLOCKING CALL. Must be called in the origin thread.
On 2012/03/13 18:53:31, keybuk wrote:
> 1) there's no place to call because the destructor is called as a result of
the
> scoped_ptr<> freeing the contained object - there isn't a sensible shutdown
path
> here
> 
> 2) this isn't shutdown, this is unregistering an object without shutting down
> the bus

re 2, i meant to just post tasks to d-bus thread from
UnregisterExportedObject(), and make sure all of these tasks
were run at the shutdown time.

[keybuk, 2012-03-13 18:58:33]

2) they have to be run *before* the next call to bus::TryRegisterObjectPath
(called under ExportedObject::ExportMethod)

otherwise the attempt to create a new exported method fails because the
method/object is already registered, because the old one hasn't been
unregistered yet

[satorux1, 2012-03-13 20:15:33]

LGTM with a nit. Looks safe now.

http://codereview.chromium.org/9691025/diff/7004/dbus/bus.cc
File dbus/bus.cc (right):

http://codereview.chromium.org/9691025/diff/7004/dbus/bus.cc#newcode267
dbus/bus.cc:267: // loop proxy is a SequencedTaskRunner, there is a guarantee
that this will
I was confused at first. might want to add something like

// loop proxy is a SequencedTaskRunner (i.e. MessageLoopProxy inherits from
SequencedTaskRunner)

[keybuk, 2012-03-13 20:25:01]

https://chromiumcodereview.appspot.com/9691025/diff/7004/dbus/bus.cc
File dbus/bus.cc (right):

https://chromiumcodereview.appspot.com/9691025/diff/7004/dbus/bus.cc#newcode267
dbus/bus.cc:267: // loop proxy is a SequencedTaskRunner, there is a guarantee
that this will
On 2012/03/13 20:15:33, satorux1 wrote:
> I was confused at first. might want to add something like
> 
> // loop proxy is a SequencedTaskRunner (i.e. MessageLoopProxy inherits from
> SequencedTaskRunner)

Done.

[commit-bot: I haz the power, 2012-03-13 20:26:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keybuk@chromium.org/9691025/1005

[commit-bot: I haz the power, 2012-03-14 01:18:44]

Change committed as 126527