Switch tests to SimpleTestTickClock

(Copying from missive)

Instead of MOCK_TIME, use SimpleTestTickClock explicitly in Storage,
StorageQueue and EncryptionModule. As opposed to MOCK_TIME,
SimpleTestTickClock advances *only* by explicit command, and as a result
tests behavior becomes more predictable.

In production code DefaultTickClock::GetInstance() is submitted, so the
behavior of prod code does not change.

Bug: b:254418902
Bug: b:258063046
Change-Id: Icdd0dda3afadaf13e78c60fcea8bc5a66623f4a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4014845
Reviewed-by: Vignesh Shenvi <vshenvi@google.com>
Commit-Queue: Leonid Baraz <lbaraz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1068952}

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

@@ -20,6 +20,7 @@
 #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"
@@ -67,11 +68,13 @@ 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(
-      StorageOptions()
-          .set_directory(local_reporting_path)
-          .set_signature_verification_public_key(verification_key),
-      std::move(async_start_upload_cb), EncryptionModule::Create(),
+      options, std::move(async_start_upload_cb),
+      EncryptionModule::Create(
+          /*renew_encryption_key_period=*/base::Days(1), options.clock()),
       CompressionModule::Create(512, compression_algorithm),
       // Callback wrapper changes result type from `StorageModule` to
       // `StorageModuleInterface`.

components/reporting/encryption/encryption_module.cc

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

components/reporting/encryption/encryption_module.h

@@ -8,6 +8,7 @@
 #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"
@@ -24,11 +25,13 @@ class EncryptionModule : public EncryptionModuleInterface {
 
   // Factory method creates |EncryptionModule| object.
   static scoped_refptr<EncryptionModuleInterface> Create(
-      base::TimeDelta renew_encryption_key_period = base::Days(1));
+      base::TimeDelta renew_encryption_key_period,
+      const base::TickClock* clock);
 
  protected:
   // Constructor can only be called by |Create| factory method.
-  explicit EncryptionModule(base::TimeDelta renew_encryption_key_period);
+  EncryptionModule(base::TimeDelta renew_encryption_key_period,
+                   const base::TickClock* clock);
 
   ~EncryptionModule() override;
 

components/reporting/encryption/encryption_module_interface.cc

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

components/reporting/encryption/encryption_module_interface.h

@@ -9,8 +9,10 @@
 
 #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"
@@ -30,7 +32,8 @@ class EncryptionModuleInterface
   using PublicKeyId = int32_t;
 
   explicit EncryptionModuleInterface(
-      base::TimeDelta renew_encryption_key_period = base::Days(1));
+      base::TimeDelta renew_encryption_key_period,
+      const base::TickClock* clock);
   EncryptionModuleInterface(const EncryptionModuleInterface& other) = delete;
   EncryptionModuleInterface& operator=(const EncryptionModuleInterface& other) =
       delete;
@@ -85,6 +88,9 @@ 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,6 +18,8 @@
 #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"
@@ -44,7 +46,9 @@ class EncryptionModuleTest : public ::testing::Test {
   EncryptionModuleTest() = default;
 
   void SetUp() override {
-    encryption_module_ = EncryptionModule::Create();
+    encryption_module_ =
+        EncryptionModule::Create(/*renew_encryption_key_period=*/base::Days(1),
+                                 base::DefaultTickClock::GetInstance());
 
     auto decryptor_result = test::Decryptor::Create();
     ASSERT_OK(decryptor_result.status()) << decryptor_result.status();

components/reporting/encryption/test_encryption_module.cc

@@ -9,6 +9,8 @@
 
 #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"
 
@@ -17,7 +19,9 @@ using ::testing::Invoke;
 namespace reporting {
 namespace test {
 
-TestEncryptionModuleStrict::TestEncryptionModuleStrict() {
+TestEncryptionModuleStrict::TestEncryptionModuleStrict()
+    : EncryptionModuleInterface(/*renew_encryption_key_period=*/base::Days(1),
+                                base::DefaultTickClock::GetInstance()) {
   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()
+StorageOptions::StorageOptions(const base::TickClock* clock)
     : 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.
-{}
+          64u * 1024uLL * 1024uLL)),  // 64 MiB by default.
+      clock_(clock) {}
 StorageOptions::StorageOptions(const StorageOptions& options) = default;
 StorageOptions::~StorageOptions() = default;
 

components/reporting/storage/storage_configuration.h

@@ -10,8 +10,11 @@
 #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"
@@ -34,7 +37,8 @@ class StorageOptions {
  public:
   using QueuesOptionsList = std::vector<std::pair<Priority, QueueOptions>>;
 
-  StorageOptions();
+  explicit StorageOptions(
+      const base::TickClock* clock = base::DefaultTickClock::GetInstance());
   StorageOptions(const StorageOptions& options);
   StorageOptions& operator=(const StorageOptions& options) = delete;
   virtual ~StorageOptions();
@@ -87,6 +91,8 @@ 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_;
@@ -101,6 +107,9 @@ 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.:
@@ -158,6 +167,7 @@ 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,6 +200,8 @@ 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) {
@@ -210,8 +212,9 @@ StorageQueue::StorageQueue(
 StorageQueue::~StorageQueue() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(storage_queue_sequence_checker_);
 
-  // Stop upload timer.
+  // Stop timers.
   upload_timer_.AbandonAndStop();
+  check_back_timer_.AbandonAndStop();
   // Make sure no pending writes is present.
   DCHECK(write_contexts_queue_.empty());
 
@@ -294,8 +297,9 @@ Status StorageQueue::Init() {
   //
   if (!options_.upload_period().is_zero() &&
       !options_.upload_period().is_max()) {
-    upload_timer_.Start(FROM_HERE, options_.upload_period(), this,
-                        &StorageQueue::PeriodicUpload);
+    upload_timer_.Start(FROM_HERE, options_.upload_period(),
+                        base::BindRepeating(&StorageQueue::PeriodicUpload,
+                                            weakptr_factory_.GetWeakPtr()));
   }
   // In case some events are found in the queue, initiate an upload.
   // This is especially imporant for non-periodic queues, but won't harm
@@ -1072,10 +1076,13 @@ class StorageQueue::ReadContext : public TaskRunnerContext<Status> {
     // retry the upload.
     if (storage_queue_ &&
         !storage_queue_->options_.upload_retry_delay().is_zero()) {
-      ScheduleAfter(storage_queue_->options_.upload_retry_delay(),
-                    base::BindOnce(
-                        &StorageQueue::CheckBackUpload, storage_queue_, status,
-                        /*next_sequencing_id=*/sequence_info_.sequencing_id()));
+      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())));
     }
   }
 

components/reporting/storage/storage_queue.h

@@ -467,9 +467,15 @@ 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).
+  // Upload timer (active only if options_.upload_period() is not 0 and not
+  // infinity).
   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_;