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

components/gcm_driver/gcm_client_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/gcm_driver/gcm_client_impl.h"
 
 #include <stddef.h>
 
 #include <memory>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #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"
 #include "google_apis/gcm/engine/checkin_request.h"
 #include "google_apis/gcm/engine/connection_factory_impl.h"
 #include "google_apis/gcm/engine/gcm_registration_request_handler.h"
 #include "google_apis/gcm/engine/gcm_store_impl.h"
 #include "google_apis/gcm/engine/gcm_unregistration_request_handler.h"
 #include "google_apis/gcm/engine/instance_id_delete_token_request_handler.h"
 #include "google_apis/gcm/engine/instance_id_get_token_request_handler.h"
 #include "google_apis/gcm/monitoring/gcm_stats_recorder.h"
 #include "google_apis/gcm/protocol/checkin.pb.h"
 #include "google_apis/gcm/protocol/mcs.pb.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_transaction_factory.h"
 #include "net/url_request/url_request_context.h"
 #include "url/gurl.h"
 
 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.
   DATA_MESSAGE,      // Regular data message.
   DELETED_MESSAGES,  // Messages were deleted on the server.
   SEND_ERROR,        // Error sending a message.
 };
 
 enum OutgoingMessageTTLCategory {
   TTL_ZERO,
@@ skipping 1274 matching lines @@
       HandleIncomingDataMessage(app_id, use_subtype, data_message_stanza,
                                 message_data);
       break;
     case DELETED_MESSAGES:
       HandleIncomingDeletedMessages(app_id, data_message_stanza, message_data);
       break;
     case SEND_ERROR:
       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;
   }
 }
 
 void GCMClientImpl::HandleIncomingDataMessage(
     const std::string& app_id,
     bool was_subtype,
     const mcs_proto::DataMessageStanza& data_message_stanza,
@@ skipping 48 matching lines @@
   if (registered) {
     UMA_HISTOGRAM_BOOLEAN("GCM.DataMessageReceived", true);
     bool has_collapse_key =
         data_message_stanza.has_token() && !data_message_stanza.token().empty();
     UMA_HISTOGRAM_BOOLEAN("GCM.DataMessageReceivedHasCollapseKey",
                           has_collapse_key);
   }
   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();
   if (data_message_stanza.has_token())
     incoming_message.collapse_key = data_message_stanza.token();
   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(
     const std::string& app_id,
     const mcs_proto::DataMessageStanza& data_message_stanza,
     MessageData& message_data) {
   int deleted_count = 0;
   MessageData::iterator count_iter = message_data.find(kDeletedCountKey);
   if (count_iter != message_data.end()) {
     if (!base::StringToInt(count_iter->second, &deleted_count))
@@ skipping 30 matching lines @@
 
 bool GCMClientImpl::HasStandaloneRegisteredApp() const {
   if (registrations_.empty())
     return false;
   // Note that account mapper is not counted as a standalone app since it is
   // automatically started when other app uses GCM.
   return registrations_.size() > 1 ||
          !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