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 <[email protected]>
Reviewed-by: Alex Chau <[email protected]>
Cr-Commit-Position: refs/heads/master@{#732097}
diff --git a/chrome/browser/sharing/sharing_fcm_handler_unittest.cc b/chrome/browser/sharing/sharing_fcm_handler_unittest.cc
index 1e4d225..f2db507 100644
--- a/chrome/browser/sharing/sharing_fcm_handler_unittest.cc
+++ b/chrome/browser/sharing/sharing_fcm_handler_unittest.cc
@@ -72,7 +72,7 @@
/*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,
diff --git a/chrome/browser/sharing/sharing_message_sender.cc b/chrome/browser/sharing/sharing_message_sender.cc
index 33b62812..16eab0f 100644
--- a/chrome/browser/sharing/sharing_message_sender.cc
+++ b/chrome/browser/sharing/sharing_message_sender.cc
@@ -101,15 +101,26 @@
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);
diff --git a/chrome/browser/sharing/sharing_message_sender.h b/chrome/browser/sharing/sharing_message_sender.h
index 529ff73d..999c4d0 100644
--- a/chrome/browser/sharing/sharing_message_sender.h
+++ b/chrome/browser/sharing/sharing_message_sender.h
@@ -112,6 +112,10 @@
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_;
diff --git a/chrome/browser/sharing/sharing_message_sender_unittest.cc b/chrome/browser/sharing/sharing_message_sender_unittest.cc
index e10e48b..bfc1878a 100644
--- a/chrome/browser/sharing/sharing_message_sender_unittest.cc
+++ b/chrome/browser/sharing/sharing_message_sender_unittest.cc
@@ -72,6 +72,23 @@
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 @@
}
~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 @@
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 @@
} // 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, 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 @@
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, 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 @@
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_,
