Delete D-Bus clients before shutting down the system bus

Delete D-Bus clients before shutting down the system bus

Previously, D-Bus clients were deleted after the system bus was
shut down. This order was not desirable because D-Bus clients
could access the system bus in their destructors, even though the
system bus was already shut down.

BUG=344270
TEST=Chrome shuts down cleanly as before

[satorux1, 2014-02-17 06:15:42]

There is no rush on this patch. We should hold off on this patch if it would
make it difficult to create a patch for issue 342454.

[stevenjb, 2014-02-18 17:37:03]

lgtm w/ nits

https://codereview.chromium.org/166593005/diff/1/chromeos/dbus/dbus_thread_ma...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/166593005/diff/1/chromeos/dbus/dbus_thread_ma...
chromeos/dbus/dbus_thread_manager.cc:59: struct DBusClientBundle {
Since this has non-trivial behavior we should make it a class.

https://codereview.chromium.org/166593005/diff/1/chromeos/dbus/dbus_thread_ma...
chromeos/dbus/dbus_thread_manager.cc:63: REAL_DBUS_CLIENT_IMPLEMENTATION;
= client_type might be more clear?

[satorux1, 2014-02-19 04:16:30]

https://codereview.chromium.org/166593005/diff/1/chromeos/dbus/dbus_thread_ma...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/166593005/diff/1/chromeos/dbus/dbus_thread_ma...
chromeos/dbus/dbus_thread_manager.cc:59: struct DBusClientBundle {
On 2014/02/18 17:37:03, stevenjb wrote:
> Since this has non-trivial behavior we should make it a class.

Done.

https://codereview.chromium.org/166593005/diff/1/chromeos/dbus/dbus_thread_ma...
chromeos/dbus/dbus_thread_manager.cc:63: REAL_DBUS_CLIENT_IMPLEMENTATION;
On 2014/02/18 17:37:03, stevenjb wrote:
> = client_type might be more clear?

Done.

[hashimoto, 2014-02-19 04:36:57]

lgtm

https://codereview.chromium.org/166593005/diff/110001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/166593005/diff/110001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:418: scoped_refptr<dbus::Bus> system_bus_;
suggestion: How about putting |dbus_thread_| and |system_bus_| in a bundle class
instead of creating a bundle for clients and duplicating the all getters?

[satorux1, 2014-02-19 05:11:34]

https://codereview.chromium.org/166593005/diff/110001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/166593005/diff/110001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:418: scoped_refptr<dbus::Bus> system_bus_;
On 2014/02/19 04:36:57, hashimoto wrote:
> suggestion: How about putting |dbus_thread_| and |system_bus_| in a bundle
class
> instead of creating a bundle for clients and duplicating the all getters?

If we create a bundle for |dbus_thread_| and |system_bus_|, we need to delete
the bundle after all clients are deleted, which seems to be tricky to do. Maybe
I'm misunderstanding your suggestion, though.

[hashimoto, 2014-02-19 05:31:43]

https://codereview.chromium.org/166593005/diff/110001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/166593005/diff/110001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:418: scoped_refptr<dbus::Bus> system_bus_;
On 2014/02/19 05:11:35, satorux at chromium.org wrote:
> On 2014/02/19 04:36:57, hashimoto wrote:
> > suggestion: How about putting |dbus_thread_| and |system_bus_| in a bundle
> class
> > instead of creating a bundle for clients and duplicating the all getters?
> 
> If we create a bundle for |dbus_thread_| and |system_bus_|, we need to delete
> the bundle after all clients are deleted, which seems to be tricky to do.
Maybe
> I'm misunderstanding your suggestion, though.

I meant to create a bundle for the thread and the bus, doing the sanitary works
in the bundle's dtor.
This way, what you should do is just putting scoped_ptr of the bundle before the
all clients, so that DBusThreadManager's dtor (it would be almost empty, no need
to manually reset scoped_ptrs) does the clean up in the right order just by
destructing the member variables.

[satorux1, 2014-02-19 05:41:12]

On 2014/02/19 05:31:43, hashimoto wrote:
>
https://codereview.chromium.org/166593005/diff/110001/chromeos/dbus/dbus_thre...
> File chromeos/dbus/dbus_thread_manager.cc (right):
> 
>
https://codereview.chromium.org/166593005/diff/110001/chromeos/dbus/dbus_thre...
> chromeos/dbus/dbus_thread_manager.cc:418: scoped_refptr<dbus::Bus>
system_bus_;
> On 2014/02/19 05:11:35, satorux at http://chromium.org wrote:
> > On 2014/02/19 04:36:57, hashimoto wrote:
> > > suggestion: How about putting |dbus_thread_| and |system_bus_| in a bundle
> > class
> > > instead of creating a bundle for clients and duplicating the all getters?
> > 
> > If we create a bundle for |dbus_thread_| and |system_bus_|, we need to
delete
> > the bundle after all clients are deleted, which seems to be tricky to do.
> Maybe
> > I'm misunderstanding your suggestion, though.
> 
> I meant to create a bundle for the thread and the bus, doing the sanitary
works
> in the bundle's dtor.
> This way, what you should do is just putting scoped_ptr of the bundle before
the
> all clients, so that DBusThreadManager's dtor (it would be almost empty, no
need
> to manually reset scoped_ptrs) does the clean up in the right order just by
> destructing the member variables.

Ah that's clever! That'd be less code, but that bundle may be conceptually a bit
awkward (BundleOfThreadAndBus?). So I'd go with the client bundle approach.
Thank you for the suggestion anyway.

[satorux1, 2014-02-19 05:41:14]

On 2014/02/19 05:31:43, hashimoto wrote:
>
https://codereview.chromium.org/166593005/diff/110001/chromeos/dbus/dbus_thre...
> File chromeos/dbus/dbus_thread_manager.cc (right):
> 
>
https://codereview.chromium.org/166593005/diff/110001/chromeos/dbus/dbus_thre...
> chromeos/dbus/dbus_thread_manager.cc:418: scoped_refptr<dbus::Bus>
system_bus_;
> On 2014/02/19 05:11:35, satorux at http://chromium.org wrote:
> > On 2014/02/19 04:36:57, hashimoto wrote:
> > > suggestion: How about putting |dbus_thread_| and |system_bus_| in a bundle
> > class
> > > instead of creating a bundle for clients and duplicating the all getters?
> > 
> > If we create a bundle for |dbus_thread_| and |system_bus_|, we need to
delete
> > the bundle after all clients are deleted, which seems to be tricky to do.
> Maybe
> > I'm misunderstanding your suggestion, though.
> 
> I meant to create a bundle for the thread and the bus, doing the sanitary
works
> in the bundle's dtor.
> This way, what you should do is just putting scoped_ptr of the bundle before
the
> all clients, so that DBusThreadManager's dtor (it would be almost empty, no
need
> to manually reset scoped_ptrs) does the clean up in the right order just by
> destructing the member variables.

Ah that's clever! That'd be less code, but that bundle may be conceptually a bit
awkward (BundleOfThreadAndBus?). So I'd go with the client bundle approach.
Thank you for the suggestion anyway.

[hashimoto, 2014-02-19 05:56:56]

On 2014/02/19 05:41:14, satorux at chromium.org wrote:
> On 2014/02/19 05:31:43, hashimoto wrote:
> >
>
https://codereview.chromium.org/166593005/diff/110001/chromeos/dbus/dbus_thre...
> > File chromeos/dbus/dbus_thread_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/166593005/diff/110001/chromeos/dbus/dbus_thre...
> > chromeos/dbus/dbus_thread_manager.cc:418: scoped_refptr<dbus::Bus>
> system_bus_;
> > On 2014/02/19 05:11:35, satorux at http://chromium.org wrote:
> > > On 2014/02/19 04:36:57, hashimoto wrote:
> > > > suggestion: How about putting |dbus_thread_| and |system_bus_| in a
bundle
> > > class
> > > > instead of creating a bundle for clients and duplicating the all
getters?
> > > 
> > > If we create a bundle for |dbus_thread_| and |system_bus_|, we need to
> delete
> > > the bundle after all clients are deleted, which seems to be tricky to do.
> > Maybe
> > > I'm misunderstanding your suggestion, though.
> > 
> > I meant to create a bundle for the thread and the bus, doing the sanitary
> works
> > in the bundle's dtor.
> > This way, what you should do is just putting scoped_ptr of the bundle before
> the
> > all clients, so that DBusThreadManager's dtor (it would be almost empty, no
> need
> > to manually reset scoped_ptrs) does the clean up in the right order just by
> > destructing the member variables.
> 
> Ah that's clever! That'd be less code, but that bundle may be conceptually a
bit
> awkward (BundleOfThreadAndBus?). So I'd go with the client bundle approach.
> Thank you for the suggestion anyway.

It also makes sense.
lgtm

[satorux1, 2014-02-19 08:20:06]

The CQ bit was checked by satorux@chromium.org

[commit-bot: I haz the power, 2014-02-19 08:20:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/satorux@chromium.org/166593005/110001

[commit-bot: I haz the power, 2014-02-19 08:48:48]

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

[commit-bot: I haz the power, 2014-02-19 08:48:48]

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

[satorux1, 2014-02-24 06:13:26]

On 2014/02/19 08:48:48, I haz the power (commit-bot) wrote:
> Retried try job too often on win_rel for step(s) unit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Submitted: https://code.google.com/p/chromium/issues/detail?id=344270#c4
Closing...