dbus: Fix ResponseSender crash caused by lifetime mismatch

`ExportedObject` could out live its method call callbacks.
In such case, method call callbacks is no op hence not running
ResponseSender and crash. This CL checks the edge case and
send back `DBUS_ERROR_UNKNOWN_METHOD` error response.

Find a simpler way to export method and use it to update
existing `NotSendingResponseCrash` as well as adding a new
`SendFailureForShortLivedObject` to cover the fix. And reverted
the previous changes in `TestService`

Change ResponseSender CHECK to LOG(FATAL) to make it easier
to debug crashes that are not uploaded (hence no crash keys).

Re-enabled the disabled crosier bluetoothe tests.

Cq-Include-Trybots: luci.chrome.try:chromeos-trogdor-chrome

Bug: 404601483, 400758194
Change-Id: I578a7b2e902b71351104ce3ec0195ed16920202b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6373547
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1436057}

chrome/browser/ash/bluetooth/bluetooth_integration_test.cc

@@ -88,12 +88,8 @@ IN_PROC_BROWSER_TEST_F(BluetoothIntegrationTest,
       WaitForState(kBluetoothPowerState, true));
 }
 
-/*
- * TODO(crbug.com/404601483): Reenable once bug is fixed. Was disabled due to
- * cascading integration test failure blocking 136.0.7078.0 uprev.
- */
 IN_PROC_BROWSER_TEST_F(BluetoothIntegrationTest,
-                       DISABLED_ToggleBluetoothFromQuickSettingsBluetoothPage) {
+                       ToggleBluetoothFromQuickSettingsBluetoothPage) {
   SetupContextWidget();
   RunTestSequence(
       ObserveState(kBluetoothPowerState, BluetoothPowerStateObserver::Create()),
@@ -133,12 +129,8 @@ IN_PROC_BROWSER_TEST_F(BluetoothIntegrationTest,
       WaitForState(kBluetoothPowerState, true));
 }
 
-/*
- * TODO(crbug.com/404601483): Reenable once bug is fixed. Was disabled due to
- * cascading integration test failure blocking 136.0.7078.0 uprev.
- */
 IN_PROC_BROWSER_TEST_F(BluetoothIntegrationTest,
-                       DISABLED_ToggleBluetoothFromOsSettings) {
+                       ToggleBluetoothFromOsSettings) {
   base::AddFeatureIdTagToTestResult(
       "screenplay-d3c7f622-a376-4ca1-9be2-47a004234655");
   SetupContextWidget();

dbus/bus.cc

@@ -595,7 +595,8 @@ bool Bus::ReleaseOwnership(const std::string& service_name) {
   internal::ScopedDBusError error;
   const int result = dbus_bus_release_name(connection_, service_name.c_str(),
                                            error.get());
-  if (result == DBUS_RELEASE_NAME_REPLY_RELEASED) {
+  if (result == DBUS_RELEASE_NAME_REPLY_RELEASED ||
+      result == DBUS_RELEASE_NAME_REPLY_NON_EXISTENT) {
     owned_service_names_.erase(found);
     return true;
   } else {

dbus/exported_object.cc

@@ -50,9 +50,8 @@ class ResponseSenderWrapper {
     SCOPED_CRASH_KEY_STRING32("ResponseSenderWrapper", "DBusMember", member_);
 
     // `sender_` must have run (or not set).
-    CHECK(sender_.is_null())
+    LOG_IF(FATAL, !sender_.is_null())
         << "ResponseSender did not run for " << interface_ << "." << member_;
-    ;
   }
 
   // Convenience factory.
@@ -312,11 +311,7 @@ DBusHandlerResult ExportedObject::HandleMessage(
                                   iter->second, std::move(method_call)));
   } else {
     // If the D-Bus thread is not used, just call the method directly.
-    MethodCall* method = method_call.get();
-    iter->second.Run(method, ResponseSenderWrapper::Create(
-                                 std::move(interface), std::move(member),
-                                 base::BindOnce(&ExportedObject::SendResponse,
-                                                this, std::move(method_call))));
+    RunMethod(iter->second, std::move(method_call));
   }
 
   // It's valid to say HANDLED here, and send a method response at a later
@@ -327,7 +322,18 @@ DBusHandlerResult ExportedObject::HandleMessage(
 void ExportedObject::RunMethod(const MethodCallCallback& method_call_callback,
                                std::unique_ptr<MethodCall> method_call) {
   bus_->AssertOnOriginThread();
+
   MethodCall* method = method_call.get();
+
+  // Edge case that `method_call_callback` may be canceled when the method
+  // call message is processed. Send an error response.
+  if (method_call_callback.IsCancelled()) {
+    SendResponse(std::move(method_call), ErrorResponse::FromMethodCall(
+                                             method, DBUS_ERROR_UNKNOWN_METHOD,
+                                             "Method is no longer available"));
+    return;
+  }
+
   method_call_callback.Run(
       method, ResponseSenderWrapper::Create(
                   method->GetInterface(), method->GetMember(),

dbus/exported_object_unittest.cc

@@ -4,20 +4,82 @@
 
 #include "dbus/exported_object.h"
 
+#include <string>
+
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "base/run_loop.h"
 #include "base/test/bind.h"
 #include "base/test/gtest_util.h"
 #include "base/test/task_environment.h"
+#include "base/uuid.h"
 #include "dbus/bus.h"
 #include "dbus/message.h"
 #include "dbus/object_proxy.h"
-#include "dbus/test_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace dbus {
 namespace {
 
+inline constexpr char kObjectPath[] = "/org/chromium/TestObject";
+inline constexpr char kInterface[] = "org.chromium.TestObject";
+
+inline constexpr char kMethodNeverRun[] = "NeverRun";
+inline constexpr char kMethodNotSendingResponse[] = "NotSendingResponse";
+
+class TestObject {
+ public:
+  explicit TestObject(Bus* bus) : bus_(bus) {}
+  ~TestObject() = default;
+
+  ExportedObject* ExportMethods() {
+    exported_object_ = bus_->GetExportedObject(ObjectPath(kObjectPath));
+
+    ExportOneMethod(kMethodNotSendingResponse,
+                    base::BindRepeating(&TestObject::NotSendingResponse,
+                                        weak_ptr_factory_.GetWeakPtr()));
+    ExportOneMethod(kMethodNeverRun,
+                    base::BindRepeating(&TestObject::NeverRun,
+                                        weak_ptr_factory_.GetWeakPtr()));
+
+    return exported_object_.get();
+  }
+
+ private:
+  void ExportOneMethod(
+      const std::string& method_name,
+      const ExportedObject::MethodCallCallback& method_callback) {
+    ASSERT_TRUE(exported_object_);
+
+    base::RunLoop run_loop;
+    exported_object_->ExportMethod(
+        kInterface, method_name, method_callback,
+        base::BindLambdaForTesting([&](const std::string& interface_name,
+                                       const std::string& method_name,
+                                       bool success) {
+          ASSERT_TRUE(success);
+          run_loop.Quit();
+        }));
+    run_loop.Run();
+  }
+
+  void NotSendingResponse(MethodCall* method_call,
+                          ExportedObject::ResponseSender response_sender) {
+    // Intentionally not invoking `response_sender`. `ExportedObject` would
+    // catch the case and crash.
+  }
+
+  void NeverRun(MethodCall* method_call,
+                ExportedObject::ResponseSender response_sender) {
+    ADD_FAILURE() << "NeverRun should never run.";
+  }
+
+  raw_ptr<Bus> bus_;
+  scoped_refptr<ExportedObject> exported_object_;
+
+  base::WeakPtrFactory<TestObject> weak_ptr_factory_{this};
+};
+
 class ExportedObjectTest : public testing::Test {
  protected:
   ExportedObjectTest() = default;
@@ -39,33 +101,63 @@ class ExportedObjectTest : public testing::Test {
 
 // Tests that calling a method that doesn't send a response crashes.
 TEST_F(ExportedObjectTest, NotSendingResponseCrash) {
-  TestService::Options options;
-  TestService test_service(options);
-  ObjectProxy* object_proxy = bus_->GetObjectProxy(
-      test_service.service_name(), ObjectPath("/org/chromium/TestObject"));
+  const std::string service_name =
+      "org.chromium.NotSendingResponse" +
+      base::Uuid::GenerateRandomV4().AsLowercaseString();
+  ASSERT_TRUE(bus_->Connect());
+  bus_->RequestOwnershipAndBlock(service_name,
+                                 Bus::REQUIRE_PRIMARY_ALLOW_REPLACEMENT);
 
-  base::RunLoop run_loop;
-  object_proxy->WaitForServiceToBeAvailable(
-      base::BindLambdaForTesting([&](bool service_available) {
-        ASSERT_TRUE(service_available);
-        run_loop.Quit();
-      }));
-
-  ASSERT_TRUE(test_service.StartService());
-  test_service.WaitUntilServiceIsStarted();
-  ASSERT_TRUE(test_service.has_ownership());
-
-  // Spin a loop and wait for `TestService` to be available.
-  run_loop.Run();
+  TestObject test_object(bus_.get());
+  test_object.ExportMethods();
 
   // Call the bad method and expect a CHECK crash.
-  MethodCall method_call("org.chromium.TestInterface",
-                         "NotSendingResponseCrash");
-  base::expected<std::unique_ptr<Response>, Error> result;
-  EXPECT_CHECK_DEATH_WITH(result = object_proxy->CallMethodAndBlock(
-                              &method_call, ObjectProxy::TIMEOUT_USE_DEFAULT),
+  auto call_bad_method = [&]() {
+    ObjectProxy* object_proxy =
+        bus_->GetObjectProxy(service_name, ObjectPath(kObjectPath));
+    MethodCall method_call(kInterface, kMethodNotSendingResponse);
+    base::RunLoop run_loop;
+    object_proxy->CallMethod(&method_call, ObjectProxy::TIMEOUT_USE_DEFAULT,
+                             base::BindLambdaForTesting(
+                                 [&](Response* response) { run_loop.Quit(); }));
+    run_loop.Run();
+  };
+
+  EXPECT_CHECK_DEATH_WITH(call_bad_method(),
                           "ResponseSender did not run for "
-                          "org.chromium.TestInterface.NotSendingResponseCrash");
+                          "org.chromium.TestObject.NotSendingResponse");
+}
+
+// Tests that an error response is sent when calling a method after a short
+// lived object destruction but before its `ExportedObject` gone.
+TEST_F(ExportedObjectTest, SendFailureForShortLivedObject) {
+  const std::string service_name =
+      "org.chromium.ShortLived" +
+      base::Uuid::GenerateRandomV4().AsLowercaseString();
+  ASSERT_TRUE(bus_->Connect());
+  bus_->RequestOwnershipAndBlock(service_name,
+                                 Bus::REQUIRE_PRIMARY_ALLOW_REPLACEMENT);
+
+  auto short_lived = std::make_unique<TestObject>(bus_.get());
+
+  // Hold on to `ExportedObject` and destroy the short lived.
+  scoped_refptr<ExportedObject> expored_object = short_lived->ExportMethods();
+  short_lived.reset();
+
+  // Call `NeverRun`. It should not run and an error response should be sent.
+  ObjectProxy* object_proxy =
+      bus_->GetObjectProxy(service_name, ObjectPath(kObjectPath));
+  MethodCall method_call(kInterface, kMethodNeverRun);
+  base::RunLoop run_loop;
+  object_proxy->CallMethodWithErrorResponse(
+      &method_call, ObjectProxy::TIMEOUT_USE_DEFAULT,
+      base::BindLambdaForTesting([&](Response* response, ErrorResponse* error) {
+        ASSERT_FALSE(response);
+        ASSERT_TRUE(error);
+        EXPECT_EQ(DBUS_ERROR_UNKNOWN_METHOD, error->GetErrorName());
+        run_loop.Quit();
+      }));
+  run_loop.Run();
 }
 
 }  // namespace

dbus/test_service.cc

@@ -29,8 +29,8 @@
 namespace dbus {
 
 // Echo, SlowEcho, AsyncEcho, BrokenMethod, GetAll, Get, Set, PerformAction,
-// GetManagedObjects, NotSendingResponseCrash.
-constexpr int TestService::kNumMethodsToExport = 10;
+// GetManagedObjects.
+constexpr int TestService::kNumMethodsToExport = 9;
 
 TestService::Options::Options()
     : request_ownership_options(Bus::REQUIRE_PRIMARY) {
@@ -259,13 +259,6 @@ void TestService::Run(base::RunLoop* run_loop) {
       base::BindOnce(&TestService::OnExported, base::Unretained(this)));
   ++num_methods;
 
-  exported_object_->ExportMethod(
-      "org.chromium.TestInterface", "NotSendingResponseCrash",
-      base::BindRepeating(&TestService::NotSendingResponseCrash,
-                          base::Unretained(this)),
-      base::BindOnce(&TestService::OnExported, base::Unretained(this)));
-  ++num_methods;
-
   // Just print an error message as we don't want to crash tests.
   // Tests will fail at a call to WaitUntilServiceIsStarted().
   if (num_methods != kNumMethodsToExport) {
@@ -504,12 +497,6 @@ void TestService::OwnershipRegained(
   PerformActionResponse(method_call, std::move(response_sender));
 }
 
-void TestService::NotSendingResponseCrash(
-    MethodCall* method_call,
-    dbus::ExportedObject::ResponseSender response_sender) {
-  // Not invoking `response_sender` and CHECK crash.
-}
-
 void TestService::GetManagedObjects(
     MethodCall* method_call,
     ExportedObject::ResponseSender response_sender) {

dbus/test_service.h

@@ -208,12 +208,6 @@ class TestService : public base::Thread {
       dbus::ExportedObject::ResponseSender response_sender,
       bool success);
 
-  // Simulate DBus service that uses ExportedObject but forgets to send a
-  // response. This would trigger a CHECK crash.
-  void NotSendingResponseCrash(
-      MethodCall* method_call,
-      dbus::ExportedObject::ResponseSender response_sender);
-
   // Name of this service.
   std::string service_name_;