Fix bug in reservation monitoring.

In ScopedReservation destructor did Reserve instead of Discard!
This is now fixed, and unit tests provided for this and other cases.
Also, Storage tests now verify that all memory has been released after
the storage is reset.

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

chrome/browser/policy/messaging_layer/storage/resources/resource_interface.cc

@@ -27,7 +27,7 @@ bool ScopedReservation::reserved() const {
 
 ScopedReservation::~ScopedReservation() {
   if (reserved()) {
-    resource_interface_->Reserve(size_.value());
+    resource_interface_->Discard(size_.value());
   }
 }
 

chrome/browser/policy/messaging_layer/storage/resources/resource_interface_unittest.cc

+// Copyright 2020 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 <cstdint>
+
+#include "chrome/browser/policy/messaging_layer/storage/resources/resource_interface.h"
+
+#include "base/task/post_task.h"
+#include "base/task_runner.h"
+#include "base/test/task_environment.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::Eq;
+
+namespace reporting {
+namespace {
+
+class TestCallbackWaiter {
+ public:
+  TestCallbackWaiter() = default;
+  TestCallbackWaiter(const TestCallbackWaiter& other) = delete;
+  TestCallbackWaiter& operator=(const TestCallbackWaiter& other) = delete;
+
+  void Attach() {
+    const size_t old_counter = counter_.fetch_add(1);
+    DCHECK_GT(old_counter, 0u) << "Cannot attach when already being released";
+  }
+
+  void Signal() {
+    const size_t old_counter = counter_.fetch_sub(1);
+    DCHECK_GT(old_counter, 0u) << "Already being released";
+    if (old_counter == 1u) {
+      // Dropped last owner.
+      run_loop_.Quit();
+    }
+  }
+
+  void Wait() {
+    Signal();  // Rid of the constructor's ownership.
+    run_loop_.Run();
+  }
+
+ private:
+  std::atomic<size_t> counter_{1};  // Onwed by constructor.
+  base::RunLoop run_loop_;
+};
+
+class ResourceInterfaceTest
+    : public ::testing::TestWithParam<ResourceInterface*> {
+ protected:
+  ResourceInterface* resource_interface() const { return GetParam(); }
+
+ private:
+  base::test::TaskEnvironment task_environment_;
+};
+
+TEST_P(ResourceInterfaceTest, NestedReservationTest) {
+  uint64_t size = resource_interface()->GetTotal();
+  while ((size / 2) > 0u) {
+    size /= 2;
+    EXPECT_TRUE(resource_interface()->Reserve(size));
+  }
+
+  for (; size < resource_interface()->GetTotal(); size *= 2) {
+    resource_interface()->Discard(size);
+  }
+
+  EXPECT_THAT(resource_interface()->GetUsed(), Eq(0u));
+}
+
+TEST_P(ResourceInterfaceTest, SimultaneousReservationTest) {
+  uint64_t size = resource_interface()->GetTotal();
+
+  // Schedule reservations.
+  TestCallbackWaiter reserve_waiter;
+  while ((size / 2) > 0u) {
+    size /= 2;
+    reserve_waiter.Attach();
+    base::ThreadPool::PostTask(
+        FROM_HERE, {base::TaskPriority::BEST_EFFORT},
+        base::BindOnce(
+            [](size_t size, ResourceInterface* resource_interface,
+               TestCallbackWaiter* waiter) {
+              EXPECT_TRUE(resource_interface->Reserve(size));
+              waiter->Signal();
+            },
+            size, resource_interface(), &reserve_waiter));
+  }
+  reserve_waiter.Wait();
+
+  // Schedule discards.
+  TestCallbackWaiter discard_waiter;
+  for (; size < resource_interface()->GetTotal(); size *= 2) {
+    discard_waiter.Attach();
+    base::ThreadPool::PostTask(
+        FROM_HERE, {base::TaskPriority::BEST_EFFORT},
+        base::BindOnce(
+            [](size_t size, ResourceInterface* resource_interface,
+               TestCallbackWaiter* waiter) {
+              resource_interface->Discard(size);
+              waiter->Signal();
+            },
+            size, resource_interface(), &discard_waiter));
+  }
+  discard_waiter.Wait();
+
+  EXPECT_THAT(resource_interface()->GetUsed(), Eq(0u));
+}
+
+TEST_P(ResourceInterfaceTest, SimultaneousScopedReservationTest) {
+  uint64_t size = resource_interface()->GetTotal();
+  TestCallbackWaiter waiter;
+  while ((size / 2) > 0u) {
+    size /= 2;
+    waiter.Attach();
+    base::ThreadPool::PostTask(
+        FROM_HERE, {base::TaskPriority::BEST_EFFORT},
+        base::BindOnce(
+            [](size_t size, ResourceInterface* resource_interface,
+               TestCallbackWaiter* waiter) {
+              { ScopedReservation(size, resource_interface); }
+              waiter->Signal();
+            },
+            size, resource_interface(), &waiter));
+  }
+  waiter.Wait();
+  EXPECT_THAT(resource_interface()->GetUsed(), Eq(0u));
+}
+
+TEST_P(ResourceInterfaceTest, ReservationOverMaxTest) {
+  EXPECT_FALSE(
+      resource_interface()->Reserve(resource_interface()->GetTotal() + 1));
+  EXPECT_TRUE(resource_interface()->Reserve(resource_interface()->GetTotal()));
+  resource_interface()->Discard(resource_interface()->GetTotal());
+  EXPECT_THAT(resource_interface()->GetUsed(), Eq(0u));
+}
+
+TEST_P(ResourceInterfaceTest, DiscardMoreThenReservedDeathTest) {
+  EXPECT_TRUE(
+      resource_interface()->Reserve(resource_interface()->GetTotal() / 2));
+  EXPECT_DEATH(resource_interface()->Discard(resource_interface()->GetTotal()),
+               "Check failed");
+}
+
+INSTANTIATE_TEST_SUITE_P(VariousResources,
+                         ResourceInterfaceTest,
+                         testing::Values(GetMemoryResource(),
+                                         GetDiskResource()));
+
+}  // namespace
+}  // namespace reporting

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

@@ -139,10 +139,8 @@ StorageQueue::~StorageQueue() {
 
   // Stop upload timer.
   upload_timer_.AbandonAndStop();
-  // CLose all opened files.
-  for (auto& file : files_) {
-    file.second->Close();
-  }
+  // Close all opened files.
+  files_.clear();
 }
 
 Status StorageQueue::Init() {
@@ -1318,6 +1316,7 @@ StorageQueue::SingleFile::SingleFile(const base::FilePath& filename,
     : filename_(filename), size_(size) {}
 
 StorageQueue::SingleFile::~SingleFile() {
+  Close();
   handle_.reset();
 }
 
@@ -1355,8 +1354,10 @@ void StorageQueue::SingleFile::Close() {
   }
   handle_.reset();
   is_readonly_ = base::nullopt;
-  buffer_.reset();
-  GetMemoryResource()->Discard(buffer_size_);
+  if (buffer_) {
+    buffer_.reset();
+    GetMemoryResource()->Discard(buffer_size_);
+  }
 }
 
 Status StorageQueue::SingleFile::Delete() {

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

@@ -15,9 +15,9 @@
 #include "base/optional.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/synchronization/waitable_event.h"
 #include "base/test/task_environment.h"
 #include "chrome/browser/policy/messaging_layer/encryption/test_encryption_module.h"
+#include "chrome/browser/policy/messaging_layer/storage/resources/resource_interface.h"
 #include "chrome/browser/policy/messaging_layer/storage/storage_configuration.h"
 #include "chrome/browser/policy/messaging_layer/util/status.h"
 #include "chrome/browser/policy/messaging_layer/util/statusor.h"
@@ -288,6 +288,8 @@ class StorageQueueTest : public ::testing::TestWithParam<size_t> {
   void ResetTestStorageQueue() {
     task_environment_.RunUntilIdle();
     storage_queue_.reset();
+    // Make sure all memory is deallocated.
+    ASSERT_THAT(GetMemoryResource()->GetUsed(), Eq(0u));
   }
 
   void InjectFailures(std::initializer_list<int64_t> seq_numbers) {

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

@@ -13,12 +13,12 @@
 #include "base/sequenced_task_runner.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/synchronization/waitable_event.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "chrome/browser/policy/messaging_layer/encryption/decryption.h"
 #include "chrome/browser/policy/messaging_layer/encryption/encryption.h"
 #include "chrome/browser/policy/messaging_layer/encryption/test_encryption_module.h"
+#include "chrome/browser/policy/messaging_layer/storage/resources/resource_interface.h"
 #include "chrome/browser/policy/messaging_layer/storage/storage_configuration.h"
 #include "chrome/browser/policy/messaging_layer/util/status.h"
 #include "chrome/browser/policy/messaging_layer/util/status_macros.h"
@@ -493,6 +493,8 @@ class StorageTest
   void ResetTestStorage() {
     task_environment_.RunUntilIdle();
     storage_.reset();
+    // Make sure all memory is deallocated.
+    ASSERT_THAT(GetMemoryResource()->GetUsed(), Eq(0u));
     expect_to_need_key_ = false;
   }
 

chrome/test/BUILD.gn

@@ -3660,6 +3660,7 @@ test("unit_tests") {
     "../browser/policy/messaging_layer/public/report_client_unittest.cc",
     "../browser/policy/messaging_layer/public/report_queue_configuration_unittest.cc",
     "../browser/policy/messaging_layer/public/report_queue_unittest.cc",
+    "../browser/policy/messaging_layer/storage/resources/resource_interface_unittest.cc",
     "../browser/policy/messaging_layer/storage/storage_queue_unittest.cc",
     "../browser/policy/messaging_layer/storage/storage_unittest.cc",
     "../browser/policy/messaging_layer/storage/test_storage_module.cc",