Enable auto-refreshing of the gcm-internals page whenever a GCM activity is...

Enable auto-refreshing of the gcm-internals page whenever a GCM activity is recorded, and fix a bug that previously causes this cl to crash the browser upon signin.

Basically GCMStatsRecorder::RecordRegistrationResponse() method did not check whether is_recording_ is true or not, so it always records. When the user signs in, a registration event is automatically triggered, but since the gcm-internals page has not been loaded yet, a callback object is still NULL. Therefore recorder calling this callback will crash the browser. I have added the check as well as test case for this scenario.

BUG=341256, 371259

Review URL: https://codereview.chromium.org/277743002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@269622 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/services/gcm/gcm_service.cc

@@ -146,6 +146,7 @@ class GCMService::IOWorker : public GCMClient::Delegate {
       const std::string& app_id,
       const GCMClient::SendErrorDetails& send_error_details) OVERRIDE;
   virtual void OnGCMReady() OVERRIDE;
+  virtual void OnActivityRecorded() OVERRIDE;
 
   // Called on IO thread.
   void Initialize(scoped_ptr<GCMClientFactory> gcm_client_factory,
@@ -296,6 +297,13 @@ void GCMService::IOWorker::OnGCMReady() {
                                               service_));
 }
 
+void GCMService::IOWorker::OnActivityRecorded() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+  // When an activity is recorded, get all the stats and refresh the UI of
+  // gcm-internals page.
+  GetGCMStatistics(false);
+}
+
 void GCMService::IOWorker::Load(const base::WeakPtr<GCMService>& service) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
 
@@ -844,7 +852,12 @@ GCMAppHandler* GCMService::GetAppHandler(const std::string& app_id) {
 void GCMService::GetGCMStatisticsFinished(
     GCMClient::GCMStatistics stats) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-  request_gcm_statistics_callback_.Run(stats);
+
+  // Normally request_gcm_statistics_callback_ would not be null.
+  if (!request_gcm_statistics_callback_.is_null())
+    request_gcm_statistics_callback_.Run(stats);
+  else
+    LOG(WARNING) << "request_gcm_statistics_callback_ is NULL.";
 }
 
 }  // namespace gcm

google_apis/gcm/gcm_client.h

@@ -158,6 +158,10 @@ class GCM_EXPORT GCMClient {
     // finished loading from the GCM store and retrieved the device check-in
     // from the server if it hadn't yet.
     virtual void OnGCMReady() = 0;
+
+    // Called when activities are being recorded and a new activity has just
+    // been recorded.
+    virtual void OnActivityRecorded() = 0;
   };
 
   GCMClient();

google_apis/gcm/gcm_client_impl.cc

@@ -213,7 +213,7 @@ void GCMClientImpl::Initialize(
     const scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner,
     const scoped_refptr<net::URLRequestContextGetter>&
         url_request_context_getter,
-    Delegate* delegate) {
+    GCMClient::Delegate* delegate) {
   DCHECK_EQ(UNINITIALIZED, state_);
   DCHECK(url_request_context_getter);
   DCHECK(delegate);
@@ -232,6 +232,8 @@ void GCMClientImpl::Initialize(
 
   delegate_ = delegate;
 
+  recorder_.SetDelegate(this);
+
   state_ = INITIALIZED;
 }
 
@@ -666,6 +668,10 @@ GCMClient::GCMStatistics GCMClientImpl::GetStatistics() const {
   return stats;
 }
 
+void GCMClientImpl::OnActivityRecorded() {
+  delegate_->OnActivityRecorded();
+}
+
 void GCMClientImpl::OnMessageReceivedFromMCS(const gcm::MCSMessage& message) {
   switch (message.tag()) {
     case kLoginResponseTag:

google_apis/gcm/gcm_client_impl.h

@@ -73,7 +73,8 @@ class GCM_EXPORT GCMInternalsBuilder {
 // with MCS) and other pieces of GCM infrastructure like Registration and
 // Checkins. It also allows for registering user delegates that host
 // applications that send and receive messages.
-class GCM_EXPORT GCMClientImpl : public GCMClient {
+class GCM_EXPORT GCMClientImpl
+    : public GCMClient, public GCMStatsRecorder::Delegate {
  public:
   explicit GCMClientImpl(scoped_ptr<GCMInternalsBuilder> internals_builder);
   virtual ~GCMClientImpl();
@@ -86,7 +87,7 @@ class GCM_EXPORT GCMClientImpl : public GCMClient {
       const scoped_refptr<base::SequencedTaskRunner>& blocking_task_runner,
       const scoped_refptr<net::URLRequestContextGetter>&
           url_request_context_getter,
-      Delegate* delegate) OVERRIDE;
+      GCMClient::Delegate* delegate) OVERRIDE;
   virtual void Load() OVERRIDE;
   virtual void Stop() OVERRIDE;
   virtual void CheckOut() OVERRIDE;
@@ -99,6 +100,7 @@ class GCM_EXPORT GCMClientImpl : public GCMClient {
   virtual void SetRecording(bool recording) OVERRIDE;
   virtual void ClearActivityLogs() OVERRIDE;
   virtual GCMStatistics GetStatistics() const OVERRIDE;
+  virtual void OnActivityRecorded() OVERRIDE;
 
  private:
   // State representation of the GCMClient.
@@ -235,7 +237,7 @@ class GCM_EXPORT GCMClientImpl : public GCMClient {
   // State of the GCM Client Implementation.
   State state_;
 
-  Delegate* delegate_;
+  GCMClient::Delegate* delegate_;
 
   // Device checkin info (android ID and security token used by device).
   CheckinInfo device_checkin_info_;

google_apis/gcm/gcm_client_impl_unittest.cc

@@ -250,6 +250,7 @@ class GCMClientImplTest : public testing::Test,
       const std::string& app_id,
       const gcm::GCMClient::SendErrorDetails& send_error_details) OVERRIDE;
   virtual void OnGCMReady() OVERRIDE;
+  virtual void OnActivityRecorded() OVERRIDE {}
 
   GCMClientImpl* gcm_client() const { return gcm_client_.get(); }
   FakeMCSClient* mcs_client() const {

google_apis/gcm/monitoring/gcm_stats_recorder.cc

@@ -184,7 +184,7 @@ GCMStatsRecorder::RecordedActivities::RecordedActivities() {
 GCMStatsRecorder::RecordedActivities::~RecordedActivities() {
 }
 
-GCMStatsRecorder::GCMStatsRecorder() : is_recording_(false) {
+GCMStatsRecorder::GCMStatsRecorder() : is_recording_(false), delegate_(NULL) {
 }
 
 GCMStatsRecorder::~GCMStatsRecorder() {
@@ -194,6 +194,10 @@ void GCMStatsRecorder::SetRecording(bool recording) {
   is_recording_ = recording;
 }
 
+void GCMStatsRecorder::SetDelegate(Delegate* delegate) {
+  delegate_ = delegate;
+}
+
 void GCMStatsRecorder::Clear() {
   checkin_activities_.clear();
   connection_activities_.clear();
@@ -202,6 +206,11 @@ void GCMStatsRecorder::Clear() {
   sending_activities_.clear();
 }
 
+void GCMStatsRecorder::NotifyActivityRecorded() {
+  if (delegate_)
+    delegate_->OnActivityRecorded();
+}
+
 void GCMStatsRecorder::RecordCheckin(
     const std::string& event,
     const std::string& details) {
@@ -210,6 +219,7 @@ void GCMStatsRecorder::RecordCheckin(
       &checkin_activities_, data);
   inserted_data->event = event;
   inserted_data->details = details;
+  NotifyActivityRecorded();
 }
 
 void GCMStatsRecorder::RecordCheckinInitiated(uint64 android_id) {
@@ -251,6 +261,7 @@ void GCMStatsRecorder::RecordConnection(
       &connection_activities_, data);
   inserted_data->event = event;
   inserted_data->details = details;
+  NotifyActivityRecorded();
 }
 
 void GCMStatsRecorder::RecordConnectionInitiated(const std::string& host) {
@@ -300,6 +311,7 @@ void GCMStatsRecorder::RecordRegistration(
   inserted_data->sender_ids = sender_ids;
   inserted_data->event = event;
   inserted_data->details = details;
+  NotifyActivityRecorded();
 }
 
 void GCMStatsRecorder::RecordRegistrationSent(
@@ -316,6 +328,8 @@ void GCMStatsRecorder::RecordRegistrationResponse(
     const std::string& app_id,
     const std::vector<std::string>& sender_ids,
     RegistrationRequest::Status status) {
+  if (!is_recording_)
+    return;
   RecordRegistration(app_id, JoinString(sender_ids, ","),
                      "Registration response received",
                      GetRegistrationStatusString(status));
@@ -378,6 +392,7 @@ void GCMStatsRecorder::RecordReceiving(
   inserted_data->message_byte_size = message_byte_size;
   inserted_data->event = event;
   inserted_data->details = details;
+  NotifyActivityRecorded();
 }
 
 void GCMStatsRecorder::RecordDataMessageReceived(
@@ -447,6 +462,7 @@ void GCMStatsRecorder::RecordSending(const std::string& app_id,
   inserted_data->message_id = message_id;
   inserted_data->event = event;
   inserted_data->details = details;
+  NotifyActivityRecorded();
 }
 
 void GCMStatsRecorder::RecordDataSentToWire(

google_apis/gcm/monitoring/gcm_stats_recorder.h

@@ -95,6 +95,15 @@ class GCM_EXPORT GCMStatsRecorder {
     std::vector<GCMStatsRecorder::SendingActivity> sending_activities;
   };
 
+  // A delegate interface that allows the GCMStatsRecorder instance to interact
+  // with its container.
+  class Delegate {
+   public:
+    // Called when the GCMStatsRecorder is recording activities and a new
+    // activity has just been recorded.
+    virtual void OnActivityRecorded() = 0;
+  };
+
   GCMStatsRecorder();
   virtual ~GCMStatsRecorder();
 
@@ -106,6 +115,9 @@ class GCM_EXPORT GCMStatsRecorder {
   // Turns recording on/off.
   void SetRecording(bool recording);
 
+  // Set a delegate to receive callback from the recorder.
+  void SetDelegate(Delegate* delegate);
+
   // Clear all recorded activities.
   void Clear();
 
@@ -219,19 +231,27 @@ class GCM_EXPORT GCMStatsRecorder {
   }
 
  protected:
+  // Notify the recorder delegate, if it exists, that an activity has been
+  // recorded.
+  void NotifyActivityRecorded();
+
   void RecordCheckin(const std::string& event,
                      const std::string& details);
+
   void RecordConnection(const std::string& event,
                         const std::string& details);
+
   void RecordRegistration(const std::string& app_id,
                           const std::string& sender_id,
                           const std::string& event,
                           const std::string& details);
+
   void RecordReceiving(const std::string& app_id,
                        const std::string& from,
                        int message_byte_size,
                        const std::string& event,
                        const std::string& details);
+
   void RecordSending(const std::string& app_id,
                      const std::string& receiver_id,
                      const std::string& message_id,
@@ -239,6 +259,7 @@ class GCM_EXPORT GCMStatsRecorder {
                      const std::string& details);
 
   bool is_recording_;
+  Delegate* delegate_;
 
   std::deque<CheckinActivity> checkin_activities_;
   std::deque<ConnectionActivity> connection_activities_;

google_apis/gcm/monitoring/gcm_stats_recorder_unittest.cc

@@ -123,6 +123,13 @@ class GCMStatsRecorderTest : public testing::Test {
     EXPECT_EQ(expected_count,
               static_cast<int>(recorder_.sending_activities().size()));
   }
+  void VerifyAllActivityQueueEmpty(const std::string& remark) {
+    EXPECT_TRUE(recorder_.checkin_activities().empty()) << remark;
+    EXPECT_TRUE(recorder_.connection_activities().empty()) << remark;
+    EXPECT_TRUE(recorder_.registration_activities().empty()) << remark;
+    EXPECT_TRUE(recorder_.receiving_activities().empty()) << remark;
+    EXPECT_TRUE(recorder_.sending_activities().empty()) << remark;
+  }
 
   void VerifyCheckinInitiated(const std::string& remark) {
     VerifyCheckin(recorder_.checkin_activities(),
@@ -354,9 +361,46 @@ TEST_F(GCMStatsRecorderTest, StartStopRecordingTest) {
 
   recorder_.SetRecording(false);
   EXPECT_FALSE(recorder_.is_recording());
+  recorder_.Clear();
+  VerifyAllActivityQueueEmpty("all cleared");
+
+  // Exercise every recording method below and verify that nothing is recorded.
+  recorder_.RecordCheckinInitiated(kAndroidId);
+  recorder_.RecordCheckinDelayedDueToBackoff(kDelay);
+  recorder_.RecordCheckinSuccess();
+  recorder_.RecordCheckinFailure(kCheckinStatus, true);
+  VerifyAllActivityQueueEmpty("no checkin");
+
+  recorder_.RecordConnectionInitiated(kHost);
+  recorder_.RecordConnectionDelayedDueToBackoff(kDelay);
+  recorder_.RecordConnectionSuccess();
+  recorder_.RecordConnectionFailure(kNetworkError);
+  recorder_.RecordConnectionResetSignaled(kReason);
+  VerifyAllActivityQueueEmpty("no registration");
+
+  recorder_.RecordRegistrationSent(kAppId, kSenderIds);
+  recorder_.RecordRegistrationResponse(kAppId, sender_ids_,
+                                       kRegistrationStatus);
+  recorder_.RecordRegistrationRetryRequested(kAppId, sender_ids_, kRetries);
+  recorder_.RecordUnregistrationSent(kAppId);
+  recorder_.RecordUnregistrationResponse(kAppId, kUnregistrationStatus);
+  recorder_.RecordUnregistrationRetryDelayed(kAppId, kDelay);
+  VerifyAllActivityQueueEmpty("no unregistration");
+
+  recorder_.RecordDataMessageReceived(kAppId, kFrom, kByteSize, true,
+                                      GCMStatsRecorder::DATA_MESSAGE);
+  recorder_.RecordDataMessageReceived(kAppId, kFrom, kByteSize, true,
+                                      GCMStatsRecorder::DELETED_MESSAGES);
+  recorder_.RecordDataMessageReceived(kAppId, kFrom, kByteSize, false,
+                                      GCMStatsRecorder::DATA_MESSAGE);
+  VerifyAllActivityQueueEmpty("no receiving");
+
   recorder_.RecordDataSentToWire(kAppId, kReceiverId, kMessageId, kQueuedSec);
-  VerifyRecordedSendingCount(1);
-  VerifyDataSentToWire("2nd call");
+  recorder_.RecordNotifySendStatus(kAppId, kReceiverId, kMessageId,
+                                   kMessageSendStatus, kByteSize, kTTL);
+  recorder_.RecordIncomingSendError(kAppId, kReceiverId, kMessageId);
+  recorder_.RecordDataSentToWire(kAppId, kReceiverId, kMessageId, kQueuedSec);
+  VerifyAllActivityQueueEmpty("no sending");
 }
 
 TEST_F(GCMStatsRecorderTest, ClearLogTest) {