Fix race when sending a Sharing message

There is a race when we wait on the response from FCM to give us the
message id of the sent message. If we for some reason get the ACK for
that message before the response from FCM, we discard the ACK and
timeout.

Bug: None
Change-Id: I35a1965aa5e1034af1b046099a60656ea61b59cf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2001302
Commit-Queue: Richard Knoll <knollr@chromium.org>
Reviewed-by: Alex Chau <alexchau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#732097}

chrome/browser/sharing/sharing_fcm_handler_unittest.cc

@@ -72,7 +72,7 @@ class MockSharingFCMSender : public SharingFCMSender {
                          /*sync_preference=*/nullptr,
                          /*vapid_key_manager=*/nullptr,
                          /*local_device_info_provider=*/nullptr) {}
-  ~MockSharingFCMSender() override {}
+  ~MockSharingFCMSender() override = default;
 
   MOCK_METHOD4(SendMessageToTargetInfo,
                void(syncer::DeviceInfo::SharingTargetInfo target,

chrome/browser/sharing/sharing_message_sender.cc

@@ -101,15 +101,26 @@ void SharingMessageSender::OnMessageSent(
     return;
   }
 
+  // Got a new message id, store it for later.
   message_guids_.emplace(*message_id, message_guid);
+
+  // Check if we got the ack while waiting for the FCM response.
+  auto cached_iter = cached_ack_response_messages_.find(*message_id);
+  if (cached_iter != cached_ack_response_messages_.end()) {
+    OnAckReceived(*message_id, std::move(cached_iter->second));
+    cached_ack_response_messages_.erase(cached_iter);
+  }
 }
 
 void SharingMessageSender::OnAckReceived(
     const std::string& message_id,
     std::unique_ptr<chrome_browser_sharing::ResponseMessage> response) {
   auto guid_iter = message_guids_.find(message_id);
-  if (guid_iter == message_guids_.end())
+  if (guid_iter == message_guids_.end()) {
+    // We don't have the guid yet, store the response until we receive it.
+    cached_ack_response_messages_.emplace(message_id, std::move(response));
     return;
+  }
 
   std::string message_guid = std::move(guid_iter->second);
   message_guids_.erase(guid_iter);

chrome/browser/sharing/sharing_message_sender.h

@@ -112,6 +112,10 @@ class SharingMessageSender {
   std::map<std::string, SentMessageMetadata> message_metadata_;
   // Map of FCM message_id to random GUID.
   std::map<std::string, std::string> message_guids_;
+  // Map of FCM message_id to received ACK response messages.
+  std::map<std::string,
+           std::unique_ptr<chrome_browser_sharing::ResponseMessage>>
+      cached_ack_response_messages_;
 
   // Registered delegates to send messages.
   std::map<DelegateType, std::unique_ptr<SendMessageDelegate>> send_delegates_;

chrome/browser/sharing/sharing_message_sender_unittest.cc

@@ -72,6 +72,23 @@ class MockSendMessageDelegate
                     SendMessageCallback callback));
 };
 
+// static
+syncer::DeviceInfo::SharingInfo CreateLocalSharingInfo() {
+  return syncer::DeviceInfo::SharingInfo(
+      {kSenderVapidFcmToken, kSenderP256dh, kSenderAuthSecret},
+      {"sender_id_fcm_token", "sender_id_p256dh", "sender_id_auth_secret"},
+      std::set<sync_pb::SharingSpecificFields::EnabledFeatures>());
+}
+
+// static
+syncer::DeviceInfo::SharingInfo CreateSharingInfo() {
+  return syncer::DeviceInfo::SharingInfo(
+      {kFCMToken, kP256dh, kAuthSecret},
+      {"sender_id_fcm_token", "sender_id_p256dh", "sender_id_auth_secret"},
+      std::set<sync_pb::SharingSpecificFields::EnabledFeatures>{
+          sync_pb::SharingSpecificFields::CLICK_TO_CALL});
+}
+
 }  // namespace
 
 class SharingMessageSenderTest : public testing::Test {
@@ -88,6 +105,20 @@ class SharingMessageSenderTest : public testing::Test {
   }
   ~SharingMessageSenderTest() override = default;
 
+  std::unique_ptr<syncer::DeviceInfo> SetupDevice() {
+    std::unique_ptr<syncer::DeviceInfo> device_info = CreateFakeDeviceInfo(
+        kReceiverGUID, kReceiverDeviceName, CreateSharingInfo());
+    fake_device_info_sync_service_.GetDeviceInfoTracker()->Add(
+        device_info.get());
+    fake_device_info_sync_service_.GetLocalDeviceInfoProvider()
+        ->GetMutableDeviceInfo()
+        ->set_sharing_info(CreateLocalSharingInfo());
+    sharing_sync_preference_.SetFCMRegistration(
+        SharingSyncPreference::FCMRegistration(kAuthorizedEntity,
+                                               base::Time::Now()));
+    return device_info;
+  }
+
  protected:
   content::BrowserTaskEnvironment task_environment_{
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
@@ -104,21 +135,6 @@ class SharingMessageSenderTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(SharingMessageSenderTest);
 };
 
-static syncer::DeviceInfo::SharingInfo CreateLocalSharingInfo() {
-  return syncer::DeviceInfo::SharingInfo(
-      {kSenderVapidFcmToken, kSenderP256dh, kSenderAuthSecret},
-      {"sender_id_fcm_token", "sender_id_p256dh", "sender_id_auth_secret"},
-      std::set<sync_pb::SharingSpecificFields::EnabledFeatures>());
-}
-
-static syncer::DeviceInfo::SharingInfo CreateSharingInfo() {
-  return syncer::DeviceInfo::SharingInfo(
-      {kFCMToken, kP256dh, kAuthSecret},
-      {"sender_id_fcm_token", "sender_id_p256dh", "sender_id_auth_secret"},
-      std::set<sync_pb::SharingSpecificFields::EnabledFeatures>{
-          sync_pb::SharingSpecificFields::CLICK_TO_CALL});
-}
-
 MATCHER_P(ProtoEquals, message, "") {
   if (!arg)
     return false;
@@ -132,15 +148,7 @@ MATCHER_P(ProtoEquals, message, "") {
 }  // namespace
 
 TEST_F(SharingMessageSenderTest, MessageSent_AckTimedout) {
-  std::unique_ptr<syncer::DeviceInfo> device_info = CreateFakeDeviceInfo(
-      kReceiverGUID, kReceiverDeviceName, CreateSharingInfo());
-  fake_device_info_sync_service_.GetDeviceInfoTracker()->Add(device_info.get());
-  fake_device_info_sync_service_.GetLocalDeviceInfoProvider()
-      ->GetMutableDeviceInfo()
-      ->set_sharing_info(CreateLocalSharingInfo());
-  sharing_sync_preference_.SetFCMRegistration(
-      SharingSyncPreference::FCMRegistration(kAuthorizedEntity,
-                                             base::Time::Now()));
+  std::unique_ptr<syncer::DeviceInfo> device_info = SetupDevice();
 
   base::MockCallback<SharingMessageSender::ResponseCallback> mock_callback;
   EXPECT_CALL(mock_callback,
@@ -173,15 +181,7 @@ TEST_F(SharingMessageSenderTest, MessageSent_AckTimedout) {
 }
 
 TEST_F(SharingMessageSenderTest, SendMessageToDevice_InternalError) {
-  std::unique_ptr<syncer::DeviceInfo> device_info = CreateFakeDeviceInfo(
-      kReceiverGUID, kReceiverDeviceName, CreateSharingInfo());
-  fake_device_info_sync_service_.GetDeviceInfoTracker()->Add(device_info.get());
-  fake_device_info_sync_service_.GetLocalDeviceInfoProvider()
-      ->GetMutableDeviceInfo()
-      ->set_sharing_info(CreateLocalSharingInfo());
-  sharing_sync_preference_.SetFCMRegistration(
-      SharingSyncPreference::FCMRegistration(kAuthorizedEntity,
-                                             base::Time::Now()));
+  std::unique_ptr<syncer::DeviceInfo> device_info = SetupDevice();
 
   base::MockCallback<SharingMessageSender::ResponseCallback> mock_callback;
   EXPECT_CALL(mock_callback,
@@ -193,7 +193,7 @@ TEST_F(SharingMessageSenderTest, SendMessageToDevice_InternalError) {
           base::TimeDelta time_to_live,
           chrome_browser_sharing::SharingMessage message,
           SharingFCMSender::SendMessageCallback callback) {
-        // FCM message not sent succesfully.
+        // FCM message not sent successfully.
         std::move(callback).Run(SharingSendMessageResult::kInternalError,
                                 base::nullopt);
 
@@ -214,15 +214,7 @@ TEST_F(SharingMessageSenderTest, SendMessageToDevice_InternalError) {
 }
 
 TEST_F(SharingMessageSenderTest, MessageSent_AckReceived) {
-  std::unique_ptr<syncer::DeviceInfo> device_info = CreateFakeDeviceInfo(
-      kReceiverGUID, kReceiverDeviceName, CreateSharingInfo());
-  fake_device_info_sync_service_.GetDeviceInfoTracker()->Add(device_info.get());
-  fake_device_info_sync_service_.GetLocalDeviceInfoProvider()
-      ->GetMutableDeviceInfo()
-      ->set_sharing_info(CreateLocalSharingInfo());
-  sharing_sync_preference_.SetFCMRegistration(
-      SharingSyncPreference::FCMRegistration(kAuthorizedEntity,
-                                             base::Time::Now()));
+  std::unique_ptr<syncer::DeviceInfo> device_info = SetupDevice();
 
   chrome_browser_sharing::SharingMessage sent_message;
   sent_message.mutable_click_to_call_message()->set_phone_number("999999");
@@ -275,6 +267,47 @@ TEST_F(SharingMessageSenderTest, MessageSent_AckReceived) {
       SharingMessageSender::DelegateType::kFCM, mock_callback.Get());
 }
 
+TEST_F(SharingMessageSenderTest, MessageSent_AckReceivedBeforeMessageId) {
+  std::unique_ptr<syncer::DeviceInfo> device_info = SetupDevice();
+
+  chrome_browser_sharing::SharingMessage sent_message;
+  sent_message.mutable_click_to_call_message()->set_phone_number("999999");
+
+  chrome_browser_sharing::ResponseMessage expected_response_message;
+  base::MockCallback<SharingMessageSender::ResponseCallback> mock_callback;
+  EXPECT_CALL(mock_callback,
+              Run(testing::Eq(SharingSendMessageResult::kSuccessful),
+                  ProtoEquals(expected_response_message)));
+
+  auto simulate_expected_ack_message_received =
+      [&](syncer::DeviceInfo::SharingTargetInfo target,
+          base::TimeDelta time_to_live,
+          chrome_browser_sharing::SharingMessage message,
+          SharingFCMSender::SendMessageCallback callback) {
+        // Simulate ack message received.
+        std::unique_ptr<chrome_browser_sharing::ResponseMessage>
+            response_message =
+                std::make_unique<chrome_browser_sharing::ResponseMessage>();
+        response_message->CopyFrom(expected_response_message);
+
+        sharing_message_sender_.OnAckReceived(kSenderMessageID,
+                                              std::move(response_message));
+
+        // Call FCM send success after receiving the ACK.
+        std::move(callback).Run(SharingSendMessageResult::kSuccessful,
+                                kSenderMessageID);
+      };
+
+  EXPECT_CALL(
+      *mock_sharing_fcm_sender_,
+      SendMessageToTargetInfo(testing::_, testing::_, testing::_, testing::_))
+      .WillOnce(testing::Invoke(simulate_expected_ack_message_received));
+
+  sharing_message_sender_.SendMessageToDevice(
+      *device_info.get(), kTimeToLive, std::move(sent_message),
+      SharingMessageSender::DelegateType::kFCM, mock_callback.Get());
+}
+
 TEST_F(SharingMessageSenderTest, NonExistingDelegate) {
   SharingMessageSender sharing_message_sender{
       &sharing_sync_preference_,