commit e11e81c98987c5bd4b3d50f5cd0188c2b25df864
author Christopher Wiley <wiley@chromium.org> Fri Dec 19 13:10:52 2014 -0800
committer chrome-internal-fetch <chrome-internal-fetch@google.com> Sat Dec 20 04:46:24 2014 +0000
tree d29ae02cb1a809a4f68a113d6fb2b3d128156462
parent f3c689780c9c0cc5fd1630f01ee05060bb696403

chromeos-dbus-bindings: Add destructor for ObjectManagerProxy

Previously, the generated ObjectManagerProxy was registering interfaces
with its wrapped ObjectManager instance but not unregistering them. The
registered interfaces were using the ObjectManagerProxy pointer to
create new property sets for the given interfaces. Unfortunately, this
led to a race where:

1) ObjectManagerProxy is destroyed
2) dbus::Bus object receives a signal that new interface has been added
3) ObjectManager instance previously wrapped by ObjectManagerProxy deferences
   the pointer to the ObjectManagerProxy (now invalid)
4) Terrible things happen here.

BUG=None
TEST=generated proxies still compile.

Change-Id: Ie1da0955088606d5cd41a1bffc131b114dc1f173
Reviewed-on: https://chromium-review.googlesource.com/236885
Commit-Queue: Christopher Wiley <wiley@chromium.org>
Tested-by: Christopher Wiley <wiley@chromium.org>
Reviewed-by: Vitaly Buka <vitalybuka@chromium.org>

chromeos-dbus-bindings/proxy_generator.cc

diff --git a/chromeos-dbus-bindings/proxy_generator.cc b/chromeos-dbus-bindings/proxy_generator.cc
index 66f39ea..797a7d5 100644
--- a/chromeos-dbus-bindings/proxy_generator.cc
+++ b/chromeos-dbus-bindings/proxy_generator.cc
@@ -553,6 +553,7 @@
   text->PushOffset(kBlockOffset);
 
   AddConstructor(config, class_name, interfaces, text);
+  AddDestructor(class_name, interfaces, text);
   AddGetObjectManagerProxy(text);
   for (const auto& itf : interfaces) {
     AddInterfaceAccessors(itf, text);
@@ -618,6 +619,22 @@
   text->AddBlankLine();
 }
 
+void ProxyGenerator::ObjectManager::AddDestructor(
+    const std::string& class_name,
+    const std::vector<Interface>& interfaces,
+    IndentedText* text) {
+  text->AddLine(StringPrintf("~%s() override {", class_name.c_str()));
+  text->PushOffset(kBlockOffset);
+  for (const auto& itf : interfaces) {
+    text->AddLine(
+        StringPrintf("dbus_object_manager_->UnregisterInterface(\"%s\");",
+                     itf.name.c_str()));
+  }
+  text->PopOffset();
+  text->AddLine("}");
+  text->AddBlankLine();
+}
+
 void ProxyGenerator::ObjectManager::AddGetObjectManagerProxy(
     IndentedText* text) {
   text->AddLine("dbus::ObjectManager* GetObjectManagerProxy() const {");

chromeos-dbus-bindings/proxy_generator.h

diff --git a/chromeos-dbus-bindings/proxy_generator.h b/chromeos-dbus-bindings/proxy_generator.h
index 8fd46a4..682e6dc 100644
--- a/chromeos-dbus-bindings/proxy_generator.h
+++ b/chromeos-dbus-bindings/proxy_generator.h
@@ -101,6 +101,11 @@
                                const std::vector<Interface>& interfaces,
                                IndentedText* text);
 
+    // Generates Object Manager destructor.
+    static void AddDestructor(const std::string& class_name,
+                              const std::vector<Interface>& interfaces,
+                              IndentedText* text);
+
     // Generates GetObjectManagerProxy() method.
     static void AddGetObjectManagerProxy(IndentedText* text);
 

chromeos-dbus-bindings/proxy_generator_unittest.cc

diff --git a/chromeos-dbus-bindings/proxy_generator_unittest.cc b/chromeos-dbus-bindings/proxy_generator_unittest.cc
index 7251293..d374ee4 100644
--- a/chromeos-dbus-bindings/proxy_generator_unittest.cc
+++ b/chromeos-dbus-bindings/proxy_generator_unittest.cc
@@ -621,6 +621,11 @@
     dbus_object_manager_->RegisterInterface("org.chromium.Itf2", this);
   }
 
+  ~ObjectManagerProxy() override {
+    dbus_object_manager_->UnregisterInterface("org.chromium.Itf1");
+    dbus_object_manager_->UnregisterInterface("org.chromium.Itf2");
+  }
+
   dbus::ObjectManager* GetObjectManagerProxy() const {
     return dbus_object_manager_;
   }
@@ -937,6 +942,11 @@
     dbus_object_manager_->RegisterInterface("org.chromium.Itf2", this);
   }
 
+  ~ObjectManagerProxy() override {
+    dbus_object_manager_->UnregisterInterface("org.chromium.Itf1");
+    dbus_object_manager_->UnregisterInterface("org.chromium.Itf2");
+  }
+
   dbus::ObjectManager* GetObjectManagerProxy() const {
     return dbus_object_manager_;
   }