Delete D-Bus clients before shutting down the system bus

chromeos/dbus/dbus_thread_manager.cc

Index: chromeos/dbus/dbus_thread_manager.cc
diff --git a/chromeos/dbus/dbus_thread_manager.cc b/chromeos/dbus/dbus_thread_manager.cc
index 91a474d0014b8f40204929223c216f5e8877c434..d930567b1bccdbd01adab6209dcf8f9c0dc304ce 100644
--- a/chromeos/dbus/dbus_thread_manager.cc
+++ b/chromeos/dbus/dbus_thread_manager.cc
@@ -53,6 +53,183 @@ namespace chromeos {
 static DBusThreadManager* g_dbus_thread_manager = NULL;
 static DBusThreadManager* g_dbus_thread_manager_for_testing = NULL;
 
+// The bundle of all D-Bus clients used in DBusThreadManagerImpl. The bundle
+// is used to delete them at once in the right order before shutting down the
+// system bus. See also the comment in the destructor of DBusThreadManagerImpl.
+class DBusClientBundle {
+ public:
+  DBusClientBundle() {
+    DBusClientImplementationType client_type = REAL_DBUS_CLIENT_IMPLEMENTATION;
+    DBusClientImplementationType client_type_override = client_type;
+    // If --dbus-stub was requested, pass STUB to specific components;
+    // Many components like login are not useful with a stub implementation.
+    if (CommandLine::ForCurrentProcess()->HasSwitch(
+            chromeos::switches::kDbusStub)) {
+      client_type_override = STUB_DBUS_CLIENT_IMPLEMENTATION;
+    }
+
+    bluetooth_adapter_client_.reset(BluetoothAdapterClient::Create());
+    bluetooth_agent_manager_client_.reset(
+        BluetoothAgentManagerClient::Create());
+    bluetooth_device_client_.reset(BluetoothDeviceClient::Create());
+    bluetooth_input_client_.reset(BluetoothInputClient::Create());
+    bluetooth_profile_manager_client_.reset(
+        BluetoothProfileManagerClient::Create());
+    cras_audio_client_.reset(CrasAudioClient::Create());
+    cros_disks_client_.reset(CrosDisksClient::Create(client_type));
+    cryptohome_client_.reset(CryptohomeClient::Create());
+    debug_daemon_client_.reset(DebugDaemonClient::Create());
+    shill_manager_client_.reset(ShillManagerClient::Create());
+    shill_device_client_.reset(ShillDeviceClient::Create());
+    shill_ipconfig_client_.reset(ShillIPConfigClient::Create());
+    shill_service_client_.reset(ShillServiceClient::Create());
+    shill_profile_client_.reset(ShillProfileClient::Create());
+    gsm_sms_client_.reset(GsmSMSClient::Create());
+    image_burner_client_.reset(ImageBurnerClient::Create());
+    introspectable_client_.reset(IntrospectableClient::Create());
+    modem_messaging_client_.reset(ModemMessagingClient::Create());
+    // Create the NFC clients in the correct order based on their dependencies.
+    nfc_manager_client_.reset(NfcManagerClient::Create());
+    nfc_adapter_client_.reset(
+        NfcAdapterClient::Create(nfc_manager_client_.get()));
+    nfc_device_client_.reset(
+        NfcDeviceClient::Create(nfc_adapter_client_.get()));
+    nfc_tag_client_.reset(NfcTagClient::Create(nfc_adapter_client_.get()));
+    nfc_record_client_.reset(NfcRecordClient::Create(nfc_device_client_.get(),
+                                                    nfc_tag_client_.get()));
+    permission_broker_client_.reset(PermissionBrokerClient::Create());
+    power_manager_client_.reset(
+        PowerManagerClient::Create(client_type_override));
+    session_manager_client_.reset(SessionManagerClient::Create(client_type));
+    sms_client_.reset(SMSClient::Create());
+    system_clock_client_.reset(SystemClockClient::Create());
+    update_engine_client_.reset(UpdateEngineClient::Create(client_type));
+  }
+
+  BluetoothAdapterClient* bluetooth_adapter_client() {
+    return bluetooth_adapter_client_.get();
+  }
+  BluetoothAgentManagerClient* bluetooth_agent_manager_client() {
+    return bluetooth_agent_manager_client_.get();
+  }
+  BluetoothDeviceClient* bluetooth_device_client() {
+    return bluetooth_device_client_.get();
+  }
+  BluetoothInputClient* bluetooth_input_client() {
+    return bluetooth_input_client_.get();
+  }
+  BluetoothProfileManagerClient* bluetooth_profile_manager_client() {
+    return bluetooth_profile_manager_client_.get();
+  }
+  CrasAudioClient* cras_audio_client() {
+    return cras_audio_client_.get();
+  }
+  CrosDisksClient* cros_disks_client() {
+    return cros_disks_client_.get();
+  }
+  CryptohomeClient* cryptohome_client() {
+    return cryptohome_client_.get();
+  }
+  DebugDaemonClient* debug_daemon_client() {
+    return debug_daemon_client_.get();
+  }
+  ShillDeviceClient* shill_device_client() {
+    return shill_device_client_.get();
+  }
+  ShillIPConfigClient* shill_ipconfig_client() {
+    return shill_ipconfig_client_.get();
+  }
+  ShillManagerClient* shill_manager_client() {
+    return shill_manager_client_.get();
+  }
+  ShillServiceClient* shill_service_client() {
+    return shill_service_client_.get();
+  }
+  ShillProfileClient* shill_profile_client() {
+    return shill_profile_client_.get();
+  }
+  GsmSMSClient* gsm_sms_client() {
+    return gsm_sms_client_.get();
+  }
+  ImageBurnerClient* image_burner_client() {
+    return image_burner_client_.get();
+  }
+  IntrospectableClient* introspectable_client() {
+    return introspectable_client_.get();
+  }
+  ModemMessagingClient* modem_messaging_client() {
+    return modem_messaging_client_.get();
+  }
+  NfcManagerClient* nfc_manager_client() {
+    return nfc_manager_client_.get();
+  }
+  NfcAdapterClient* nfc_adapter_client() {
+    return nfc_adapter_client_.get();
+  }
+  NfcDeviceClient* nfc_device_client() {
+    return nfc_device_client_.get();
+  }
+  NfcTagClient* nfc_tag_client() {
+    return nfc_tag_client_.get();
+  }
+  NfcRecordClient* nfc_record_client() {
+    return nfc_record_client_.get();
+  }
+  PermissionBrokerClient* permission_broker_client() {
+    return permission_broker_client_.get();
+  }
+  SystemClockClient* system_clock_client() {
+    return system_clock_client_.get();
+  }
+  PowerManagerClient* power_manager_client() {
+    return power_manager_client_.get();
+  }
+  SessionManagerClient* session_manager_client() {
+    return session_manager_client_.get();
+  }
+  SMSClient* sms_client() {
+    return sms_client_.get();
+  }
+  UpdateEngineClient* update_engine_client() {
+    return update_engine_client_.get();
+  }
+
+ private:
+  scoped_ptr<BluetoothAdapterClient> bluetooth_adapter_client_;
+  scoped_ptr<BluetoothAgentManagerClient> bluetooth_agent_manager_client_;
+  scoped_ptr<BluetoothDeviceClient> bluetooth_device_client_;
+  scoped_ptr<BluetoothInputClient> bluetooth_input_client_;
+  scoped_ptr<BluetoothProfileManagerClient> bluetooth_profile_manager_client_;
+  scoped_ptr<CrasAudioClient> cras_audio_client_;
+  scoped_ptr<CrosDisksClient> cros_disks_client_;
+  scoped_ptr<CryptohomeClient> cryptohome_client_;
+  scoped_ptr<DebugDaemonClient> debug_daemon_client_;
+  scoped_ptr<ShillDeviceClient> shill_device_client_;
+  scoped_ptr<ShillIPConfigClient> shill_ipconfig_client_;
+  scoped_ptr<ShillManagerClient> shill_manager_client_;
+  scoped_ptr<ShillServiceClient> shill_service_client_;
+  scoped_ptr<ShillProfileClient> shill_profile_client_;
+  scoped_ptr<GsmSMSClient> gsm_sms_client_;
+  scoped_ptr<ImageBurnerClient> image_burner_client_;
+  scoped_ptr<IntrospectableClient> introspectable_client_;
+  scoped_ptr<ModemMessagingClient> modem_messaging_client_;
+  // The declaration order for NFC client objects is important. See
+  // DBusThreadManager::CreateDefaultClients for the dependencies.
+  scoped_ptr<NfcManagerClient> nfc_manager_client_;
+  scoped_ptr<NfcAdapterClient> nfc_adapter_client_;
+  scoped_ptr<NfcDeviceClient> nfc_device_client_;
+  scoped_ptr<NfcTagClient> nfc_tag_client_;
+  scoped_ptr<NfcRecordClient> nfc_record_client_;
+  scoped_ptr<PermissionBrokerClient> permission_broker_client_;
+  scoped_ptr<SystemClockClient> system_clock_client_;
+  scoped_ptr<PowerManagerClient> power_manager_client_;
+  scoped_ptr<SessionManagerClient> session_manager_client_;
+  scoped_ptr<SMSClient> sms_client_;
+  scoped_ptr<UpdateEngineClient> update_engine_client_;
+
+  DISALLOW_COPY_AND_ASSIGN(DBusClientBundle);
+};
+
 // The DBusThreadManager implementation used in production.
 class DBusThreadManagerImpl : public DBusThreadManager {
  public:
@@ -77,6 +254,9 @@ class DBusThreadManagerImpl : public DBusThreadManager {
     FOR_EACH_OBSERVER(DBusThreadManagerObserver, observers_,
                       OnDBusThreadManagerDestroying(this));
 
+    // Delete all D-Bus clients before shutting down the system bus.
+    client_bundle_.reset();
+
     // Shut down the bus. During the browser shutdown, it's ok to shut down
     // the bus synchronously.
     system_bus_->ShutdownOnDBusThreadAndBlock();
@@ -101,178 +281,132 @@ class DBusThreadManagerImpl : public DBusThreadManager {
   }
 
   virtual BluetoothAdapterClient* GetBluetoothAdapterClient() OVERRIDE {
-    return bluetooth_adapter_client_.get();
+    return client_bundle_->bluetooth_adapter_client();
   }
 
   virtual BluetoothAgentManagerClient* GetBluetoothAgentManagerClient()
       OVERRIDE {
-    return bluetooth_agent_manager_client_.get();
+    return client_bundle_->bluetooth_agent_manager_client();
   }
 
   virtual BluetoothDeviceClient* GetBluetoothDeviceClient() OVERRIDE {
-    return bluetooth_device_client_.get();
+    return client_bundle_->bluetooth_device_client();
   }
 
   virtual BluetoothInputClient* GetBluetoothInputClient() OVERRIDE {
-    return bluetooth_input_client_.get();
+    return client_bundle_->bluetooth_input_client();
   }
 
   virtual BluetoothProfileManagerClient* GetBluetoothProfileManagerClient()
       OVERRIDE {
-    return bluetooth_profile_manager_client_.get();
+    return client_bundle_->bluetooth_profile_manager_client();
   }
 
   virtual CrasAudioClient* GetCrasAudioClient() OVERRIDE {
-    return cras_audio_client_.get();
+    return client_bundle_->cras_audio_client();
   }
 
   virtual CrosDisksClient* GetCrosDisksClient() OVERRIDE {
-    return cros_disks_client_.get();
+    return client_bundle_->cros_disks_client();
   }
 
   virtual CryptohomeClient* GetCryptohomeClient() OVERRIDE {
-    return cryptohome_client_.get();
+    return client_bundle_->cryptohome_client();
   }
 
   virtual DebugDaemonClient* GetDebugDaemonClient() OVERRIDE {
-    return debug_daemon_client_.get();
+    return client_bundle_->debug_daemon_client();
   }
 
   virtual ShillDeviceClient* GetShillDeviceClient() OVERRIDE {
-    return shill_device_client_.get();
+    return client_bundle_->shill_device_client();
   }
 
   virtual ShillIPConfigClient* GetShillIPConfigClient() OVERRIDE {
-    return shill_ipconfig_client_.get();
+    return client_bundle_->shill_ipconfig_client();
   }
 
   virtual ShillManagerClient* GetShillManagerClient() OVERRIDE {
-    return shill_manager_client_.get();
+    return client_bundle_->shill_manager_client();
   }
 
   virtual ShillServiceClient* GetShillServiceClient() OVERRIDE {
-    return shill_service_client_.get();
+    return client_bundle_->shill_service_client();
   }
 
   virtual ShillProfileClient* GetShillProfileClient() OVERRIDE {
-    return shill_profile_client_.get();
+    return client_bundle_->shill_profile_client();
   }
 
   virtual GsmSMSClient* GetGsmSMSClient() OVERRIDE {
-    return gsm_sms_client_.get();
+    return client_bundle_->gsm_sms_client();
   }
 
   virtual ImageBurnerClient* GetImageBurnerClient() OVERRIDE {
-    return image_burner_client_.get();
+    return client_bundle_->image_burner_client();
   }
 
   virtual IntrospectableClient* GetIntrospectableClient() OVERRIDE {
-    return introspectable_client_.get();
+    return client_bundle_->introspectable_client();
   }
 
   virtual ModemMessagingClient* GetModemMessagingClient() OVERRIDE {
-    return modem_messaging_client_.get();
+    return client_bundle_->modem_messaging_client();
   }
 
   virtual NfcAdapterClient* GetNfcAdapterClient() OVERRIDE {
-    return nfc_adapter_client_.get();
+    return client_bundle_->nfc_adapter_client();
   }
 
   virtual NfcDeviceClient* GetNfcDeviceClient() OVERRIDE {
-    return nfc_device_client_.get();
+    return client_bundle_->nfc_device_client();
   }
 
   virtual NfcManagerClient* GetNfcManagerClient() OVERRIDE {
-    return nfc_manager_client_.get();
+    return client_bundle_->nfc_manager_client();
   }
 
   virtual NfcRecordClient* GetNfcRecordClient() OVERRIDE {
-    return nfc_record_client_.get();
+    return client_bundle_->nfc_record_client();
   }
 
   virtual NfcTagClient* GetNfcTagClient() OVERRIDE {
-    return nfc_tag_client_.get();
+    return client_bundle_->nfc_tag_client();
   }
 
   virtual PermissionBrokerClient* GetPermissionBrokerClient() OVERRIDE {
-    return permission_broker_client_.get();
+    return client_bundle_->permission_broker_client();
   }
 
   virtual PowerManagerClient* GetPowerManagerClient() OVERRIDE {
-    return power_manager_client_.get();
-  }
-
-  virtual PowerPolicyController* GetPowerPolicyController() OVERRIDE {
-    return power_policy_controller_.get();
+    return client_bundle_->power_manager_client();
   }
 
   virtual SessionManagerClient* GetSessionManagerClient() OVERRIDE {
-    return session_manager_client_.get();
+    return client_bundle_->session_manager_client();
   }
 
   virtual SMSClient* GetSMSClient() OVERRIDE {
-    return sms_client_.get();
+    return client_bundle_->sms_client();
   }
 
   virtual SystemClockClient* GetSystemClockClient() OVERRIDE {
-    return system_clock_client_.get();
+    return client_bundle_->system_clock_client();
   }
 
   virtual UpdateEngineClient* GetUpdateEngineClient() OVERRIDE {
-    return update_engine_client_.get();
+    return client_bundle_->update_engine_client();
+  }
+
+  virtual PowerPolicyController* GetPowerPolicyController() OVERRIDE {
+    return power_policy_controller_.get();
   }
 
  private:
   // Constructs all clients and stores them in the respective *_client_ member
   // variable.
   void CreateDefaultClients() {
-    DBusClientImplementationType client_type = REAL_DBUS_CLIENT_IMPLEMENTATION;
-    DBusClientImplementationType client_type_override =
-        REAL_DBUS_CLIENT_IMPLEMENTATION;
-    // If --dbus-stub was requested, pass STUB to specific components;
-    // Many components like login are not useful with a stub implementation.
-    if (CommandLine::ForCurrentProcess()->HasSwitch(
-            chromeos::switches::kDbusStub)) {
-      client_type_override = STUB_DBUS_CLIENT_IMPLEMENTATION;
-    }
-
-    bluetooth_adapter_client_.reset(BluetoothAdapterClient::Create());
-    bluetooth_agent_manager_client_.reset(
-        BluetoothAgentManagerClient::Create());
-    bluetooth_device_client_.reset(BluetoothDeviceClient::Create());
-    bluetooth_input_client_.reset(BluetoothInputClient::Create());
-    bluetooth_profile_manager_client_.reset(
-        BluetoothProfileManagerClient::Create());
-    cras_audio_client_.reset(CrasAudioClient::Create());
-    cros_disks_client_.reset(CrosDisksClient::Create(client_type));
-    cryptohome_client_.reset(CryptohomeClient::Create());
-    debug_daemon_client_.reset(DebugDaemonClient::Create());
-    shill_manager_client_.reset(ShillManagerClient::Create());
-    shill_device_client_.reset(ShillDeviceClient::Create());
-    shill_ipconfig_client_.reset(ShillIPConfigClient::Create());
-    shill_service_client_.reset(ShillServiceClient::Create());
-    shill_profile_client_.reset(ShillProfileClient::Create());
-    gsm_sms_client_.reset(GsmSMSClient::Create());
-    image_burner_client_.reset(ImageBurnerClient::Create());
-    introspectable_client_.reset(IntrospectableClient::Create());
-    modem_messaging_client_.reset(ModemMessagingClient::Create());
-    // Create the NFC clients in the correct order based on their dependencies.
-    nfc_manager_client_.reset(NfcManagerClient::Create());
-    nfc_adapter_client_.reset(
-        NfcAdapterClient::Create(nfc_manager_client_.get()));
-    nfc_device_client_.reset(
-        NfcDeviceClient::Create(nfc_adapter_client_.get()));
-    nfc_tag_client_.reset(NfcTagClient::Create(nfc_adapter_client_.get()));
-    nfc_record_client_.reset(NfcRecordClient::Create(nfc_device_client_.get(),
-                                                     nfc_tag_client_.get()));
-    permission_broker_client_.reset(PermissionBrokerClient::Create());
-    power_manager_client_.reset(
-        PowerManagerClient::Create(client_type_override));
-    session_manager_client_.reset(SessionManagerClient::Create(client_type));
-    sms_client_.reset(SMSClient::Create());
-    system_clock_client_.reset(SystemClockClient::Create());
-    update_engine_client_.reset(UpdateEngineClient::Create(client_type));
-
+    client_bundle_.reset(new DBusClientBundle);
     power_policy_controller_.reset(new PowerPolicyController);
   }
 
@@ -282,38 +416,7 @@ class DBusThreadManagerImpl : public DBusThreadManager {
 
   scoped_ptr<base::Thread> dbus_thread_;
   scoped_refptr<dbus::Bus> system_bus_;

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

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:35]

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]

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.

-  scoped_ptr<BluetoothAdapterClient> bluetooth_adapter_client_;
-  scoped_ptr<BluetoothAgentManagerClient> bluetooth_agent_manager_client_;
-  scoped_ptr<BluetoothDeviceClient> bluetooth_device_client_;
-  scoped_ptr<BluetoothInputClient> bluetooth_input_client_;
-  scoped_ptr<BluetoothProfileManagerClient> bluetooth_profile_manager_client_;
-  scoped_ptr<CrasAudioClient> cras_audio_client_;
-  scoped_ptr<CrosDisksClient> cros_disks_client_;
-  scoped_ptr<CryptohomeClient> cryptohome_client_;
-  scoped_ptr<DebugDaemonClient> debug_daemon_client_;
-  scoped_ptr<ShillDeviceClient> shill_device_client_;
-  scoped_ptr<ShillIPConfigClient> shill_ipconfig_client_;
-  scoped_ptr<ShillManagerClient> shill_manager_client_;
-  scoped_ptr<ShillServiceClient> shill_service_client_;
-  scoped_ptr<ShillProfileClient> shill_profile_client_;
-  scoped_ptr<GsmSMSClient> gsm_sms_client_;
-  scoped_ptr<ImageBurnerClient> image_burner_client_;
-  scoped_ptr<IntrospectableClient> introspectable_client_;
-  scoped_ptr<ModemMessagingClient> modem_messaging_client_;
-  // The declaration order for NFC client objects is important. See
-  // DBusThreadManager::CreateDefaultClients for the dependencies.
-  scoped_ptr<NfcManagerClient> nfc_manager_client_;
-  scoped_ptr<NfcAdapterClient> nfc_adapter_client_;
-  scoped_ptr<NfcDeviceClient> nfc_device_client_;
-  scoped_ptr<NfcTagClient> nfc_tag_client_;
-  scoped_ptr<NfcRecordClient> nfc_record_client_;
-  scoped_ptr<PermissionBrokerClient> permission_broker_client_;
-  scoped_ptr<SystemClockClient> system_clock_client_;
-  scoped_ptr<PowerManagerClient> power_manager_client_;
-  scoped_ptr<SessionManagerClient> session_manager_client_;
-  scoped_ptr<SMSClient> sms_client_;
-  scoped_ptr<UpdateEngineClient> update_engine_client_;
-
+  scoped_ptr<DBusClientBundle> client_bundle_;
   scoped_ptr<PowerPolicyController> power_policy_controller_;
 };