Chromecast: reset crash rate limit once a reboot is scheduled

The rate limit was never cleared after a reboot, causing each new crash
report to trigger a new rate limit and another reboot.

Bug: internal b/157741351
Test: Run cast_crash_unittests
Change-Id: Ic92054f3aeb5d22e8f35dbc6d744e84e7a64b015
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2222966Reviewed-by: Yuchen Liu <yucliu@chromium.org>
Commit-Queue: Joshua LeVasseur <jlevasseur@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774303}

chromecast/crash/linux/minidump_uploader.cc

@@ -83,7 +83,6 @@ MinidumpUploader::MinidumpUploader(CastSysInfo* sys_info,
       system_version_(sys_info->GetSystemBuildNumber()),
       upload_location_(!server_url.empty() ? server_url
                                            : kCrashServerProduction),
-      last_upload_ratelimited_(true),
       reboot_scheduled_(false),
       filestate_initialized_(false),
       uploader_(uploader),
@@ -150,27 +149,21 @@ bool MinidumpUploader::DoWork() {
       ignore_and_erase_dump = true;
     }
 
-    // Ratelimiting persists across reboots, thus we to keep track of
-    // last_upload_ratelimited_ to detect when we first become ratelimited.
-    // Otherwise once ratelimited, we will reboot every time we try to upload a
-    // dump.
-    if (CanUploadDump()) {
-      last_upload_ratelimited_ = false;
+    // Ratelimiting persists across reboots.
+    if (reboot_scheduled_) {
+      LOG(INFO) << "Already rate limited with a reboot scheduled, removing "
+                   "crash dump";
+      ignore_and_erase_dump = true;
+    } else if (CanUploadDump()) {
+      // Record dump for ratelimiting
+      IncrementNumDumpsInCurrentPeriod();
     } else {
-      LOG(INFO) << "Can't upload dump: Ratelimited.";
+      LOG(INFO) << "Can't upload dump due to rate limit, will reboot";
+      ResetRateLimitPeriod();
       ignore_and_erase_dump = true;
-
-      // If the last upload wasn't ratelimited and this one is, then this is the
-      // first time we reached the ratelimit. Reboot the device.
-      if (!last_upload_ratelimited_)
-        reboot_scheduled_ = true;
-
-      last_upload_ratelimited_ = true;
+      reboot_scheduled_ = true;
     }
 
-    // Record dump for ratelimiting
-    IncrementNumDumpsInCurrentPeriod();
-
     if (ignore_and_erase_dump) {
       base::DeleteFile(dump_path, false);
       base::DeleteFile(log_path, false);

chromecast/crash/linux/minidump_uploader.h

@@ -56,11 +56,6 @@ class MinidumpUploader : public SynchronizedMinidumpManager {
 
   const std::string upload_location_;
 
-  // Whether or not we were ratelimited for the last upload.
-  // Used to detect when we first become ratelimited. Must be initialized to
-  // true to prevent reboot loops when ratelimited.
-  bool last_upload_ratelimited_;
-
   // Whether or not a reboot should be scheduled.
   bool reboot_scheduled_;
 

chromecast/crash/linux/minidump_uploader_unittest.cc

@@ -368,9 +368,12 @@ TEST_F(MinidumpUploaderTest, SchedulesRebootWhenRatelimited) {
   // ratelimit.
   EXPECT_CALL(mock_crash_uploader(),
               AddAttachment("log_file", logfile_path.value()))
-      .WillOnce(Return(true));
+      .WillOnce(Return(true))
+      .RetiresOnSaturation();
   EXPECT_CALL(mock_crash_uploader(), SetParameter(_, _)).Times(AtLeast(0));
-  EXPECT_CALL(mock_crash_uploader(), Upload(_)).WillOnce(Return(true));
+  EXPECT_CALL(mock_crash_uploader(), Upload(_))
+      .WillOnce(Return(true))
+      .RetiresOnSaturation();
   ASSERT_TRUE(uploader.UploadAllMinidumps());
   ASSERT_TRUE(uploader.reboot_scheduled());
 
@@ -387,8 +390,12 @@ TEST_F(MinidumpUploaderTest, SchedulesRebootWhenRatelimited) {
   MinidumpUploader uploader2(&sys_info_dummy(), "", &mock_crash_uploader(),
                              base::BindRepeating(&CreateFakePrefService, true));
 
-  // MinidumpUploader should not call CastCrashdumpUploader (due to ratelimit).
-  // Reboot should NOT be scheduled, as this is second ratelimit.
+  // Since a reboot was scheduled, the rate limit was cleared.  New uploads
+  // should be scheduled.
+  EXPECT_CALL(mock_crash_uploader(),
+              AddAttachment("log_file", logfile_path.value()))
+      .WillOnce(Return(true));
+  EXPECT_CALL(mock_crash_uploader(), Upload(_)).WillOnce(Return(true));
   ASSERT_TRUE(uploader2.UploadAllMinidumps());
   ASSERT_FALSE(uploader2.reboot_scheduled());
 

chromecast/crash/linux/synchronized_minidump_manager.cc

@@ -400,6 +400,11 @@ bool SynchronizedMinidumpManager::DecrementNumDumpsInCurrentPeriod() {
   return true;
 }
 
+void SynchronizedMinidumpManager::ResetRateLimitPeriod() {
+  SetRatelimitPeriodStart(metadata_.get(), base::Time::Now());
+  SetRatelimitPeriodDumps(metadata_.get(), 0);
+}
+
 bool SynchronizedMinidumpManager::CanUploadDump() {
   base::Time cur_time = base::Time::Now();
   base::Time period_start = GetRatelimitPeriodStart(metadata_.get());
@@ -414,10 +419,8 @@ bool SynchronizedMinidumpManager::CanUploadDump() {
       (cur_time < period_start &&
        cur_time.ToDoubleT() > kRatelimitPeriodSeconds) ||
       (cur_time - period_start).InSeconds() >= kRatelimitPeriodSeconds) {
-    period_start = cur_time;
-    period_dumps_count = 0;
-    SetRatelimitPeriodStart(metadata_.get(), period_start);
-    SetRatelimitPeriodDumps(metadata_.get(), period_dumps_count);
+    ResetRateLimitPeriod();
+    return true;
   }
 
   return period_dumps_count < kRatelimitPeriodMaxDumps;

chromecast/crash/linux/synchronized_minidump_manager.h

@@ -96,6 +96,9 @@ class SynchronizedMinidumpManager {
   // Returns true on success, false on error.
   bool DecrementNumDumpsInCurrentPeriod();
 
+  // Start a new rate-limit period, thus allowing crash uploads to proceed.
+  void ResetRateLimitPeriod();
+
   // Returns true when dumps uploaded in current rate limit period is less than
   // |kRatelimitPeriodMaxDumps|. Resets rate limit period if period time has
   // elapsed.