Revert to Mock time, fix clock advance

Copy from missive to Chromium.

Since it appears that test clock forwarding can trigger more timer
actions than it is implied, attempt to set up exact number of expected
uploads and impose it (ignore uploads that have not been expected).

Harden the tests by verifying that all expected uploads indeed happened
(counter dropped to 0 when the Storage/StorageQueue is shut down).

Also removed precautionary EXPECT_CALLs for extraneous uploads set
before in sensitive cases - with the new change such uploads should be
prevented by the counter.

The Test*Event should not advance the mock time in any case, so we should
control the run loop manually.

Bug: b:254418902
Bug: b:258063046
Change-Id: Idc2ef106e2d7b4774682dc0b1a881148a659a379
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4024244
Auto-Submit: Leonid Baraz <lbaraz@chromium.org>
Reviewed-by: Hong Xu <xuhong@google.com>
Commit-Queue: Hong Xu <xuhong@google.com>
Cr-Commit-Position: refs/heads/main@{#1070580}

chrome/browser/policy/messaging_layer/public/report_client.cc

@@ -20,7 +20,6 @@
 #include "base/task/sequenced_task_runner.h"
 #include "base/task/thread_pool.h"
 #include "base/threading/sequence_bound.h"
-#include "base/time/time.h"
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
 #include "chrome/browser/policy/messaging_layer/upload/upload_provider.h"
@@ -68,13 +67,11 @@ void ReportingClient::CreateLocalStorageModule(
   LOG(WARNING) << "Store reporting data locally";
   DCHECK(!StorageSelector::is_use_missive())
       << "Can only be used in local mode";
-  StorageOptions options;
-  options.set_directory(local_reporting_path);
-  options.set_signature_verification_public_key(verification_key);
   StorageModule::Create(
-      options, std::move(async_start_upload_cb),
-      EncryptionModule::Create(
-          /*renew_encryption_key_period=*/base::Days(1), options.clock()),
+      StorageOptions()
+          .set_directory(local_reporting_path)
+          .set_signature_verification_public_key(verification_key),
+      std::move(async_start_upload_cb), EncryptionModule::Create(),
       CompressionModule::Create(512, compression_algorithm),
       // Callback wrapper changes result type from `StorageModule` to
       // `StorageModuleInterface`.

components/reporting/encryption/encryption_module.cc

@@ -45,9 +45,8 @@ void AddToRecord(base::StringPiece record,
 
 }  // namespace
 
-EncryptionModule::EncryptionModule(base::TimeDelta renew_encryption_key_period,
-                                   const base::TickClock* clock)
-    : EncryptionModuleInterface(renew_encryption_key_period, clock) {
+EncryptionModule::EncryptionModule(base::TimeDelta renew_encryption_key_period)
+    : EncryptionModuleInterface(renew_encryption_key_period) {
   static_assert(std::is_same<PublicKeyId, Encryptor::PublicKeyId>::value,
                 "Public key id types must match");
   auto encryptor_result = Encryptor::Create();
@@ -88,10 +87,9 @@ void EncryptionModule::UpdateAsymmetricKeyImpl(
 
 // static
 scoped_refptr<EncryptionModuleInterface> EncryptionModule::Create(
-    base::TimeDelta renew_encryption_key_period,
-    const base::TickClock* clock) {
+    base::TimeDelta renew_encryption_key_period) {
   return base::WrapRefCounted(
-      new EncryptionModule(renew_encryption_key_period, clock));
+      new EncryptionModule(renew_encryption_key_period));
 }
 
 }  // namespace reporting

components/reporting/encryption/encryption_module.h

@@ -8,7 +8,6 @@
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/string_piece.h"
-#include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "components/reporting/encryption/encryption.h"
 #include "components/reporting/encryption/encryption_module_interface.h"
@@ -25,13 +24,11 @@ class EncryptionModule : public EncryptionModuleInterface {
 
   // Factory method creates |EncryptionModule| object.
   static scoped_refptr<EncryptionModuleInterface> Create(
-      base::TimeDelta renew_encryption_key_period,
-      const base::TickClock* clock);
+      base::TimeDelta renew_encryption_key_period = base::Days(1));
 
  protected:
   // Constructor can only be called by |Create| factory method.
-  EncryptionModule(base::TimeDelta renew_encryption_key_period,
-                   const base::TickClock* clock);
+  explicit EncryptionModule(base::TimeDelta renew_encryption_key_period);
 
   ~EncryptionModule() override;
 

components/reporting/encryption/encryption_module_interface.cc

@@ -29,10 +29,8 @@ bool EncryptionModuleInterface::is_enabled() {
 }
 
 EncryptionModuleInterface::EncryptionModuleInterface(
-    base::TimeDelta renew_encryption_key_period,
-    const base::TickClock* clock)
-    : renew_encryption_key_period_(renew_encryption_key_period),
-      clock_(clock) {}
+    base::TimeDelta renew_encryption_key_period)
+    : renew_encryption_key_period_(renew_encryption_key_period) {}
 
 EncryptionModuleInterface::~EncryptionModuleInterface() = default;
 
@@ -71,7 +69,7 @@ void EncryptionModuleInterface::UpdateAsymmetricKey(
              base::OnceCallback<void(Status)> response_cb, Status status) {
             if (status.ok()) {
               encryption_module_interface->last_encryption_key_update_.store(
-                  encryption_module_interface->clock_->NowTicks());
+                  base::TimeTicks::Now());
             }
             std::move(response_cb).Run(status);
           },
@@ -85,7 +83,7 @@ bool EncryptionModuleInterface::has_encryption_key() const {
 bool EncryptionModuleInterface::need_encryption_key() const {
   return !has_encryption_key() ||
          last_encryption_key_update_.load() + renew_encryption_key_period_ <
-             clock_->NowTicks();
+             base::TimeTicks::Now();
 }
 
 }  // namespace reporting

components/reporting/encryption/encryption_module_interface.h

@@ -9,10 +9,8 @@
 
 #include "base/callback.h"
 #include "base/feature_list.h"
-#include "base/memory/raw_ptr.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/string_piece.h"
-#include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "components/reporting/proto/synced/record.pb.h"
 #include "components/reporting/util/status.h"
@@ -32,8 +30,7 @@ class EncryptionModuleInterface
   using PublicKeyId = int32_t;
 
   explicit EncryptionModuleInterface(
-      base::TimeDelta renew_encryption_key_period,
-      const base::TickClock* clock);
+      base::TimeDelta renew_encryption_key_period = base::Days(1));
   EncryptionModuleInterface(const EncryptionModuleInterface& other) = delete;
   EncryptionModuleInterface& operator=(const EncryptionModuleInterface& other) =
       delete;
@@ -88,9 +85,6 @@ class EncryptionModuleInterface
 
   // Period of encryption key update.
   const base::TimeDelta renew_encryption_key_period_;
-
-  // Clock reference (real clock for prod, simulated clock for tests).
-  const base::raw_ptr<const base::TickClock> clock_;
 };
 
 }  // namespace reporting

components/reporting/encryption/encryption_module_unittest.cc

@@ -18,8 +18,6 @@
 #include "base/task/thread_pool.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
-#include "base/time/default_tick_clock.h"
-#include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "components/reporting/encryption/decryption.h"
 #include "components/reporting/encryption/encryption.h"
@@ -46,9 +44,7 @@ class EncryptionModuleTest : public ::testing::Test {
   EncryptionModuleTest() = default;
 
   void SetUp() override {
-    encryption_module_ =
-        EncryptionModule::Create(/*renew_encryption_key_period=*/base::Days(1),
-                                 base::DefaultTickClock::GetInstance());
+    encryption_module_ = EncryptionModule::Create();
 
     auto decryptor_result = test::Decryptor::Create();
     ASSERT_OK(decryptor_result.status()) << decryptor_result.status();

components/reporting/encryption/test_encryption_module.cc

@@ -9,8 +9,6 @@
 
 #include "base/callback.h"
 #include "base/strings/string_piece.h"
-#include "base/time/default_tick_clock.h"
-#include "base/time/tick_clock.h"
 #include "components/reporting/proto/synced/record.pb.h"
 #include "components/reporting/util/statusor.h"
 
@@ -19,9 +17,7 @@ using ::testing::Invoke;
 namespace reporting {
 namespace test {
 
-TestEncryptionModuleStrict::TestEncryptionModuleStrict()
-    : EncryptionModuleInterface(/*renew_encryption_key_period=*/base::Days(1),
-                                base::DefaultTickClock::GetInstance()) {
+TestEncryptionModuleStrict::TestEncryptionModuleStrict() {
   ON_CALL(*this, EncryptRecordImpl)
       .WillByDefault(
           Invoke([](base::StringPiece record,

components/reporting/storage/storage_configuration.cc

@@ -59,12 +59,12 @@ constexpr base::TimeDelta kFailedUploadRetryDelay = base::Seconds(1);
 
 }  // namespace
 
-StorageOptions::StorageOptions(const base::TickClock* clock)
+StorageOptions::StorageOptions()
     : memory_resource_(base::MakeRefCounted<MemoryResourceImpl>(
           4u * 1024uLL * 1024uLL)),  // 4 MiB by default
       disk_space_resource_(base::MakeRefCounted<DiskResourceImpl>(
-          64u * 1024uLL * 1024uLL)),  // 64 MiB by default.
-      clock_(clock) {}
+          64u * 1024uLL * 1024uLL))  // 64 MiB by default.
+{}
 StorageOptions::StorageOptions(const StorageOptions& options) = default;
 StorageOptions::~StorageOptions() = default;
 

components/reporting/storage/storage_configuration.h

@@ -10,11 +10,8 @@
 #include <vector>
 
 #include "base/files/file_path.h"
-#include "base/memory/raw_ptr.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/strings/string_piece.h"
-#include "base/time/default_tick_clock.h"
-#include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "components/reporting/proto/synced/record_constants.pb.h"
 #include "components/reporting/resources/disk_resource_impl.h"
@@ -37,8 +34,7 @@ class StorageOptions {
  public:
   using QueuesOptionsList = std::vector<std::pair<Priority, QueueOptions>>;
 
-  explicit StorageOptions(
-      const base::TickClock* clock = base::DefaultTickClock::GetInstance());
+  StorageOptions();
   StorageOptions(const StorageOptions& options);
   StorageOptions& operator=(const StorageOptions& options) = delete;
   virtual ~StorageOptions();
@@ -91,8 +87,6 @@ class StorageOptions {
     return memory_resource_;
   }
 
-  const base::TickClock* clock() const { return clock_.get(); }
-
  private:
   // Subdirectory of the location assigned for this Storage.
   base::FilePath directory_;
@@ -107,9 +101,6 @@ class StorageOptions {
   // Resources managements.
   scoped_refptr<ResourceInterface> memory_resource_;
   scoped_refptr<ResourceInterface> disk_space_resource_;
-
-  // Clock reference (real clock for prod, simulated clock for tests).
-  const base::raw_ptr<const base::TickClock> clock_;
 };
 
 // Single queue options class allowing to set parameters individually, e.g.:
@@ -167,7 +158,6 @@ class QueueOptions {
   scoped_refptr<ResourceInterface> memory_resource() const {
     return storage_options_.memory_resource();
   }
-  const base::TickClock* clock() const { return storage_options_.clock(); }
 
  private:
   // Whole storage options, which this queue options are based on.

components/reporting/storage/storage_queue.cc

@@ -200,8 +200,6 @@ StorageQueue::StorageQueue(
       low_priority_task_runner_(base::ThreadPool::CreateSequencedTaskRunner(
           {base::TaskPriority::BEST_EFFORT, base::MayBlock()})),
       options_(options),
-      upload_timer_(options.clock()),
-      check_back_timer_(options.clock()),
       async_start_upload_cb_(async_start_upload_cb),
       encryption_module_(encryption_module),
       compression_module_(compression_module) {
@@ -212,9 +210,8 @@ StorageQueue::StorageQueue(
 StorageQueue::~StorageQueue() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(storage_queue_sequence_checker_);
 
-  // Stop timers.
+  // Stop upload timer.
   upload_timer_.AbandonAndStop();
-  check_back_timer_.AbandonAndStop();
   // Make sure no pending writes is present.
   DCHECK(write_contexts_queue_.empty());
 
@@ -297,9 +294,8 @@ Status StorageQueue::Init() {
   //
   if (!options_.upload_period().is_zero() &&
       !options_.upload_period().is_max()) {
-    upload_timer_.Start(FROM_HERE, options_.upload_period(),
-                        base::BindRepeating(&StorageQueue::PeriodicUpload,
-                                            weakptr_factory_.GetWeakPtr()));
+    upload_timer_.Start(FROM_HERE, options_.upload_period(), this,
+                        &StorageQueue::PeriodicUpload);
   }
   // In case some events are found in the queue, initiate an upload.
   // This is especially imporant for non-periodic queues, but won't harm
@@ -1076,13 +1072,10 @@ class StorageQueue::ReadContext : public TaskRunnerContext<Status> {
     // retry the upload.
     if (storage_queue_ &&
         !storage_queue_->options_.upload_retry_delay().is_zero()) {
-      storage_queue_->check_back_timer_.Start(
-          FROM_HERE, storage_queue_->options_.upload_retry_delay(),
-          base::BindPostTask(
-              storage_queue_->sequenced_task_runner_,
-              base::BindRepeating(
-                  &StorageQueue::CheckBackUpload, storage_queue_, status,
-                  /*next_sequencing_id=*/sequence_info_.sequencing_id())));
+      ScheduleAfter(storage_queue_->options_.upload_retry_delay(),
+                    base::BindOnce(
+                        &StorageQueue::CheckBackUpload, storage_queue_, status,
+                        /*next_sequencing_id=*/sequence_info_.sequencing_id()));
     }
   }
 

components/reporting/storage/storage_queue.h

@@ -467,15 +467,9 @@ class StorageQueue : public base::RefCountedDeleteOnSequence<StorageQueue> {
   // All accesses take place on sequenced_task_runner_.
   int32_t active_read_operations_ = 0;
 
-  // Upload timer (active only if options_.upload_period() is not 0 and not
-  // infinity).
+  // Upload timer (active only if options_.upload_period() is not 0).
   base::RepeatingTimer upload_timer_;
 
-  // Check back after upload timer (activated after upload has been started
-  // and options_.upload_retry_delay() is not 0). If already started, it will
-  // be reset to the new delay.
-  base::RetainingOneShotTimer check_back_timer_;
-
   // Upload provider callback.
   const UploaderInterface::AsyncStartUploaderCb async_start_upload_cb_;