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..7c0494fe3d45b9e4c12504fa44e1e6ce51af6e8a 100644
--- a/components/gcm_driver/gcm_client_impl.cc
+++ b/components/gcm_driver/gcm_client_impl.cc
@@ -10,6 +10,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ -22,10 +23,13 @@
 #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 "components/variations/variations_associated_data.h"
 #include "google_apis/gcm/base/encryptor.h"
 #include "google_apis/gcm/base/mcs_message.h"
 #include "google_apis/gcm/base/mcs_util.h"
@@ -79,6 +83,13 @@ enum ResetStoreError {
   RESET_STORE_ERROR_COUNT
 };
 
+// Indicates what type of message receipts should be sent to the GCM.
+enum MessageReceiptsToSend {
+  SEND_NONE = 0,
+  SEND_FAILURES = 1,
+  SEND_ALL = 2,
+};
+
 const char kGCMScope[] = "GCM";
 const int kMaxRegistrationRetries = 5;
 const int kMaxUnregistrationRetries = 5;
@@ -92,6 +103,17 @@ const char kSubtypeKey[] = "subtype";
 const char kSendMessageFromValue[] = "gcm@chrome.com";
 const int64_t kDefaultUserSerialNumber = 0LL;
 const int kDestroyGCMStoreDelayMS = 5 * 60 * 1000;  // 5 minutes.
+const char kGcmMessageTypeKey[] = "type";

[Peter Beverloo, 2017/02/13 13:47:07]

Q: Why id this different from kMessageTypeKey?

[harkness, 2017/02/14 19:22:29]

Actually, I don't remember where I got this key from, but you're right they
should be the same.

+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;
+const base::Feature kGCMMessageReceiptsFeature{
+    "GCMMessageReceiptsFeature", base::FEATURE_ENABLED_BY_DEFAULT};

[Peter Beverloo, 2017/02/13 13:47:06]

Rather than having a trial that's always enabled and has three values {none,
failures, all}, have you considered having a trial that can be disabled {none}
and enabled w/ a value of {failures, all}?

[harkness, 2017/02/14 19:22:28]

I did consider that, and functionally they're identical. This one just make a
bit more sense to me, but it's purely an aesthetic choice. I can change it if
you prefer.

[Ilya Sherman, 2017/02/22 23:17:02]

Either approach is okay from a functionality perspective, so I'll defer to your
team on which setup you prefer.  I do agree with Peter that if I were writing
this code, I'd probably treat "none" as a "disabled" state rather than as an
"enabled" state.

+const char kMessageReceiptsToSend[] = "message_receipts_to_send";
 
 GCMClient::Result ToGCMClientResult(MCSClient::MessageSendStatus status) {
   switch (status) {
@@ -1342,6 +1364,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;
@@ -1354,6 +1378,7 @@ void GCMClientImpl::HandleIncomingDataMessage(
     const mcs_proto::DataMessageStanza& data_message_stanza,
     MessageData& message_data) {
   std::string sender = data_message_stanza.from();
+  GCMMessageStatus status = GCMMessageStatus::GCM_NO_REGISTRATION;
 
   // Drop the message when the app is not registered for the sender of the
   // message.
@@ -1369,14 +1394,23 @@ void GCMClientImpl::HandleIncomingDataMessage(
     GCMRegistrationInfo* cached_gcm_registration =
         GCMRegistrationInfo::FromRegistrationInfo(
             gcm_registration_iter->first.get());
-    if (cached_gcm_registration &&
-        std::find(cached_gcm_registration->sender_ids.begin(),
-                  cached_gcm_registration->sender_ids.end(),
-                  sender) != cached_gcm_registration->sender_ids.end()) {
-      if (was_subtype)
-        DLOG(ERROR) << "GCM message for non-IID " << app_id << " used subtype";
-      else
-        registered = true;
+    if (cached_gcm_registration) {
+      if (std::find(cached_gcm_registration->sender_ids.begin(),
+                    cached_gcm_registration->sender_ids.end(),
+                    sender) != cached_gcm_registration->sender_ids.end()) {
+        if (was_subtype) {
+          DLOG(ERROR) << "GCM message for non-IID " << app_id
+                      << " used subtype";
+          status = GCMMessageStatus::GCM_INVALID_SUBTYPE;
+        } else {
+          registered = true;
+        }
+      } else {
+        DLOG(ERROR) << "GCM message for non-IID " << app_id
+                    << " had a reservation but the sender_id " << sender

[Peter Beverloo, 2017/02/13 13:47:07]

s/reservation/registration/

[harkness, 2017/02/14 19:22:28]

Done.

+                    << " was not valid.";
+        status = GCMMessageStatus::GCM_INVALID_SENDER_ID;
+      }
     }
   }
 
@@ -1393,6 +1427,7 @@ void GCMClientImpl::HandleIncomingDataMessage(
       if (was_subtype != InstanceIDUsesSubtypeForAppId(app_id)) {
         DLOG(ERROR) << "GCM message for " << app_id
                     << " incorrectly had was_subtype = " << was_subtype;
+        status = GCMMessageStatus::GCM_INVALID_SUBTYPE;
       } else {
         registered = true;
       }
@@ -1410,8 +1445,10 @@ 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, status);
     return;
+  }
 
   IncomingMessage incoming_message;
   incoming_message.sender_id = data_message_stanza.from();
@@ -1420,7 +1457,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 +1513,33 @@ bool GCMClientImpl::HasStandaloneRegisteredApp() const {
          !ExistsGCMRegistrationInMap(registrations_, kGCMAccountMapperAppId);
 }
 
+void GCMClientImpl::SendMessageReceipt(const std::string& message_id,
+                                       const std::string& app_id,
+                                       GCMMessageStatus status) {
+  // Only send message receipts to GCM if the trial is enabled.
+  int message_types_to_send = variations::GetVariationParamByFeatureAsInt(

[Peter Beverloo, 2017/02/13 13:47:06]

Use base::GetFieldTrialParamByFeatureAsInt() instead, the one you're using has
been marked as deprecated. That also avoids the DEPS change.

[harkness, 2017/02/14 19:22:28]

Ah, I wasn't aware of that one. I'll update it.

+      kGCMMessageReceiptsFeature, kMessageReceiptsToSend,
+      MessageReceiptsToSend::SEND_NONE);
+
+  if (message_types_to_send == MessageReceiptsToSend::SEND_NONE)
+    return;
+
+  if (message_types_to_send == MessageReceiptsToSend::SEND_FAILURES &&
+      (status == GCMMessageStatus::GCM_SUCCESS ||
+       status == GCMMessageStatus::GCM_UNRESOLVED))
+    return;

[Peter Beverloo, 2017/02/13 13:47:07]

nit: we tend to prefer switch() statements so that it's clearer to deal with odd
expected and unexpected values. Otherwise, use {} since this counts as a
multi-line statement. (Due to the condition.)

[harkness, 2017/02/14 19:22:28]

I agree in 95% of cases that a switch is better, but because the second if also
checks the status, and because both just return if they're true, I think it
reads better as if statements.

+
+  // Prepare a message to send to GCM which will log the status of the received
+  // 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());
+  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