[shill] Un-do a few usages of ref-counted pointers where they cause cycles
Giving an Adaptor a ref-counted pointer to the object for which it is an adaptor
causes an ownership cycle that leads to memory leaks.
In lieu of trying to create some more formal structure, just rely on the fact
that a Device/Service/Manager creates and owns its Adaptor and give a bare
pointer to the Adaptor when it is created.
BUG=chromium-os:16259
TEST=unit tests
Change-Id: Idf909270a156bc97c4f8d4f0a493284d7197a1c8
Reviewed-on: http://gerrit.chromium.org/gerrit/2306
Reviewed-by: Chris Masone <cmasone@chromium.org>
Tested-by: Chris Masone <cmasone@chromium.org>
diff --git a/device_dbus_adaptor.cc b/device_dbus_adaptor.cc
index adade7d..9a3f05e 100644
--- a/device_dbus_adaptor.cc
+++ b/device_dbus_adaptor.cc
@@ -20,13 +20,14 @@
// static
const char DeviceDBusAdaptor::kPath[] = "/device/";
-DeviceDBusAdaptor::DeviceDBusAdaptor(DBus::Connection* conn,
- DeviceRefPtr device)
+DeviceDBusAdaptor::DeviceDBusAdaptor(DBus::Connection* conn, Device *device)
: DBusAdaptor(conn, kPath + device->UniqueName()),
device_(device) {
}
-DeviceDBusAdaptor::~DeviceDBusAdaptor() {}
+DeviceDBusAdaptor::~DeviceDBusAdaptor() {
+ device_ = NULL;
+}
void DeviceDBusAdaptor::UpdateEnabled() {}
diff --git a/device_dbus_adaptor.h b/device_dbus_adaptor.h
index 1e1b522..96bb549 100644
--- a/device_dbus_adaptor.h
+++ b/device_dbus_adaptor.h
@@ -12,12 +12,16 @@
#include "shill/adaptor_interfaces.h"
#include "shill/dbus_adaptor.h"
-#include "shill/device.h"
#include "shill/flimflam-device.h"
namespace shill {
+class Device;
+
// Subclass of DBusAdaptor for Device objects
+// There is a 1:1 mapping between Device and DeviceDBusAdaptor instances.
+// Furthermore, the Device owns the DeviceDBusAdaptor and manages its lifetime,
+// so we're OK with DeviceDBusAdaptor having a bare pointer to its owner device.
class DeviceDBusAdaptor : public org::chromium::flimflam::Device_adaptor,
public DBusAdaptor,
public DeviceAdaptorInterface {
@@ -25,7 +29,7 @@
static const char kInterfaceName[];
static const char kPath[];
- DeviceDBusAdaptor(DBus::Connection* conn, DeviceRefPtr device);
+ DeviceDBusAdaptor(DBus::Connection* conn, Device *device);
virtual ~DeviceDBusAdaptor();
// Implementation of DeviceAdaptorInterface.
@@ -44,7 +48,7 @@
::DBus::Path AddIPConfig(const std::string& , ::DBus::Error &error);
private:
- DeviceRefPtr device_;
+ Device *device_;
DISALLOW_COPY_AND_ASSIGN(DeviceDBusAdaptor);
};
diff --git a/manager_dbus_adaptor.cc b/manager_dbus_adaptor.cc
index 381256d..e87bc45 100644
--- a/manager_dbus_adaptor.cc
+++ b/manager_dbus_adaptor.cc
@@ -23,7 +23,10 @@
: DBusAdaptor(conn, kPath),
manager_(manager) {
}
-ManagerDBusAdaptor::~ManagerDBusAdaptor() {}
+
+ManagerDBusAdaptor::~ManagerDBusAdaptor() {
+ manager_ = NULL;
+}
void ManagerDBusAdaptor::UpdateRunning() {}
diff --git a/manager_dbus_adaptor.h b/manager_dbus_adaptor.h
index 1827e70..f65e595 100644
--- a/manager_dbus_adaptor.h
+++ b/manager_dbus_adaptor.h
@@ -15,9 +15,14 @@
#include "shill/flimflam-manager.h"
namespace shill {
+
class Manager;
// Subclass of DBusAdaptor for Manager objects
+// There is a 1:1 mapping between Manager and ManagerDBusAdaptor
+// instances. Furthermore, the Manager owns the ManagerDBusAdaptor
+// and manages its lifetime, so we're OK with ManagerDBusAdaptor
+// having a bare pointer to its owner manager.
class ManagerDBusAdaptor : public org::chromium::flimflam::Manager_adaptor,
public DBusAdaptor,
public ManagerAdaptorInterface {
diff --git a/service_dbus_adaptor.cc b/service_dbus_adaptor.cc
index ab50e02..8241ac7 100644
--- a/service_dbus_adaptor.cc
+++ b/service_dbus_adaptor.cc
@@ -19,11 +19,13 @@
// static
const char ServiceDBusAdaptor::kPath[] = "/service/";
-ServiceDBusAdaptor::ServiceDBusAdaptor(DBus::Connection* conn,
- ServiceRefPtr service)
+ServiceDBusAdaptor::ServiceDBusAdaptor(DBus::Connection* conn, Service *service)
: DBusAdaptor(conn, kPath + service->UniqueName()),
service_(service) {}
-ServiceDBusAdaptor::~ServiceDBusAdaptor() {}
+
+ServiceDBusAdaptor::~ServiceDBusAdaptor() {
+ service_ = NULL;
+}
void ServiceDBusAdaptor::UpdateConnected() {}
diff --git a/service_dbus_adaptor.h b/service_dbus_adaptor.h
index fe45f87..e4990d4 100644
--- a/service_dbus_adaptor.h
+++ b/service_dbus_adaptor.h
@@ -13,11 +13,16 @@
#include "shill/adaptor_interfaces.h"
#include "shill/dbus_adaptor.h"
#include "shill/flimflam-service.h"
-#include "shill/service.h"
namespace shill {
+class Service;
+
// Subclass of DBusAdaptor for Service objects
+// There is a 1:1 mapping between Service and ServiceDBusAdaptor
+// instances. Furthermore, the Service owns the ServiceDBusAdaptor
+// and manages its lifetime, so we're OK with ServiceDBusAdaptor
+// having a bare pointer to its owner service.
class ServiceDBusAdaptor : public org::chromium::flimflam::Service_adaptor,
public DBusAdaptor,
public ServiceAdaptorInterface {
@@ -25,7 +30,7 @@
static const char kInterfaceName[];
static const char kPath[];
- ServiceDBusAdaptor(DBus::Connection* conn, ServiceRefPtr service);
+ ServiceDBusAdaptor(DBus::Connection* conn, Service *service);
virtual ~ServiceDBusAdaptor();
// Implementation of ServiceAdaptorInterface.
@@ -49,7 +54,7 @@
void ActivateCellularModem(const std::string& , ::DBus::Error &error);
private:
- ServiceRefPtr service_;
+ Service *service_;
DISALLOW_COPY_AND_ASSIGN(ServiceDBusAdaptor);
};