Skip to content

GitLab

T
tangled
Commit 48b27c5c authored by Findit's avatar Findit
Browse files
Options

Revert "Fix bug in reservation monitoring." 

This reverts commit c072c8c7.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 845219 as the
culprit for failures in the build cycles as shown on:
https://analysis.chromium.org/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2MwNzJjOGM3NDZlYjA5OWYzNzE5MGU0YzZjNWEzOWM0Yzk0N2I2OGUM

Sample Failed Build: https://ci.chromium.org/b/8857563414825836112

Sample Failed Step: bf_cache_unit_tests

Original change's description:
> 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}


Change-Id: I453d14fd867294c134b72a40331ffd98a7a6e12c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: b:174509632
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2639575
Cr-Commit-Position: refs/heads/master@{#845259}
parent 5ea30253
Hide whitespace changes
Inline Side-by-side
Showing with 9 additions and 168 deletions
chrome/browser/policy/messaging_layer/storage/resources/resource_interface.cc
View file @ 48b27c5c
......@@ -27,7 +27,7 @@ bool ScopedReservation::reserved() const {
ScopedReservation::~ScopedReservation() {
if (reserved()) {
resource_interface_->Discard(size_.value());
resource_interface_->Reserve(size_.value());
}
}
......
chrome/browser/policy/messaging_layer/storage/resources/resource_interface_unittest.cc deleted 100644 → 0
View file @ 5ea30253
// 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
View file @ 48b27c5c
......@@ -139,8 +139,10 @@ StorageQueue::~StorageQueue() {
// Stop upload timer.
upload_timer_.AbandonAndStop();
// Close all opened files.
files_.clear();
// CLose all opened files.
for (auto& file : files_) {
file.second->Close();
}
}
Status StorageQueue::Init() {
......@@ -1316,7 +1318,6 @@ StorageQueue::SingleFile::SingleFile(const base::FilePath& filename,
: filename_(filename), size_(size) {}
StorageQueue::SingleFile::~SingleFile() {
Close();
handle_.reset();
}
......@@ -1354,10 +1355,8 @@ void StorageQueue::SingleFile::Close() {
}
handle_.reset();
is_readonly_ = base::nullopt;
if (buffer_) {
buffer_.reset();
GetMemoryResource()->Discard(buffer_size_);
}
buffer_.reset();
GetMemoryResource()->Discard(buffer_size_);
}
Status StorageQueue::SingleFile::Delete() {
......
chrome/browser/policy/messaging_layer/storage/storage_queue_unittest.cc
View file @ 48b27c5c
......@@ -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,8 +288,6 @@ 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
View file @ 48b27c5c
......@@ -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,8 +493,6 @@ 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
View file @ 48b27c5c
......@@ -3660,7 +3660,6 @@ 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",
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment