Handlers should only be cleared and deleted once

- Removed handler clearance management from ModuleRegistry::StopAll().
- Ensured that handler instance new`ed and deleted in the same scope and file.
- Added a WaitUntilStopped() call after `stack_handler_->Clear()` call.

Test: m com.android.bt
Flag: com.android.bluetooth.flags.same_handler_for_all_modules
Bug: 393449774
Bug: 398195386
Bug: 402005761
Change-Id: Iada1114d547c1bc6394227503083104f8790ce21

6 files changed, 52 insertions, 10 deletions

diff --git a/system/gd/hal/hci_hal_android_test.cc b/system/gd/hal/hci_hal_android_test.cc
index 247f15745f..f128f508cf 100644
--- a/system/gd/hal/hci_hal_android_test.cc
+++ b/system/gd/hal/hci_hal_android_test.cc
@@ -20,6 +20,7 @@
 #include <queue>
 #include <thread>
 
+#include "com_android_bluetooth_flags.h"
 #include "hal/hci_backend.h"
 #include "hal/hci_hal.h"
 #include "os/thread.h"
@@ -79,10 +80,13 @@ protected:
   }
 
   void TearDown() override {
-    fake_registry_.StopAll();
     handler_->Clear();
-    delete thread_;
+    if (com::android::bluetooth::flags::same_handler_for_all_modules()) {
+      handler_->WaitUntilStopped(bluetooth::kHandlerStopTimeout);
+    }
+    fake_registry_.StopAll();
     delete handler_;
+    delete thread_;
   }
 
   HciHal* hal;
diff --git a/system/gd/hal/hci_hal_host_test.cc b/system/gd/hal/hci_hal_host_test.cc
index 4f4c6bc6d0..d4539d6c3b 100644
--- a/system/gd/hal/hci_hal_host_test.cc
+++ b/system/gd/hal/hci_hal_host_test.cc
@@ -32,6 +32,7 @@
 #include <utility>
 #include <vector>
 
+#include "com_android_bluetooth_flags.h"
 #include "hal/hci_hal.h"
 #include "hal/serialize_packet.h"
 #include "os/thread.h"
@@ -155,12 +156,15 @@ protected:
 
   void TearDown() override {
     hal_->unregisterIncomingPacketCallback();
+    handler_->Clear();
+    if (com::android::bluetooth::flags::same_handler_for_all_modules()) {
+      handler_->WaitUntilStopped(bluetooth::kHandlerStopTimeout);
+    }
     fake_registry_.StopAll();
+    delete handler_;
     close(fake_server_socket_);
-    handler_->Clear();
     delete fake_server_;
     delete thread_;
-    delete handler_;
   }
 
   void SetFakeServerSocketToBlocking() {
diff --git a/system/gd/module.cc b/system/gd/module.cc
index 8904f0c559..76b386bbfe 100644
--- a/system/gd/module.cc
+++ b/system/gd/module.cc
@@ -128,10 +128,18 @@ void ModuleRegistry::StopAll() {
     auto module = Get(*it);
     last_instance_ = "stopping " + module->ToString();
 
-    // Clear the handler before stopping the module to allow it to shut down gracefully.
-    log::info("Stopping Handler of Module {}", module->ToString());
-    module->handler_->Clear();
-    module->handler_->WaitUntilStopped(kModuleStopTimeout);
+    /*
+     * b/393449774 since we have now shifted to a single handler for all modules, we don't need
+     * to clear the handler here, it will be done in the respective teardown.
+     * Since we have a single handler, we need to make sure that the handler instance is deleted
+     * only once, otherwise we will see a crash as a handler can only be cleared once.
+     */
+    if (!com::android::bluetooth::flags::same_handler_for_all_modules()) {
+      // Clear the handler before stopping the module to allow it to shut down gracefully.
+      log::info("Stopping Handler of Module {}", module->ToString());
+      module->handler_->Clear();
+      module->handler_->WaitUntilStopped(kModuleStopTimeout);
+    }
     log::info("Stopping Module {}", module->ToString());
     module->Stop();
   }
@@ -144,7 +152,9 @@ void ModuleRegistry::StopAll() {
     auto instance = started_modules_.find(*it);
     log::assert_that(instance != started_modules_.end(),
                      "assert failed: instance != started_modules_.end()");
-    delete instance->second->handler_;
+    if (!com::android::bluetooth::flags::same_handler_for_all_modules()) {
+      delete instance->second->handler_;
+    }
     delete instance->second;
     started_modules_.erase(instance);
   }
@@ -165,4 +175,15 @@ os::Handler* ModuleRegistry::GetModuleHandler(const ModuleFactory* module) const
   return nullptr;
 }
 
+// Override the StopAll method to use the test thread and handler.
+// This function will take care of releasing the handler instances.
+void TestModuleRegistry::StopAll() {
+  os::Handler* handler = GetTestHandler();
+  handler->Clear();
+  if (com::android::bluetooth::flags::same_handler_for_all_modules()) {
+    handler->WaitUntilStopped(kHandlerStopTimeout);
+  }
+  ModuleRegistry::StopAll();  // call the base class StopAll
+  delete handler;
+}
 }  // namespace bluetooth
diff --git a/system/gd/module.h b/system/gd/module.h
index 816d455482..57e4db7d16 100644
--- a/system/gd/module.h
+++ b/system/gd/module.h
@@ -32,6 +32,9 @@
 #include "os/thread.h"
 
 namespace bluetooth {
+// Timeout for waiting for a handler to stop, used in Handler::WaitUntilStopped()
+constexpr std::chrono::milliseconds kHandlerStopTimeout = std::chrono::milliseconds(2000);
+
 namespace shim {
 class Stack;
 }  // namespace shim
@@ -191,6 +194,9 @@ public:
   os::Thread& GetTestThread() { return test_thread; }
   os::Handler* GetTestHandler() { return test_handler_; }
 
+  // Override the StopAll method to use the test thread and handler.
+  void StopAll();
+
   bool SynchronizeModuleHandler(const ModuleFactory* module,
                                 std::chrono::milliseconds timeout) const {
     return SynchronizeHandler(GetTestModuleHandler(module), timeout);
diff --git a/system/gd/module_unittest.cc b/system/gd/module_unittest.cc
index d1bf59ac73..8fb140c229 100644
--- a/system/gd/module_unittest.cc
+++ b/system/gd/module_unittest.cc
@@ -22,6 +22,7 @@
 #include <sstream>
 #include <string>
 
+#include "com_android_bluetooth_flags.h"
 #include "gtest/gtest.h"
 #include "os/handler.h"
 #include "os/thread.h"
@@ -41,9 +42,12 @@ protected:
 
   void TearDown() override {
     handler_->Clear();
+    if (com::android::bluetooth::flags::same_handler_for_all_modules()) {
+      handler_->WaitUntilStopped(kHandlerStopTimeout);
+    }
     delete registry_;
-    delete thread_;
     delete handler_;
+    delete thread_;
   }
 
   ModuleRegistry* registry_;
diff --git a/system/main/shim/stack.cc b/system/main/shim/stack.cc
index 14447d75b3..4560a6180f 100644
--- a/system/main/shim/stack.cc
+++ b/system/main/shim/stack.cc
@@ -170,6 +170,9 @@ void Stack::Stop() {
   is_running_ = false;
 
   stack_handler_->Clear();
+  if(com::android::bluetooth::flags::same_handler_for_all_modules()) {
+    stack_handler_->WaitUntilStopped(bluetooth::kHandlerStopTimeout);
+  }
 
   WakelockManager::Get().Acquire();