libchromeos: Fix race in DBusObject lifetime.
Fix a race where destroying a DBusObject with an ExportedObjectManager
caused a CHECK to fail if DBusInterface objects had not finished
claiming the corresponding DBus interfaces. This happened because
an interface would not claim the interface until all handlers had
exported, while the parent DBusObject would unilaterally release
interfaces. Now the DBusInterface owns the lifetime of its claimed
interface in the ExportedObjectManager.
BUG=brillo:110
TEST=Unittests. This new unittest crashes before this change.
Change-Id: I54dcafc0c2de50983439ff6e4759280f25f05b75
Reviewed-on: https://chromium-review.googlesource.com/252092
Tested-by: Christopher Wiley <wiley@chromium.org>
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Commit-Queue: Christopher Wiley <wiley@chromium.org>
diff --git a/chromeos/dbus/dbus_object.cc b/chromeos/dbus/dbus_object.cc
index 4e8cdc2..8f2adde 100644
--- a/chromeos/dbus/dbus_object.cc
+++ b/chromeos/dbus/dbus_object.cc
@@ -54,17 +54,32 @@
if (object_manager) {
auto property_writer_callback =
dbus_object_->property_set_.GetPropertyWriter(interface_name_);
- actions.push_back(sequencer->WrapCompletionTask(
- base::Bind(&ExportedObjectManager::ClaimInterface,
+ actions.push_back(
+ base::Bind(&DBusInterface::ClaimInterface,
+ weak_factory_.GetWeakPtr(),
object_manager->AsWeakPtr(),
object_path,
- interface_name_,
- property_writer_callback)));
+ property_writer_callback));
}
actions.push_back(completion_callback);
sequencer->OnAllTasksCompletedCall(actions);
}
+void DBusInterface::ClaimInterface(
+ base::WeakPtr<ExportedObjectManager> object_manager,
+ const dbus::ObjectPath& object_path,
+ const ExportedPropertySet::PropertyWriter& writer,
+ bool all_succeeded) {
+ if (!all_succeeded || !object_manager) {
+ LOG(ERROR) << "Skipping claiming interface: " << interface_name_;
+ return;
+ }
+ object_manager->ClaimInterface(object_path, interface_name_, writer);
+ release_interface_cb_.Reset(
+ base::Bind(&ExportedObjectManager::ReleaseInterface,
+ object_manager, object_path, interface_name_));
+}
+
void DBusInterface::HandleMethodCall(dbus::MethodCall* method_call,
ResponseSender sender) {
std::string method_name = method_call->GetMember();
@@ -115,11 +130,6 @@
}
DBusObject::~DBusObject() {
- if (object_manager_) {
- for (const auto& pair : interfaces_)
- object_manager_->ReleaseInterface(object_path_, pair.first);
- }
-
if (exported_object_)
exported_object_->Unregister();
}
diff --git a/chromeos/dbus/dbus_object.h b/chromeos/dbus/dbus_object.h
index f08027f..8dc9c88 100644
--- a/chromeos/dbus/dbus_object.h
+++ b/chromeos/dbus/dbus_object.h
@@ -65,6 +65,7 @@
#include <string>
#include <base/bind.h>
+#include <base/callback_helpers.h>
#include <base/macros.h>
#include <base/memory/weak_ptr.h>
#include <chromeos/chromeos_export.h>
@@ -360,6 +361,12 @@
const dbus::ObjectPath& object_path,
const AsyncEventSequencer::CompletionAction& completion_callback);
+ CHROMEOS_PRIVATE void ClaimInterface(
+ base::WeakPtr<ExportedObjectManager> object_manager,
+ const dbus::ObjectPath& object_path,
+ const ExportedPropertySet::PropertyWriter& writer,
+ bool all_succeeded);
+
// Method registration map.
std::map<std::string, std::unique_ptr<DBusInterfaceMethodHandlerInterface>>
handlers_;
@@ -370,7 +377,9 @@
friend class DBusInterfaceTestHelper;
DBusObject* dbus_object_;
std::string interface_name_;
+ base::ScopedClosureRunner release_interface_cb_;
+ base::WeakPtrFactory<DBusInterface> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(DBusInterface);
};
diff --git a/chromeos/dbus/dbus_object_unittest.cc b/chromeos/dbus/dbus_object_unittest.cc
index f0b7d55..251e63c 100644
--- a/chromeos/dbus/dbus_object_unittest.cc
+++ b/chromeos/dbus/dbus_object_unittest.cc
@@ -8,6 +8,7 @@
#include <base/bind.h>
#include <chromeos/dbus/dbus_object_test_helpers.h>
+#include <chromeos/dbus/mock_exported_object_manager.h>
#include <dbus/message.h>
#include <dbus/property.h>
#include <dbus/object_path.h>
@@ -17,6 +18,7 @@
using ::testing::AnyNumber;
using ::testing::Return;
using ::testing::Invoke;
+using ::testing::Mock;
using ::testing::_;
namespace chromeos {
@@ -24,6 +26,8 @@
namespace {
+const char kMethodsExportedOn[] = "/export";
+
const char kTestInterface1[] = "org.chromium.Test.MathInterface";
const char kTestMethod_Add[] = "Add";
const char kTestMethod_Negate[] = "Negate";
@@ -81,7 +85,8 @@
EXPECT_CALL(*bus_, AssertOnOriginThread()).Times(AnyNumber());
EXPECT_CALL(*bus_, AssertOnDBusThread()).Times(AnyNumber());
// Use a mock exported object.
- const dbus::ObjectPath kMethodsExportedOnPath(std::string("/export"));
+ const dbus::ObjectPath kMethodsExportedOnPath{
+ std::string{kMethodsExportedOn}};
mock_exported_object_ =
new dbus::MockExportedObject(bus_.get(), kMethodsExportedOnPath);
EXPECT_CALL(*bus_, GetExportedObject(kMethodsExportedOnPath))
@@ -318,5 +323,24 @@
ExpectError(response.get(), DBUS_ERROR_UNKNOWN_METHOD);
}
+TEST_F(DBusObjectTest, ShouldReleaseOnlyClaimedInterfaces) {
+ const dbus::ObjectPath kObjectManagerPath{std::string{"/"}};
+ const dbus::ObjectPath kMethodsExportedOnPath{
+ std::string{kMethodsExportedOn}};
+ MockExportedObjectManager mock_object_manager{bus_, kObjectManagerPath};
+ dbus_object_ = std::unique_ptr<DBusObject>(
+ new DBusObject(&mock_object_manager, bus_, kMethodsExportedOnPath));
+ EXPECT_CALL(mock_object_manager, ClaimInterface(_, _, _)).Times(0);
+ EXPECT_CALL(mock_object_manager, ReleaseInterface(_, _)).Times(0);
+ DBusInterface* itf1 = dbus_object_->AddOrGetInterface(kTestInterface1);
+ itf1->AddSimpleMethodHandler(
+ kTestMethod_Add, base::Unretained(&calc_), &Calc::Add);
+ // When we tear down our DBusObject, it should release only interfaces it has
+ // previously claimed. This prevents a check failing inside the
+ // ExportedObjectManager. Since no interfaces have finished exporting
+ // handlers, nothing should be released.
+ dbus_object_.reset();
+}
+
} // namespace dbus_utils
} // namespace chromeos
diff --git a/chromeos/dbus/exported_object_manager.h b/chromeos/dbus/exported_object_manager.h
index a346078..183e676 100644
--- a/chromeos/dbus/exported_object_manager.h
+++ b/chromeos/dbus/exported_object_manager.h
@@ -86,23 +86,25 @@
ExportedObjectManager(scoped_refptr<dbus::Bus> bus,
const dbus::ObjectPath& path);
+ virtual ~ExportedObjectManager() = default;
// Registers methods implementing the ObjectManager interface on the object
// exported on the path given in the constructor. Must be called on the
// origin thread.
- void RegisterAsync(
+ virtual void RegisterAsync(
const chromeos::dbus_utils::AsyncEventSequencer::CompletionAction&
completion_callback);
// Trigger a signal that |path| has added an interface |interface_name|
// with properties as given by |writer|.
- void ClaimInterface(const dbus::ObjectPath& path,
- const std::string& interface_name,
- const ExportedPropertySet::PropertyWriter& writer);
+ virtual void ClaimInterface(
+ const dbus::ObjectPath& path,
+ const std::string& interface_name,
+ const ExportedPropertySet::PropertyWriter& writer);
// Trigger a signal that |path| has removed an interface |interface_name|.
- void ReleaseInterface(const dbus::ObjectPath& path,
- const std::string& interface_name);
+ virtual void ReleaseInterface(const dbus::ObjectPath& path,
+ const std::string& interface_name);
const scoped_refptr<dbus::Bus>& GetBus() const { return bus_; }
diff --git a/chromeos/dbus/mock_exported_object_manager.h b/chromeos/dbus/mock_exported_object_manager.h
new file mode 100644
index 0000000..2b49dbd
--- /dev/null
+++ b/chromeos/dbus/mock_exported_object_manager.h
@@ -0,0 +1,42 @@
+// Copyright 2015 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef LIBCHROMEOS_CHROMEOS_DBUS_MOCK_EXPORTED_OBJECT_MANAGER_H_
+#define LIBCHROMEOS_CHROMEOS_DBUS_MOCK_EXPORTED_OBJECT_MANAGER_H_
+
+#include <string>
+
+#include <chromeos/dbus/async_event_sequencer.h>
+#include <chromeos/dbus/exported_object_manager.h>
+#include <dbus/object_path.h>
+#include <gmock/gmock.h>
+
+namespace chromeos {
+
+namespace dbus_utils {
+
+class MockExportedObjectManager : public ExportedObjectManager {
+ public:
+ using CompletionAction =
+ chromeos::dbus_utils::AsyncEventSequencer::CompletionAction;
+
+ using ExportedObjectManager::ExportedObjectManager;
+ ~MockExportedObjectManager() override = default;
+
+ MOCK_METHOD1(RegisterAsync,
+ void(const CompletionAction& completion_callback));
+ MOCK_METHOD3(ClaimInterface,
+ void(const dbus::ObjectPath& path,
+ const std::string& interface_name,
+ const ExportedPropertySet::PropertyWriter& writer));
+ MOCK_METHOD2(ReleaseInterface,
+ void(const dbus::ObjectPath& path,
+ const std::string& interface_name));
+};
+
+} // namespace dbus_utils
+
+} // namespace chromeos
+
+#endif // LIBCHROMEOS_CHROMEOS_DBUS_MOCK_EXPORTED_OBJECT_MANAGER_H_