[Multidevice] Bind SecureMessage delegate callbacks to weak pointers.

Previously, callbacks passed into SecureMessage delegate methods were
being forwarded directly to the EasyUnlock dbus service, which the
delegate does not own. So, these callbacks can still be invoked after
the SecureMessage delegate object is destroyed.

In this CL, we bind callbacks sent to the dbus client to weak pointers.
This ensures that consumers of the SecureMessage delegate will not have
their callbacks invoked after the delegate is destroyed.

Manually tested CryptAuth Enrollment, DeviceSync, and SmartLock flows.

Fixed: 1163335
Change-Id: I69b24c675808312d4310f37ac830c7326c971275
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2616944
Commit-Queue: Josh Nohle <nohle@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Auto-Submit: Josh Nohle <nohle@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841674}

chromeos/components/multidevice/secure_message_delegate_impl.cc

@@ -49,29 +49,6 @@ std::string SigSchemeToString(securemessage::SigScheme scheme) {
   return std::string();
 }
 
-// Parses the serialized HeaderAndBody string returned by the DBus client, and
-// calls the corresponding SecureMessageDelegate unwrap callback.
-void HandleUnwrapResult(
-    SecureMessageDelegate::UnwrapSecureMessageCallback callback,
-    const std::string& unwrap_result) {
-  securemessage::HeaderAndBody header_and_body;
-  if (!header_and_body.ParseFromString(unwrap_result)) {
-    std::move(callback).Run(false, std::string(), securemessage::Header());
-  } else {
-    std::move(callback).Run(true, header_and_body.body(),
-                            header_and_body.header());
-  }
-}
-
-// The SecureMessageDelegate expects the keys in the reverse order returned by
-// the DBus client.
-void HandleKeyPairResult(
-    SecureMessageDelegate::GenerateKeyPairCallback callback,
-    const std::string& private_key,
-    const std::string& public_key) {
-  std::move(callback).Run(public_key, private_key);
-}
-
 }  // namespace
 
 // static
@@ -103,14 +80,17 @@ SecureMessageDelegateImpl::~SecureMessageDelegateImpl() {}
 void SecureMessageDelegateImpl::GenerateKeyPair(
     GenerateKeyPairCallback callback) {
   dbus_client_->GenerateEcP256KeyPair(
-      base::BindOnce(HandleKeyPairResult, std::move(callback)));
+      base::BindOnce(&SecureMessageDelegateImpl::OnGenerateKeyPairResult,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
 void SecureMessageDelegateImpl::DeriveKey(const std::string& private_key,
                                           const std::string& public_key,
                                           DeriveKeyCallback callback) {
-  dbus_client_->PerformECDHKeyAgreement(private_key, public_key,
-                                        std::move(callback));
+  dbus_client_->PerformECDHKeyAgreement(
+      private_key, public_key,
+      base::BindOnce(&SecureMessageDelegateImpl::OnDeriveKeyResult,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
 void SecureMessageDelegateImpl::CreateSecureMessage(
@@ -143,7 +123,10 @@ void SecureMessageDelegateImpl::CreateSecureMessage(
   options.encryption_type = EncSchemeToString(create_options.encryption_scheme);
   options.signature_type = SigSchemeToString(create_options.signature_scheme);
 
-  dbus_client_->CreateSecureMessage(payload, options, std::move(callback));
+  dbus_client_->CreateSecureMessage(
+      payload, options,
+      base::BindOnce(&SecureMessageDelegateImpl::OnCreateSecureMessageResult,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
 void SecureMessageDelegateImpl::UnwrapSecureMessage(
@@ -169,7 +152,41 @@ void SecureMessageDelegateImpl::UnwrapSecureMessage(
 
   dbus_client_->UnwrapSecureMessage(
       serialized_message, options,
-      base::BindOnce(&HandleUnwrapResult, std::move(callback)));
+      base::BindOnce(&SecureMessageDelegateImpl::OnUnwrapSecureMessageResult,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+}
+
+void SecureMessageDelegateImpl::OnGenerateKeyPairResult(
+    GenerateKeyPairCallback callback,
+    const std::string& private_key,
+    const std::string& public_key) {
+  // The SecureMessageDelegate expects the keys in the reverse order returned by
+  // the DBus client.
+  std::move(callback).Run(public_key, private_key);
+}
+
+void SecureMessageDelegateImpl::OnDeriveKeyResult(
+    DeriveKeyCallback callback,
+    const std::string& derived_key) {
+  std::move(callback).Run(derived_key);
+}
+
+void SecureMessageDelegateImpl::OnCreateSecureMessageResult(
+    CreateSecureMessageCallback callback,
+    const std::string& secure_message) {
+  std::move(callback).Run(secure_message);
+}
+
+void SecureMessageDelegateImpl::OnUnwrapSecureMessageResult(
+    UnwrapSecureMessageCallback callback,
+    const std::string& unwrap_result) {
+  securemessage::HeaderAndBody header_and_body;
+  if (!header_and_body.ParseFromString(unwrap_result)) {
+    std::move(callback).Run(false, std::string(), securemessage::Header());
+  } else {
+    std::move(callback).Run(true, header_and_body.body(),
+                            header_and_body.header());
+  }
 }
 
 }  // namespace multidevice

chromeos/components/multidevice/secure_message_delegate_impl.h

@@ -6,6 +6,7 @@
 #define CHROMEOS_COMPONENTS_MULTIDEVICE_SECURE_MESSAGE_DELEGATE_IMPL_H_
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "chromeos/components/multidevice/secure_message_delegate.h"
 
 namespace chromeos {
@@ -15,6 +16,8 @@ class EasyUnlockClient;
 namespace multidevice {
 
 // Concrete SecureMessageDelegate implementation.
+// Note: Callbacks are guaranteed to *not* be invoked after
+// SecureMessageDelegateImpl is destroyed.
 class SecureMessageDelegateImpl : public SecureMessageDelegate {
  public:
   class Factory {
@@ -49,9 +52,26 @@ class SecureMessageDelegateImpl : public SecureMessageDelegate {
  private:
   SecureMessageDelegateImpl();
 
+  // Processes results returned from the dbus client, if necessary, and invokes
+  // the SecureMessageDelegate callbacks with the processed results. Note: When
+  // invoking dbus client methods, we bind these functions to a weak pointer to
+  // ensure that these functions are not called after the
+  // SecureMessageDelegateImpl object is destroyed.
+  void OnGenerateKeyPairResult(GenerateKeyPairCallback callback,
+                               const std::string& private_key,
+                               const std::string& public_key);
+  void OnDeriveKeyResult(DeriveKeyCallback callback,
+                         const std::string& derived_key);
+  void OnCreateSecureMessageResult(CreateSecureMessageCallback callback,
+                                   const std::string& secure_message);
+  void OnUnwrapSecureMessageResult(UnwrapSecureMessageCallback callback,
+                                   const std::string& unwrap_result);
+
   // Not owned by this instance.
   chromeos::EasyUnlockClient* dbus_client_;
 
+  base::WeakPtrFactory<SecureMessageDelegateImpl> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(SecureMessageDelegateImpl);
 };