Fix dangling pointers in DBus during Shutdown.
@alimariam reported the dangling pointer detector found a new dangling
pointer when running tests on linux Workstation.
The error is:
```
The memory was freed at:
#3 allocator_shim::internal::PartitionFree()
#4 bluez::BluezDBusThreadManager::~BluezDBusThreadManager()
#5 bluez::BluezDBusThreadManager::Shutdown()
#6 ChromeBrowserMainPartsLinux::PostDestroyThreads()
#7 content::BrowserMainLoop::ShutdownThreadsAndCleanUp()
#8 content::BrowserMainRunnerImpl::Shutdown()
#9 content::BrowserMain()
#10 content::RunBrowserProcessMain()
#11 content::ContentMainRunnerImpl::RunBrowser()
#12 content::ContentMainRunnerImpl::Run()
#13 content::RunContentProcess()
#14 content::ContentMain()
#15 ChromeMain
The dangling raw_ptr was released at:
#3 base::internal::RawPtrBackupRefImpl<>::ReleaseInternal()
#4 dbus::ObjectManager::~ObjectManager()
#5 std::__Cr::__tuple_impl<>::~__tuple_impl()
#6 base::internal::BindState<>::Destroy()
#7 base::[...]::LazilyDeallocatedDeque<>::Ring::~Ring()
#8 base::[...]::TaskQueueImpl::UnregisterTaskQueue()
#9 base::[...]::SequenceManagerImpl::UnregisterTaskQueueImpl()
#10 base::sequence_manager::TaskQueue::ShutdownTaskQueue()
#11 content::BrowserTaskQueues::~BrowserTaskQueues()
#12 content::BrowserUIThreadScheduler::~BrowserUIThreadScheduler()
#13 content::BrowserTaskExecutor::[...]::~UIThreadExecutor()
#14 content::BrowserTaskExecutor::[...]::~UIThreadExecutor()
#15 content::BrowserTaskExecutor::Shutdown()
#16 content::ContentMainRunnerImpl::Shutdown()
#17 content::RunContentProcess()
#18 content::ContentMain()
#19 ChromeMain
```
Diagnostic:
- `bluez::BluezDBusThreadManager` owns a `dbus::Bus` as `system_bus`.
- `dbus::Bus` owns:
- The set of `dbus::ObjectManager` as `object_manager_table_`.
- The DBus task runner as `dbus_task_runner_`.
- The `dbus::ObjectManager` references `dbus::Bus` via `bus_`.
So far so good, the ownership is clear. The problem happens when calling
`dbus::Bus::RemoveObjectManager`. Indeed this moves the ObjectManager
out of `dbus::Bus` toward a callback to a new thread. This still works
transitively, because the dbus::Bus owns the thread. The problem happens
after a second transfer back to the original thread.
Indeed, there is a race condition possible:
Behavior without problems: -----------------------------------
┌─────────────┐ ┌───────────┐
│Origin thread│ │DBus thread│
└──────┬──────┘ └─────┬─────┘
RemoveObjectManager() │
│────────────────────────────────>│
│ RemoveObjectManagerInternal()
│<────────────────────────────────│
RemoveObjectManagerInternalHelper() │
~ObjectManager() │
│ ┌─────┴─────┐
Shutdown DBus Thread ─────────────>│DBus thread│
Shutdown DBus Thread <─────────────│DBus thread│
│ └───────────┘
~Bus
┌──────┴──────┐
│Origin thread│
└─────────────┘
Behavior with problems: ----------------------------------------
┌─────────────┐ ┌───────────┐
│Origin thread│ │DBus thread│
└──────┬──────┘ └─────┬─────┘
RemoveObjectManager() │
│────────────────────────────────>│
│ RemoveObjectManagerInternal()
│ ┌────────────│
│ │ ┌─────┴─────┐
Shutdown DBus Thread ─────────────>│DBus thread│
Shutdown DBus Thread <─────────────│DBus thread│
│ │ └───────────┘
~Bus() │
│ │
│<───────────────────┘
RemoveObjectManagerInternalHelper()
~ObjectManager()
┌──────┴──────┐
│Origin thread│
└─────────────┘
-----------------------------------------------------------------
In the second case: ~Bus() is called before ~ObjectManager().
The fix is a use `ObjectManager::Cleanup()` to cleanup the raw_ptr while
the object is still transitively owned by the object it referenced.
Bug: chromium:1478759
Fixed: chromium:1478759
Change-Id: I4ac04d449ab8a7b860256c490f8ac878c1c5c7c5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4839496
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1192343}
NOKEYCHECK=True
GitOrigin-RevId: bcb156213bee63d24813d2203ef5c36f81785687
diff --git a/object_manager.cc b/object_manager.cc
index 9feaaf9..93e6c87 100644
--- a/object_manager.cc
+++ b/object_manager.cc
@@ -50,11 +50,7 @@
ObjectManager::ObjectManager(Bus* bus,
const std::string& service_name,
const ObjectPath& object_path)
- : bus_(bus),
- service_name_(service_name),
- object_path_(object_path),
- setup_success_(false),
- cleanup_called_(false) {
+ : bus_(bus), service_name_(service_name), object_path_(object_path) {
LOG_IF(FATAL, !object_path_.IsValid()) << object_path_.value();
DVLOG(1) << "Creating ObjectManager for " << service_name_
<< " " << object_path_.value();
@@ -175,15 +171,20 @@
}
match_rule_.clear();
+ // After Cleanup(), the Bus doesn't own `this` anymore and might be deleted
+ // before `this`.
+ bus_ = nullptr;
}
bool ObjectManager::SetupMatchRuleAndFilter() {
- DCHECK(bus_);
DCHECK(!setup_success_);
- bus_->AssertOnDBusThread();
- if (cleanup_called_)
+ if (cleanup_called_) {
return false;
+ }
+
+ DCHECK(bus_);
+ bus_->AssertOnDBusThread();
if (!bus_->Connect() || !bus_->SetUpAsyncOperations())
return false;
@@ -225,16 +226,17 @@
<< ": Failed to set up match rule.";
return;
}
-
- DCHECK(bus_);
DCHECK(object_proxy_);
DCHECK(setup_success_);
- bus_->AssertOnOriginThread();
// |object_proxy_| is no longer valid if the Bus was shut down before this
// call. Don't initiate any other action from the origin thread.
- if (cleanup_called_)
+ if (cleanup_called_) {
return;
+ }
+
+ DCHECK(bus_);
+ bus_->AssertOnOriginThread();
object_proxy_->ConnectToSignal(
kObjectManagerInterface, kObjectManagerInterfacesAdded,
diff --git a/object_manager.h b/object_manager.h
index 824469a..9840aa5 100644
--- a/object_manager.h
+++ b/object_manager.h
@@ -328,14 +328,16 @@
// |service_name_owner_|.
void UpdateServiceNameOwner(const std::string& new_owner);
+ // Valid in between the constructor and `CleanUp()`.
+ // After Cleanup(), `this` lifetime might exceed Bus's one.
raw_ptr<Bus> bus_;
std::string service_name_;
std::string service_name_owner_;
std::string match_rule_;
ObjectPath object_path_;
raw_ptr<ObjectProxy, AcrossTasksDanglingUntriaged> object_proxy_;
- bool setup_success_;
- bool cleanup_called_;
+ bool setup_success_ = false;
+ bool cleanup_called_ = false;
// Maps the name of an interface to the implementation class used for
// instantiating PropertySet structures for that interface's properties.
