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
2 files changed
