Fix incorrect StorageQueue initialization.

The bug assigned incorrect first seq number when StorageQueue consisted of multifile files.
Also, fixed incorrect setting of StorageQueue options in the test, and
added a new test that democased the failure.

Bug: b:165851833
Change-Id: I6bfcb96eada2b8a47e2a10ca711ba414e8de61d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2373426Reviewed-by: Zach Trudo <zatrudo@google.com>
Commit-Queue: Zach Trudo <zatrudo@google.com>
Auto-Submit: Leonid Baraz <lbaraz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801487}

chrome/browser/policy/messaging_layer/storage/storage_queue.cc

@@ -21,6 +21,7 @@
 #include "base/hash/hash.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "base/sequence_checker.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
@@ -157,7 +158,10 @@ Status StorageQueue::Init() {
 
 Status StorageQueue::EnumerateDataFiles() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(storage_queue_sequence_checker_);
-  first_seq_number_ = 0;
+  // We need to set first_seq_number_ to 0 if this is the initialization
+  // of an empty StorageQueue, and to the lowest seq number among all
+  // existing files, if it was already used.
+  base::Optional<uint64_t> first_seq_number;
   base::FileEnumerator dir_enum(
       options_.directory(),
       /*recursive=*/false, base::FileEnumerator::FILES,
@@ -170,7 +174,7 @@ Status StorageQueue::EnumerateDataFiles() {
                     base::StrCat({"File has no extension: '",
                                   full_name.MaybeAsASCII(), "'"}));
     }
-    uint64_t seq_number;
+    uint64_t seq_number = 0;
     bool success = base::StringToUint64(
         dir_enum.GetInfo().GetName().Extension().substr(1), &seq_number);
     if (!success) {
@@ -186,10 +190,16 @@ Status StorageQueue::EnumerateDataFiles() {
                     base::StrCat({"Sequencing duplicated: '",
                                   full_name.MaybeAsASCII(), "'"}));
     }
-    if (first_seq_number_ > seq_number) {
-      first_seq_number_ = seq_number;  // Records with this number exist.
+    if (!first_seq_number.has_value() ||
+        first_seq_number.value() > seq_number) {
+      first_seq_number = seq_number;
     }
   }
+  // first_seq_number.has_value() is true only if we found some files.
+  // Otherwise it is false, the StorageQueue is being initialized for the
+  // first time, and we need to set first_seq_number_ to 0.
+  first_seq_number_ =
+      first_seq_number.has_value() ? first_seq_number.value() : 0;
   return Status::StatusOK();
 }
 

chrome/browser/policy/messaging_layer/storage/storage_queue_unittest.cc

@@ -151,7 +151,7 @@ class StorageQueueTest : public ::testing::TestWithParam<size_t> {
         .set_directory(
             base::FilePath(location_.GetPath()).Append(FILE_PATH_LITERAL("D1")))
         .set_file_prefix(FILE_PATH_LITERAL("F0001"))
-        .set_total_size(GetParam());
+        .set_single_file_size(GetParam());
   }
 
   StorageQueue::Options BuildStorageQueueOptionsPeriodic(
@@ -419,6 +419,82 @@ TEST_P(StorageQueueTest, WriteAndRepeatedlyUploadWithConfirmations) {
   task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
 }
 
+TEST_P(StorageQueueTest, WriteAndRepeatedlyUploadWithConfirmationsAndReopen) {
+  CreateStorageQueueOrDie(BuildStorageQueueOptionsPeriodic());
+
+  WriteStringOrDie(blobs[0]);
+  WriteStringOrDie(blobs[1]);
+  WriteStringOrDie(blobs[2]);
+
+  // Set uploader expectations.
+  EXPECT_CALL(set_mock_uploader_expectations_, Call(NotNull()))
+      .WillOnce(Invoke([](MockUploadClient* mock_upload_client) {
+        MockUploadClient::SetUp(mock_upload_client)
+            .Required(blobs[0])
+            .Required(blobs[1])
+            .Required(blobs[2]);
+      }));
+
+  // Forward time to trigger upload
+  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
+
+  // Confirm #0 and forward time again, removing blob #0
+  ConfirmOrDie(/*seq_number=*/0);
+  // Set uploader expectations.
+  EXPECT_CALL(set_mock_uploader_expectations_, Call(NotNull()))
+      .WillOnce(Invoke([](MockUploadClient* mock_upload_client) {
+        MockUploadClient::SetUp(mock_upload_client)
+            .Required(blobs[1])
+            .Required(blobs[2]);
+      }));
+  // Forward time to trigger upload
+  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
+
+  // Confirm #1 and forward time again, removing blob #1
+  ConfirmOrDie(/*seq_number=*/1);
+  // Set uploader expectations.
+  EXPECT_CALL(set_mock_uploader_expectations_, Call(NotNull()))
+      .WillOnce(Invoke([](MockUploadClient* mock_upload_client) {
+        MockUploadClient::SetUp(mock_upload_client).Required(blobs[2]);
+      }));
+  // Forward time to trigger upload
+  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
+
+  storage_queue_.reset();
+  CreateStorageQueueOrDie(BuildStorageQueueOptionsPeriodic());
+
+  // Add more records and verify that #2 and new records are returned.
+  WriteStringOrDie(more_blobs[0]);
+  WriteStringOrDie(more_blobs[1]);
+  WriteStringOrDie(more_blobs[2]);
+
+  // Set uploader expectations.
+  EXPECT_CALL(set_mock_uploader_expectations_, Call(NotNull()))
+      .WillOnce(Invoke([](MockUploadClient* mock_upload_client) {
+        MockUploadClient::SetUp(mock_upload_client)
+            .Possible(blobs[0])
+            .Possible(blobs[1])
+            .Required(blobs[2])
+            .Required(more_blobs[0])
+            .Required(more_blobs[1])
+            .Required(more_blobs[2]);
+      }));
+  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
+
+  // Confirm #2 and forward time again, removing blob #2
+  ConfirmOrDie(/*seq_number=*/2);
+
+  // Set uploader expectations.
+  EXPECT_CALL(set_mock_uploader_expectations_, Call(NotNull()))
+      .WillOnce(Invoke([](MockUploadClient* mock_upload_client) {
+        MockUploadClient::SetUp(mock_upload_client)
+            .Required(more_blobs[0])
+            .Required(more_blobs[1])
+            .Required(more_blobs[2]);
+      }));
+  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
+}
+
 TEST_P(StorageQueueTest, WriteAndRepeatedlyImmediateUpload) {
   CreateStorageQueueOrDie(BuildStorageQueueOptionsImmediate());