Provide a mechanism for the GCM driver to send message receipts to GCM.

components/gcm_driver/gcm_client_impl.cc

Index: components/gcm_driver/gcm_client_impl.cc
diff --git a/components/gcm_driver/gcm_client_impl.cc b/components/gcm_driver/gcm_client_impl.cc
index a18b5850b5d8bb4e78dca77ee4628e696dc3f20d..667a584d6bff5cba2b2eafd04b8c114a61a31fab 100644
--- a/components/gcm_driver/gcm_client_impl.cc
+++ b/components/gcm_driver/gcm_client_impl.cc
@@ -22,10 +22,12 @@
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/default_clock.h"
+#include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "components/crx_file/id_util.h"
 #include "components/gcm_driver/gcm_account_mapper.h"
 #include "components/gcm_driver/gcm_backoff_policy.h"
+#include "components/gcm_driver/gcm_message_status.h"
 #include "google_apis/gcm/base/encryptor.h"
 #include "google_apis/gcm/base/mcs_message.h"
 #include "google_apis/gcm/base/mcs_util.h"
@@ -48,6 +50,15 @@ namespace gcm {
 
 namespace {
 
+const char kGcmMessageTypeKey[] = "type";
+const char kMessageReceiptType[] = "message_receipt";
+const char kReceiptMessageIdKey[] = "message_id";
+const char kReceiptStatusKey[] = "status";
+// kReceiptGCMDestinationID is the destination ID which GCM listens to for
+// upstream receipt messages.
+const char kReceiptGCMDestinationID[] = "1029510549786@google.com";
+const int kReceiptTTLInSeconds = 10;

[Peter Beverloo, 2017/02/08 17:09:09]

micro nit: unify with the constant block around lines 93-105

[harkness, 2017/02/09 16:27:29]

Done.

+
 // Indicates a message type of the received message.
 enum MessageType {
   UNKNOWN,           // Undetermined type.
@@ -1342,6 +1353,8 @@ void GCMClientImpl::HandleIncomingMessage(const gcm::MCSMessage& message) {
       HandleIncomingSendError(app_id, data_message_stanza, message_data);
       break;
     case UNKNOWN:
+      SendMessageReceipt(data_message_stanza.id(), app_id,
+                         GCMMessageStatus::GCM_UNKNOWN_MESSAGE_TYPE);
       DVLOG(1) << "Unknown message_type received. Message ignored. "
                << "App ID: " << app_id << ".";
       break;
@@ -1410,8 +1423,11 @@ void GCMClientImpl::HandleIncomingDataMessage(
   recorder_.RecordDataMessageReceived(app_id, sender,
       data_message_stanza.ByteSize(), registered,
       GCMStatsRecorder::DATA_MESSAGE);
-  if (!registered)
+  if (!registered) {
+    SendMessageReceipt(data_message_stanza.id(), app_id,
+                       GCMMessageStatus::GCM_INVALID_SUBTYPE);

[Peter Beverloo, 2017/02/08 17:09:09]

INVALID_SUBTYPE is not really accurate - INVALID_REGISTRATION would be better,
since that's what |registered| actually represents. Alternatively you need to
consider the pair of [was_subtype, registered, registration type (v3/IID)] to
choose the appropriate value.

[harkness, 2017/02/09 16:27:30]

Thanks for reminding me, I was planning to rewrite this and add a lot more
values, but I completely forgot. Updated now.

     return;
+  }
 
   IncomingMessage incoming_message;
   incoming_message.sender_id = data_message_stanza.from();
@@ -1420,7 +1436,12 @@ void GCMClientImpl::HandleIncomingDataMessage(
   incoming_message.data = message_data;
   incoming_message.raw_data = data_message_stanza.raw_data();
 
-  delegate_->OnMessageReceived(app_id, incoming_message);
+  // Pass the message up to the GCMDriver. Also pass a callback which will send
+  // a receipt for this message if invoked with a message status.
+  delegate_->OnMessageReceived(app_id, incoming_message,
+                               base::Bind(&GCMClientImpl::SendMessageReceipt,
+                                          weak_ptr_factory_.GetWeakPtr(),
+                                          data_message_stanza.id(), app_id));
 }
 
 void GCMClientImpl::HandleIncomingDeletedMessages(
@@ -1471,4 +1492,22 @@ bool GCMClientImpl::HasStandaloneRegisteredApp() const {
          !ExistsGCMRegistrationInMap(registrations_, kGCMAccountMapperAppId);
 }
 
+void GCMClientImpl::SendMessageReceipt(const std::string& message_id,
+                                       const std::string& app_id,
+                                       GCMMessageStatus status) {
+  DCHECK(!app_id.empty());

[Peter Beverloo, 2017/02/08 17:09:09]

Could |app_id| (or |message_id|, which should get the same treatment) be the
empty string at this point because of a server error? If so, we should silently
bail out (maybe w/ LOG(ERROR)) instead of crashing the browser.

[harkness, 2017/02/09 16:27:30]

There are checks in some of the GCMDriver code, but those are all places where
the call was coming from outside the component, so it's reasonable to check. In
this case, it's always coming from GCM, so it shouldn't ever be empty. I've
removed the check.

+
+  // Prepare a message to send to GCM which will log the status of the received

[Peter Beverloo, 2017/02/08 17:09:09]

What do you think about guarding this behind a finch roll-out? We can have three
settings -

  1) No receipts.
  2) Only unsuccessful receipts.
  3) All receipts.

That'll allow us to roll the feature out progressively and give us a kill switch
at the same time.

[harkness, 2017/02/09 16:27:29]

Sounds like a good idea. I'll upload the patch set with all the other changes
and then upload another one with Finch changes.

+  // message. This is aggregated by GCM to provide better error alerting.
+  OutgoingMessage message;
+  message.time_to_live = kReceiptTTLInSeconds;
+  message.id =
+      base::Int64ToString(base::Time::NowFromSystemTime().ToInternalValue());

[Peter Beverloo, 2017/02/08 17:09:09]

How was this decided? base::Time stores milliseconds, so collisions are entirely
possible here. (Unlike other scheduled sub-systems such as heartbeat.)

Should we use a more unique value? base::GenerateGUID() can be your friend.

[harkness, 2017/02/09 16:27:30]

I'm using the time because that's the way GCMClientImpl::Send does it, and I
thought that it would be most important to be unique between the types of
messages that are being sent to GCM.

+  message.data[kGcmMessageTypeKey] = kMessageReceiptType;
+  message.data[kReceiptMessageIdKey] = message_id;
+  message.data[kReceiptStatusKey] = base::IntToString(static_cast<int>(status));
+
+  Send(app_id, kReceiptGCMDestinationID, message);
+}
+
 }  // namespace gcm