commit 3bf0e6b1396a527c849fc5537ec762a0f51108b8
author Leonid Baraz <lbaraz@chromium.org> Tue Feb 22 20:53:11 2022 +0000
committer Commit Bot <commit-bot@chromium.org> Mon Feb 28 03:05:25 2022 +0000
tree aaed6e111081b3cb8144fd78b7b83a2a6038e289
parent edbd6ed4bd86dd835d9684a6594bf7b7cab057d0

missive: Handle reused response_callback_.

It looks like the reported crash is caused by an attempt to call
response_callback_ after it was already called (There is a DCHECK there,
but it is inactive in official builds, and doing if instead is more reliable
while doing no harm).
It probably happens because callback provided to delegate was not always
called - this is also fixed here.

BUG=b:220894570
BUG=1300921
TEST=FEATURES="test" emerge-$BOARD missive

Change-Id: I4e1e943a97003590ca2cb4ac2082eeda937b702b
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3482180
Reviewed-by: Zach Trudo <zatrudo@google.com>
Tested-by: Leonid Baraz <lbaraz@chromium.org>
Auto-Submit: Leonid Baraz <lbaraz@chromium.org>
Commit-Queue: Leonid Baraz <lbaraz@chromium.org>
(cherry picked from commit f391f63648f3dcf5fcb876fdda2b54da9cf899f2)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3489330

missive/client/missive_client.cc

diff --git a/missive/client/missive_client.cc b/missive/client/missive_client.cc
index 7c1abd7..b9cb029 100644
--- a/missive/client/missive_client.cc
+++ b/missive/client/missive_client.cc
@@ -39,9 +39,7 @@
   MissiveClientImpl() : client_(origin_task_runner()) {}
   MissiveClientImpl(const MissiveClientImpl& other) = delete;
   MissiveClientImpl& operator=(const MissiveClientImpl& other) = delete;
-  ~MissiveClientImpl() override {
-    client_.SetAvailability(/*is_available=*/false);
-  }
+  ~MissiveClientImpl() override = default;
 
   void Init(dbus::Bus* const bus) {
     DCHECK(bus);
@@ -128,6 +126,7 @@
     // Implementation of DisconnectableClient::Delegate
     void DoCall(base::OnceClosure cb) final {
       DCHECK_CALLED_ON_VALID_SEQUENCE(owner_->origin_checker_);
+      base::ScopedClosureRunner autorun(std::move(cb));
       dbus::MethodCall method_call(missive::kMissiveServiceInterface,
                                    dbus_method_);
       dbus::MessageWriter writer(&method_call);
@@ -143,8 +142,8 @@
       owner_->missive_service_proxy_->CallMethod(
           &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
           base::BindOnce(
-              [](base::OnceClosure cb, base::WeakPtr<DBusDelegate> self,
-                 dbus::Response* response) {
+              [](base::ScopedClosureRunner autorun,
+                 base::WeakPtr<DBusDelegate> self, dbus::Response* response) {
                 if (!self) {
                   return;  // Delegate already deleted.
                 }
@@ -155,19 +154,20 @@
                   return;
                 }
                 self->response_ = response;
-                std::move(cb).Run();
               },
-              std::move(cb), weak_ptr_factory_.GetWeakPtr()));
+              std::move(autorun), weak_ptr_factory_.GetWeakPtr()));
     }
 
     // Process dBus response, if status is OK, or error otherwise.
     void Respond(Status status) final {
       DCHECK_CALLED_ON_VALID_SEQUENCE(owner_->origin_checker_);
+      if (!completion_callback_) {
+        return;
+      }
       if (status.ok()) {
         dbus::MessageReader reader(response_);
         status = ParseResponse(&reader);
       }
-      DCHECK(completion_callback_);
       std::move(completion_callback_).Run(status);
     }
 

missive/dbus/upload_client.cc

diff --git a/missive/dbus/upload_client.cc b/missive/dbus/upload_client.cc
index 3b42e00..394ad78 100644
--- a/missive/dbus/upload_client.cc
+++ b/missive/dbus/upload_client.cc
@@ -7,6 +7,7 @@
 #include <utility>
 
 #include <base/bind.h>
+#include <base/callback_helpers.h>
 #include <base/logging.h>
 #include <base/memory/scoped_refptr.h>
 #include "base/run_loop.h"
@@ -87,6 +88,7 @@
   // Implementation of DisconnectableClient::Delegate
   void DoCall(base::OnceClosure cb) final {
     bus_->AssertOnOriginThread();
+    base::ScopedClosureRunner autorun(std::move(cb));
     dbus::MethodCall method_call(
         chromeos::kChromeReportingServiceInterface,
         chromeos::kChromeReportingServiceUploadEncryptedRecordMethod);
@@ -103,7 +105,7 @@
     chrome_proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::BindOnce(
-            [](base::OnceClosure cb,
+            [](base::ScopedClosureRunner autorun,
                base::WeakPtr<UploadEncryptedRecordDelegate> self,
                dbus::Response* response) {
               if (!self) {
@@ -116,15 +118,16 @@
                 return;
               }
               self->response_ = response;
-              std::move(cb).Run();
             },
-            std::move(cb), weak_ptr_factory_.GetWeakPtr()));
+            std::move(autorun), weak_ptr_factory_.GetWeakPtr()));
   }
 
   // Process dBus response, if status is OK, or error otherwise.
   void Respond(Status status) final {
     bus_->AssertOnOriginThread();
-    DCHECK(response_callback_);
+    if (!response_callback_) {
+      return;
+    }
 
     if (!status.ok()) {
       std::move(response_callback_).Run(status);