ModuleRegistry: Delete Module after all are stopped

If a Module is stopped, it may still receive callback from lower level
Module, because the callback may have already been in lower level
Module's Handler queue. Instead of deleting the stopped Module
immediately, wait until all Modules are stopped.

Bug: 150174451
Test: bluetooth_test_gd
Change-Id: I0b55fe0ac7da09332d381d2192235339a9dc23fa

diff --git a/gd/module.cc b/gd/module.cc
index f12da97..a0fc31a 100644 (file)
--- a/gd/module.cc
+++ b/gd/module.cc
@@ -98,7 +98,10 @@ void ModuleRegistry::StopAll() {
     instance->second->handler_->Clear();
     instance->second->handler_->WaitUntilStopped(kModuleStopTimeout);
     instance->second->Stop();
-
+  }
+  for (auto it = start_order_.rbegin(); it != start_order_.rend(); it++) {
+    auto instance = started_modules_.find(*it);
+    ASSERT(instance != started_modules_.end());
     delete instance->second->handler_;
     delete instance->second;
     started_modules_.erase(instance);

diff --git a/gd/module_unittest.cc b/gd/module_unittest.cc
index d0efb5d..5d9564f 100644 (file)
--- a/gd/module_unittest.cc
+++ b/gd/module_unittest.cc
@@ -15,6 +15,8 @@
  */
 
 #include "module.h"
+#include "os/handler.h"
+#include "os/thread.h"
 
 #include "gtest/gtest.h"
 
@@ -39,6 +41,8 @@ class ModuleTest : public ::testing::Test {
   Thread* thread_;
 };
 
+os::Handler* test_module_no_dependency_handler = nullptr;
+
 class TestModuleNoDependency : public Module {
  public:
   static const ModuleFactory Factory;
@@ -50,10 +54,12 @@ class TestModuleNoDependency : public Module {
   void Start() override {
     // A module is not considered started until Start() finishes
     EXPECT_FALSE(GetModuleRegistry()->IsStarted<TestModuleNoDependency>());
+    test_module_no_dependency_handler = GetHandler();
   }
 
   void Stop() override {
     // A module is not considered stopped until after Stop() finishes
+    test_module_no_dependency_handler = nullptr;
     EXPECT_TRUE(GetModuleRegistry()->IsStarted<TestModuleNoDependency>());
   }
 };
@@ -62,6 +68,8 @@ const ModuleFactory TestModuleNoDependency::Factory = ModuleFactory([]() {
   return new TestModuleNoDependency();
 });
 
+os::Handler* test_module_one_dependency_handler = nullptr;
+
 class TestModuleOneDependency : public Module {
  public:
   static const ModuleFactory Factory;
@@ -76,9 +84,11 @@ class TestModuleOneDependency : public Module {
 
     // A module is not considered started until Start() finishes
     EXPECT_FALSE(GetModuleRegistry()->IsStarted<TestModuleOneDependency>());
+    test_module_one_dependency_handler = GetHandler();
   }
 
   void Stop() override {
+    test_module_one_dependency_handler = GetHandler();
     EXPECT_TRUE(GetModuleRegistry()->IsStarted<TestModuleNoDependency>());
 
     // A module is not considered stopped until after Stop() finishes
@@ -199,5 +209,18 @@ TEST_F(ModuleTest, two_dependencies) {
   EXPECT_FALSE(registry_->IsStarted<TestModuleTwoDependencies>());
 }
 
+void post_two_module_one_handler() {
+  std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  test_module_one_dependency_handler->Post(common::BindOnce([] { FAIL(); }));
+}
+
+TEST_F(ModuleTest, shutdown_with_unhandled_callback) {
+  ModuleList list;
+  list.add<TestModuleOneDependency>();
+  registry_->Start(&list, thread_);
+  test_module_no_dependency_handler->Post(common::BindOnce(&post_two_module_one_handler));
+  registry_->StopAll();
+}
+
 }  // namespace
 }  // namespace bluetooth