cashew: do not delete DBus::ObjectProxy objects from D-Bus callbacks

cashew: do not delete DBus::ObjectProxy objects from D-Bus callbacks

This CL resolves a deadlock that causes cashew to hang indefinitely.

Deleting a subclass of DBus::ObjectProxy such as Service or Device
causes an unregister message to be sent to the bus daemon, and is
effectively the same as explicitly sending a D-Bus message from within a
D-Bus callback. As such, it can trigger the same libdbus-c++ deadlock
that caused issues 8516 and 8486.

We avoid this by scheduling deferred deletion of these objects from the
main loop.

BUG=chromium-os:8892
TEST=manual testing on device (suspend/resume while connected to 3G)

Change-Id: I5882bb581a2b3edb440954d1c32a41fef48ac48d

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=35386a9

[Daniel Kurtz, 2010-11-24 02:33:46]

Can you make a single 'defered object deleter' that can delete any DBus proxy
object while 'not during a DBus callback'?

http://codereview.chromium.org/5333003/diff/1/src/service.cc
File src/service.cc (right):

http://codereview.chromium.org/5333003/diff/1/src/service.cc#newcode478
src/service.cc:478: DLOG(INFO) << path_ << ": DeletePendingDevices: deleting
device: "
Can you use __func__ or __FUNCTION__ instead of typing it manually?
Typing manually everytime seems a bit brittle.

http://codereview.chromium.org/5333003/diff/1/src/service.cc#newcode515
src/service.cc:515: CHECK_NOTNULL(service);
DCHECK(service != NULL); ??

http://codereview.chromium.org/5333003/diff/1/src/service.cc#newcode582
src/service.cc:582: ScheduleDeviceForLaterDeletion(device_);
save a ref to device_, then NULL device_, then ScheduleForDelete

http://codereview.chromium.org/5333003/diff/1/src/service_manager.cc
File src/service_manager.cc (right):

http://codereview.chromium.org/5333003/diff/1/src/service_manager.cc#newcode206
src/service_manager.cc:206: void ServiceManager::DeletePendingServices() {
Why not just services_pending_deletion_.clear() ?

All the elements in the container are dropped: their destructors are called, and
then they are removed from the container, leaving it with a size of 0.

http://codereview.chromium.org/5333003/diff/1/src/service_manager.cc#newcode232
src/service_manager.cc:232: if (!g_main_loop_is_running(main_loop_)) {
Break this into two methods:
1) always defers, and can be called while main_loop is running.
2) a pure deconstructor that only gets called after main_loop is terminated.

[Vince Laviano, 2010-11-24 04:48:34]

PTAL

http://codereview.chromium.org/5333003/diff/1/src/service.cc
File src/service.cc (right):

http://codereview.chromium.org/5333003/diff/1/src/service.cc#newcode478
src/service.cc:478: DLOG(INFO) << path_ << ": DeletePendingDevices: deleting
device: "
On 2010/11/24 02:33:47, djkurtz wrote:
> Can you use __func__ or __FUNCTION__ instead of typing it manually?
> Typing manually everytime seems a bit brittle.

I'll look at doing this cashew-wide in the future. It would probably reduce the
executable size for debug builds.

http://codereview.chromium.org/5333003/diff/1/src/service.cc#newcode515
src/service.cc:515: CHECK_NOTNULL(service);
On 2010/11/24 02:33:47, djkurtz wrote:
> DCHECK(service != NULL); ??

Done.

http://codereview.chromium.org/5333003/diff/1/src/service.cc#newcode582
src/service.cc:582: ScheduleDeviceForLaterDeletion(device_);
On 2010/11/24 02:33:47, djkurtz wrote:
> save a ref to device_, then NULL device_, then ScheduleForDelete

Done.

http://codereview.chromium.org/5333003/diff/1/src/service_manager.cc
File src/service_manager.cc (right):

http://codereview.chromium.org/5333003/diff/1/src/service_manager.cc#newcode206
src/service_manager.cc:206: void ServiceManager::DeletePendingServices() {
On 2010/11/24 02:33:47, djkurtz wrote:
> Why not just services_pending_deletion_.clear() ?
> 
> All the elements in the container are dropped: their destructors are called,
and
> then they are removed from the container, leaving it with a size of 0.

The elements are pointers to Services. This suggestion would work if the
elements were Services, but destructing the pointers is not the same as
destructing the Services to which they point.

http://codereview.chromium.org/5333003/diff/1/src/service_manager.cc#newcode232
src/service_manager.cc:232: if (!g_main_loop_is_running(main_loop_)) {
On 2010/11/24 02:33:47, djkurtz wrote:
> Break this into two methods:
> 1) always defers, and can be called while main_loop is running.
> 2) a pure deconstructor that only gets called after main_loop is terminated.

Done. (Good call.)

[Daniel Kurtz, 2010-11-24 05:05:48]

LGTM

http://codereview.chromium.org/5333003/diff/9001/src/main.cc
File src/main.cc (right):

http://codereview.chromium.org/5333003/diff/9001/src/main.cc#newcode51
src/main.cc:51: // doing it directly here to remove some low severity races
Is this still true?

[Vince Laviano, 2010-11-24 05:14:53]

On Tue, Nov 23, 2010 at 9:05 PM, <djkurtz@chromium.org> wrote:

> LGTM
>
>
> http://codereview.chromium.org/5333003/diff/9001/src/main.cc
> File src/main.cc (right):
>
> http://codereview.chromium.org/5333003/diff/9001/src/main.cc#newcode51
> src/main.cc:51: // doing it directly here to remove some low severity
> races
> Is this still true?


Yes. A signal at the wrong time could allow deletion of the CashewServer or
ServiceManager instances while the main loop is still running despite the
asserts in their dtors that check if the main loop is running.  However,
such deletions would be programming errors that should never occur rather
than events that are expected to occur during normal operation, so I see the
presence of this race as being of low severity. That being said, it's on my
list of things to fix.


>
>
> http://codereview.chromium.org/5333003/
>