Check the SRT logs upload time for an entire queue of SRT invocations at once.

chrome/browser/safe_browsing/srt_fetcher_win.cc

 // Copyright 2015 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 "chrome/browser/safe_browsing/srt_fetcher_win.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 #include <memory>
@@ skipping 895 matching lines @@
     const base::Time last_time_triggered = base::Time::FromInternalValue(
         local_state->GetInt64(prefs::kSwReporterLastTimeTriggered));
     const base::Time next_trigger(
         last_time_triggered +
         base::TimeDelta::FromDays(days_between_reporter_runs_));
     if (!pending_invocations_.empty() &&
         (next_trigger <= now ||
          // Also make sure the kSwReporterLastTimeTriggered value is not set in
          // the future.
          last_time_triggered > now)) {
+      const base::Time now = base::Time::Now();

[ftirelo, 2017/02/06 14:13:24]

Please reuse |now| from line 905.

[Joe Mason, 2017/02/06 18:07:09]

Good catch. Done.

+      const base::Time last_time_sent_logs = base::Time::FromInternalValue(
+          local_state->GetInt64(prefs::kSwReporterLastTimeSentReport));
+      const base::Time next_time_send_logs =
+          last_time_sent_logs +
+          base::TimeDelta::FromDays(kDaysBetweenReporterLogsSent);
+      // Send the logs for this whole queue of invocations if the last send is
+      // in the future or if logs have been sent at least
+      // |kSwReporterLastTimeSentReport| days ago. The former is intended as a
+      // measure for failure recovery, in case the time in local state is
+      // incorrectly set to the future.
+      in_logs_upload_period_ =
+          last_time_sent_logs > now || next_time_send_logs <= now;
+
       DCHECK(current_invocations_.empty());
       current_invocations_ = pending_invocations_;
       ScheduleNextInvocation();
     } else {
       main_thread_task_runner_->PostDelayedTask(
           FROM_HERE,
           base::Bind(&ReporterRunner::TryToRun, base::Unretained(this)),
           next_trigger - now);
     }
   }
 
   // Returns true if the experiment to send reporter logs is enabled, the user
-  // opted into Safe Browsing extended reporting, and logs have been sent at
-  // least |kSwReporterLastTimeSentReport| days ago.
+  // opted into Safe Browsing extended reporting, and this queue of invocations
+  // started during the logs upload interval.
   bool ShouldSendReporterLogs(const std::string& suffix,
                               const PrefService& local_state) {
     UMAHistogramReporter uma(suffix);
     if (!SafeBrowsingExtendedReportingEnabled()) {
       uma.RecordLogsUploadEnabled(REPORTER_LOGS_UPLOADS_SBER_DISABLED);
       return false;
     }
-
-    const base::Time now = base::Time::Now();
-    const base::Time last_time_sent_logs = base::Time::FromInternalValue(
-        local_state.GetInt64(prefs::kSwReporterLastTimeSentReport));
-    const base::Time next_time_send_logs =
-        last_time_sent_logs +
-        base::TimeDelta::FromDays(kDaysBetweenReporterLogsSent);
-    // Send the logs if the last send is the future or if the interval has
-    // passed. The former is intended as a measure for failure recovery, in
-    // case the time in local state is incorrectly set to the future.
-    if (last_time_sent_logs > now || next_time_send_logs <= now) {
-      uma.RecordLogsUploadEnabled(REPORTER_LOGS_UPLOADS_ENABLED);
-      return true;
+    if (!in_logs_upload_period_) {
+      uma.RecordLogsUploadEnabled(REPORTER_LOGS_UPLOADS_RECENTLY_SENT_LOGS);
+      return false;
     }
-    uma.RecordLogsUploadEnabled(REPORTER_LOGS_UPLOADS_RECENTLY_SENT_LOGS);
-    return false;
+    uma.RecordLogsUploadEnabled(REPORTER_LOGS_UPLOADS_ENABLED);
+    return true;
   }
 
   // Appends switches to the next invocation that depend on the user current
   // state with respect to opting into extended Safe Browsing reporting and
   // metrics and crash reporting. The invocation object is changed locally right
   // before the actual process is launched because user status can change
   // between this and the next run for this ReporterRunner object. For example,
   // the ReporterDone() callback schedules the next run for a few days later,
   // and the user might have changed settings in the meantime.
   void AppendInvocationSpecificSwitches(SwReporterInvocation* next_invocation) {
@@ skipping 39 matching lines @@
   base::Version version_;
   scoped_refptr<base::TaskRunner> main_thread_task_runner_;
   scoped_refptr<base::TaskRunner> blocking_task_runner_;
 
   // This value is used to identify how long to wait before starting a new run
   // of the reporter queue. It's initialized with the default value and may be
   // changed to a different value when a prompt is pending and the reporter
   // should be run before adding the global error to the Chrome menu.
   int days_between_reporter_runs_ = kDaysBetweenSuccessfulSwReporterRuns;
 
+  bool in_logs_upload_period_ = false;

[grt (UTC plus 2), 2017/02/06 08:25:35]

please document this new member.

[Joe Mason, 2017/02/06 18:07:09]

Done.

+
   // A single leaky instance.
   static ReporterRunner* instance_;
 
   DISALLOW_COPY_AND_ASSIGN(ReporterRunner);
 };
 
 ReporterRunner* ReporterRunner::instance_ = nullptr;
 
 SwReporterInvocation::SwReporterInvocation()
     : command_line(base::CommandLine::NO_PROGRAM) {}
@@ skipping 51 matching lines @@
   return srt_cleaner_key.Open(HKEY_CURRENT_USER, cleaner_key_path.c_str(),
                               KEY_QUERY_VALUE) == ERROR_SUCCESS &&
          srt_cleaner_key.GetValueCount() > 0;
 }
 
 void SetSwReporterTestingDelegate(SwReporterTestingDelegate* delegate) {
   g_testing_delegate_ = delegate;
 }
 
 }  // namespace safe_browsing