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:
  chromium#3  allocator_shim::internal::PartitionFree()
  chromium#4  bluez::BluezDBusThreadManager::~BluezDBusThreadManager()
  chromium#5  bluez::BluezDBusThreadManager::Shutdown()
  chromium#6  ChromeBrowserMainPartsLinux::PostDestroyThreads()
  chromium#7  content::BrowserMainLoop::ShutdownThreadsAndCleanUp()
  chromium#8  content::BrowserMainRunnerImpl::Shutdown()
  chromium#9  content::BrowserMain()
  chromium#10 content::RunBrowserProcessMain()
  chromium#11 content::ContentMainRunnerImpl::RunBrowser()
  chromium#12 content::ContentMainRunnerImpl::Run()
  chromium#13 content::RunContentProcess()
  chromium#14 content::ContentMain()
  chromium#15 ChromeMain

  The dangling raw_ptr was released at:
  chromium#3  base::internal::RawPtrBackupRefImpl<>::ReleaseInternal()
  chromium#4  dbus::ObjectManager::~ObjectManager()
  chromium#5  std::__Cr::__tuple_impl<>::~__tuple_impl()
  chromium#6  base::internal::BindState<>::Destroy()
  chromium#7  base::[...]::LazilyDeallocatedDeque<>::Ring::~Ring()
  chromium#8  base::[...]::TaskQueueImpl::UnregisterTaskQueue()
  chromium#9  base::[...]::SequenceManagerImpl::UnregisterTaskQueueImpl()
  chromium#10 base::sequence_manager::TaskQueue::ShutdownTaskQueue()
  chromium#11 content::BrowserTaskQueues::~BrowserTaskQueues()
  chromium#12 content::BrowserUIThreadScheduler::~BrowserUIThreadScheduler()
  chromium#13 content::BrowserTaskExecutor::[...]::~UIThreadExecutor()
  chromium#14 content::BrowserTaskExecutor::[...]::~UIThreadExecutor()
  chromium#15 content::BrowserTaskExecutor::Shutdown()
  chromium#16 content::ContentMainRunnerImpl::Shutdown()
  chromium#17 content::RunContentProcess()
  chromium#18 content::ContentMain()
  chromium#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 <[email protected]>
Commit-Queue: Arthur Sonzogni <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1192343}

Author: ArthurSonzogni

dbus/object_manager.cc

@@ -50,11 +50,7 @@ scoped_refptr<ObjectManager> ObjectManager::Create(
 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 @@ void ObjectManager::CleanUp() {
   }
 
   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 @@ void ObjectManager::OnSetupMatchRuleAndFilterComplete(bool success) {
                  << ": 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,

dbus/object_manager.h

@@ -328,14 +328,16 @@ class CHROME_DBUS_EXPORT ObjectManager final
   // |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.