Commit e789b31d authored by Curt Clemens's avatar Curt Clemens Committed by Commit Bot

[Nearby Share] Certificate Storage: queue calls made before init

NearbyShareCertificateStorageImpl requires that Initialize be called
before calling any functions that rely on LevelDB. This change queues up
any calls made before initialization succeeds so that the caller doesn't
have to wait for initialization to complete.

Change-Id: I9f6de24826c9b5d449142366ceb86828f8e0ef4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2332332
Commit-Queue: Curt Clemens <cclem@google.com>
Reviewed-by: default avatarJosh Nohle <nohle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794663}
parent fb42d854
......@@ -71,14 +71,6 @@ FakeNearbyShareCertificateStorage::FakeNearbyShareCertificateStorage() =
FakeNearbyShareCertificateStorage::~FakeNearbyShareCertificateStorage() =
default;
bool FakeNearbyShareCertificateStorage::IsInitialized() {
return is_initialized_;
}
void FakeNearbyShareCertificateStorage::Initialize(ResultCallback callback) {
initialize_callbacks_.push_back(std::move(callback));
}
std::vector<std::string>
FakeNearbyShareCertificateStorage::GetPublicCertificateIds() const {
return public_certificate_ids_;
......@@ -142,10 +134,6 @@ void FakeNearbyShareCertificateStorage::ClearPublicCertificates(
clear_public_certificates_callbacks_.push_back(std::move(callback));
}
void FakeNearbyShareCertificateStorage::SetIsInitialized(bool is_initialized) {
is_initialized_ = is_initialized;
}
void FakeNearbyShareCertificateStorage::SetPublicCertificateIds(
const std::vector<std::string>& ids) {
public_certificate_ids_ = ids;
......
......@@ -95,7 +95,6 @@ class FakeNearbyShareCertificateStorage : public NearbyShareCertificateStorage {
FakeNearbyShareCertificateStorage();
~FakeNearbyShareCertificateStorage() override;
void SetIsInitialized(bool is_initialized);
void SetPublicCertificateIds(const std::vector<std::string>& ids);
void SetPrivateCertificates(
base::Optional<std::vector<NearbySharePrivateCertificate>>
......@@ -103,10 +102,6 @@ class FakeNearbyShareCertificateStorage : public NearbyShareCertificateStorage {
void SetNextPrivateCertificateExpirationTime(base::Optional<base::Time> time);
void SetNextPublicCertificateExpirationTime(base::Optional<base::Time> time);
std::vector<ResultCallback>& initialize_callbacks() {
return initialize_callbacks_;
}
std::vector<PublicCertificateCallback>& get_public_certificates_callbacks() {
return get_public_certificates_callbacks_;
}
......@@ -140,8 +135,6 @@ class FakeNearbyShareCertificateStorage : public NearbyShareCertificateStorage {
private:
// NearbyShareCertificateStorage:
bool IsInitialized() override;
void Initialize(ResultCallback callback) override;
std::vector<std::string> GetPublicCertificateIds() const override;
void GetPublicCertificates(PublicCertificateCallback callback) override;
base::Optional<std::vector<NearbySharePrivateCertificate>>
......@@ -166,14 +159,12 @@ class FakeNearbyShareCertificateStorage : public NearbyShareCertificateStorage {
void ClearPrivateCertificates() override;
void ClearPublicCertificates(ResultCallback callback) override;
bool is_initialized_ = false;
size_t num_clear_private_certificates_calls_ = 0;
base::Optional<base::Time> next_private_certificate_expiration_time_;
base::Optional<base::Time> next_public_certificate_expiration_time_;
std::vector<std::string> public_certificate_ids_;
base::Optional<std::vector<NearbySharePrivateCertificate>>
private_certificates_;
std::vector<ResultCallback> initialize_callbacks_;
std::vector<PublicCertificateCallback> get_public_certificates_callbacks_;
std::vector<std::vector<NearbySharePrivateCertificate>>
replace_private_certificates_calls_;
......
......@@ -25,13 +25,6 @@ class NearbyShareCertificateStorage {
NearbyShareCertificateStorage() = default;
virtual ~NearbyShareCertificateStorage() = default;
// Initialize the LevelDB and Prefs databases. Must be called successfully
// before calling other methods.
virtual void Initialize(ResultCallback callback) = 0;
// Returns whether Initialize has been called and succeeded.
virtual bool IsInitialized() = 0;
// Returns the secret ids of all stored public certificates
virtual std::vector<std::string> GetPublicCertificateIds() const = 0;
......
......@@ -11,6 +11,10 @@
#include "base/base64url.h"
#include "base/memory/ptr_util.h"
#include "base/optional.h"
#include "base/sequenced_task_runner.h"
#include "base/task/post_task.h"
#include "base/task/thread_pool.h"
#include "base/threading/sequenced_task_runner_handle.h"
#include "base/util/values/values_util.h"
#include "base/values.h"
#include "chrome/browser/nearby_sharing/certificates/nearby_share_private_certificate.h"
......@@ -104,6 +108,7 @@ NearbyShareCertificateStorageImpl::NearbyShareCertificateStorageImpl(
proto_database)
: pref_service_(pref_service), db_(std::move(proto_database)) {
FetchPublicCertificateExpirations();
Initialize();
}
NearbyShareCertificateStorageImpl::~NearbyShareCertificateStorageImpl() =
......@@ -117,56 +122,78 @@ void NearbyShareCertificateStorageImpl::RegisterPrefs(
registry->RegisterListPref(kNearbySharePrivateCertificateListPref);
}
bool NearbyShareCertificateStorageImpl::IsInitialized() {
return is_initialized_;
}
void NearbyShareCertificateStorageImpl::Initialize(ResultCallback callback) {
DCHECK(!is_initialized_);
num_initialize_attempts_++;
if (num_initialize_attempts_ > kMaxNumInitializeAttempts) {
std::move(callback).Run(false);
return;
void NearbyShareCertificateStorageImpl::Initialize() {
switch (init_status_) {
case InitStatus::kUninitialized:
case InitStatus::kFailed:
num_initialize_attempts_++;
if (num_initialize_attempts_ > kMaxNumInitializeAttempts) {
FinishInitialization(false);
break;
}
db_->Init(base::BindOnce(
&NearbyShareCertificateStorageImpl::OnDatabaseInitialized,
base::Unretained(this)));
break;
case InitStatus::kInitialized:
NOTREACHED();
break;
}
db_->Init(
base::BindOnce(&NearbyShareCertificateStorageImpl::OnDatabaseInitialized,
base::Unretained(this), std::move(callback)));
}
void NearbyShareCertificateStorageImpl::DestroyAndReinitialize(
ResultCallback callback) {
is_initialized_ = false;
db_->Destroy(
base::BindOnce(&NearbyShareCertificateStorageImpl::OnDatabaseDestroyed,
base::Unretained(this), /*should_reinitialize=*/true,
std::move(callback)));
void NearbyShareCertificateStorageImpl::DestroyAndReinitialize() {
init_status_ = InitStatus::kUninitialized;
db_->Destroy(base::BindOnce(
&NearbyShareCertificateStorageImpl::OnDatabaseDestroyedReinitialize,
base::Unretained(this)));
}
void NearbyShareCertificateStorageImpl::OnDatabaseInitialized(
ResultCallback callback,
leveldb_proto::Enums::InitStatus status) {
switch (status) {
case leveldb_proto::Enums::InitStatus::kOK:
is_initialized_ = true;
std::move(callback).Run(true);
FinishInitialization(true);
break;
case leveldb_proto::Enums::InitStatus::kError:
Initialize(std::move(callback));
Initialize();
break;
case leveldb_proto::Enums::InitStatus::kCorrupt:
DestroyAndReinitialize(std::move(callback));
DestroyAndReinitialize();
break;
case leveldb_proto::Enums::InitStatus::kInvalidOperation:
case leveldb_proto::Enums::InitStatus::kNotInitialized:
std::move(callback).Run(false);
FinishInitialization(false);
break;
}
}
void NearbyShareCertificateStorageImpl::FinishInitialization(bool success) {
init_status_ = success ? InitStatus::kInitialized : InitStatus::kFailed;
// We run deferred callbacks even if initialization failed not to cause
// possible client-side blocks of next calls to the database.
while (!deferred_callbacks_.empty()) {
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE, std::move(deferred_callbacks_.front()));
deferred_callbacks_.pop();
}
}
void NearbyShareCertificateStorageImpl::OnDatabaseDestroyedReinitialize(
bool success) {
if (!success) {
FinishInitialization(false);
return;
}
public_certificate_expirations_.clear();
SavePublicCertificateExpirations();
Initialize();
}
void NearbyShareCertificateStorageImpl::OnDatabaseDestroyed(
bool should_reinitialize,
ResultCallback callback,
bool success) {
if (!success) {
......@@ -177,10 +204,7 @@ void NearbyShareCertificateStorageImpl::OnDatabaseDestroyed(
public_certificate_expirations_.clear();
SavePublicCertificateExpirations();
if (should_reinitialize)
Initialize(std::move(callback));
else
std::move(callback).Run(true);
std::move(callback).Run(true);
}
void NearbyShareCertificateStorageImpl::
......@@ -269,6 +293,19 @@ NearbyShareCertificateStorageImpl::GetPublicCertificateIds() const {
void NearbyShareCertificateStorageImpl::GetPublicCertificates(
PublicCertificateCallback callback) {
if (init_status_ == InitStatus::kFailed) {
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback), false, nullptr));
return;
}
if (init_status_ == InitStatus::kUninitialized) {
deferred_callbacks_.push(base::BindOnce(
&NearbyShareCertificateStorageImpl::GetPublicCertificates,
base::Unretained(this), std::move(callback)));
return;
}
db_->LoadEntries(std::move(callback));
}
......@@ -329,7 +366,19 @@ void NearbyShareCertificateStorageImpl::ReplacePublicCertificates(
const std::vector<nearbyshare::proto::PublicCertificate>&
public_certificates,
ResultCallback callback) {
DCHECK(is_initialized_);
if (init_status_ == InitStatus::kFailed) {
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback), false));
return;
}
if (init_status_ == InitStatus::kUninitialized) {
deferred_callbacks_.push(base::BindOnce(
&NearbyShareCertificateStorageImpl::ReplacePublicCertificates,
base::Unretained(this), public_certificates, std::move(callback)));
return;
}
auto new_entries = std::make_unique<std::vector<
std::pair<std::string, nearbyshare::proto::PublicCertificate>>>();
auto new_expirations = std::make_unique<ExpirationList>();
......@@ -351,7 +400,19 @@ void NearbyShareCertificateStorageImpl::AddPublicCertificates(
const std::vector<nearbyshare::proto::PublicCertificate>&
public_certificates,
ResultCallback callback) {
DCHECK(is_initialized_);
if (init_status_ == InitStatus::kFailed) {
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback), false));
return;
}
if (init_status_ == InitStatus::kUninitialized) {
deferred_callbacks_.push(base::BindOnce(
&NearbyShareCertificateStorageImpl::AddPublicCertificates,
base::Unretained(this), public_certificates, std::move(callback)));
return;
}
auto new_entries = std::make_unique<std::vector<
std::pair<std::string, nearbyshare::proto::PublicCertificate>>>();
auto new_expirations = std::make_unique<ExpirationList>();
......@@ -374,7 +435,19 @@ void NearbyShareCertificateStorageImpl::AddPublicCertificates(
void NearbyShareCertificateStorageImpl::RemoveExpiredPublicCertificates(
base::Time now,
ResultCallback callback) {
DCHECK(is_initialized_);
if (init_status_ == InitStatus::kFailed) {
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback), false));
return;
}
if (init_status_ == InitStatus::kUninitialized) {
deferred_callbacks_.push(base::BindOnce(
&NearbyShareCertificateStorageImpl::RemoveExpiredPublicCertificates,
base::Unretained(this), now, std::move(callback)));
return;
}
auto ids_to_remove = std::make_unique<std::vector<std::string>>();
for (const auto& pair : public_certificate_expirations_) {
if (pair.second > now)
......@@ -407,11 +480,22 @@ void NearbyShareCertificateStorageImpl::ClearPrivateCertificates() {
void NearbyShareCertificateStorageImpl::ClearPublicCertificates(
ResultCallback callback) {
DCHECK(is_initialized_);
if (init_status_ == InitStatus::kFailed) {
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback), false));
return;
}
if (init_status_ == InitStatus::kUninitialized) {
deferred_callbacks_.push(base::BindOnce(
&NearbyShareCertificateStorageImpl::ClearPublicCertificates,
base::Unretained(this), std::move(callback)));
return;
}
db_->Destroy(
base::BindOnce(&NearbyShareCertificateStorageImpl::OnDatabaseDestroyed,
base::Unretained(this), /*should_reinitialize=*/false,
std::move(callback)));
base::Unretained(this), std::move(callback)));
}
bool NearbyShareCertificateStorageImpl::FetchPublicCertificateExpirations() {
......
......@@ -6,6 +6,7 @@
#define CHROME_BROWSER_NEARBY_SHARING_CERTIFICATES_NEARBY_SHARE_CERTIFICATE_STORAGE_IMPL_H_
#include "base/containers/flat_set.h"
#include "base/containers/queue.h"
#include "chrome/browser/nearby_sharing/certificates/nearby_share_certificate_storage.h"
#include "components/leveldb_proto/public/proto_database.h"
......@@ -57,8 +58,6 @@ class NearbyShareCertificateStorageImpl : public NearbyShareCertificateStorage {
void operator=(NearbyShareCertificateStorageImpl&) = delete;
// NearbyShareCertificateStorage
bool IsInitialized() override;
void Initialize(ResultCallback callback) override;
std::vector<std::string> GetPublicCertificateIds() const override;
void GetPublicCertificates(PublicCertificateCallback callback) override;
base::Optional<std::vector<NearbySharePrivateCertificate>>
......@@ -90,14 +89,16 @@ class NearbyShareCertificateStorageImpl : public NearbyShareCertificateStorage {
leveldb_proto::ProtoDatabase<nearbyshare::proto::PublicCertificate>>
proto_database);
void OnDatabaseInitialized(ResultCallback callback,
leveldb_proto::Enums::InitStatus status);
enum class InitStatus { kUninitialized, kInitialized, kFailed };
void OnDatabaseDestroyed(bool should_reinitialize,
ResultCallback callback,
bool success);
void Initialize();
void OnDatabaseInitialized(leveldb_proto::Enums::InitStatus status);
void FinishInitialization(bool success);
void DestroyAndReinitialize(ResultCallback callback);
void OnDatabaseDestroyedReinitialize(bool success);
void OnDatabaseDestroyed(ResultCallback callback, bool success);
void DestroyAndReinitialize();
void ReplacePublicCertificatesDestroyCallback(
std::unique_ptr<std::vector<
......@@ -122,7 +123,7 @@ class NearbyShareCertificateStorageImpl : public NearbyShareCertificateStorage {
bool FetchPublicCertificateExpirations();
void SavePublicCertificateExpirations();
bool is_initialized_ = false;
InitStatus init_status_ = InitStatus::kUninitialized;
size_t num_initialize_attempts_ = 0;
......@@ -134,6 +135,7 @@ class NearbyShareCertificateStorageImpl : public NearbyShareCertificateStorage {
std::vector<NearbySharePrivateCertificate> private_certificates_;
ExpirationList public_certificate_expirations_;
base::queue<base::OnceClosure> deferred_callbacks_;
};
#endif // CHROME_BROWSER_NEARBY_SHARING_CERTIFICATES_NEARBY_SHARE_CERTIFICATE_STORAGE_IMPL_H_
......@@ -143,19 +143,8 @@ class NearbyShareCertificateStorageImplTest : public ::testing::Test {
cert_store_ = NearbyShareCertificateStorageImpl::Factory::Create(
pref_service_.get(), std::move(db));
}
bool Initialize(leveldb_proto::Enums::InitStatus init_status) {
// Make a fresh copy of cert_store_ to get back to uninitialized state.
if (cert_store_->IsInitialized())
SetUp();
bool init_success = false;
cert_store_->Initialize(base::BindOnce(
&NearbyShareCertificateStorageImplTest::CaptureBoolCallback,
base::Unretained(this), &init_success));
db_->InitStatusCallback(init_status);
return init_success;
db_->InitStatusCallback(leveldb_proto::Enums::InitStatus::kOK);
}
void PrepopulatePublicCertificates() {
......@@ -208,34 +197,7 @@ class NearbyShareCertificateStorageImplTest : public ::testing::Test {
std::vector<nearbyshare::proto::PublicCertificate> public_certificates_;
};
TEST_F(NearbyShareCertificateStorageImplTest, InitializeSucceeded) {
if (cert_store_->IsInitialized())
SetUp();
ASSERT_FALSE(cert_store_->IsInitialized());
bool succeeded = Initialize(leveldb_proto::Enums::InitStatus::kOK);
ASSERT_TRUE(cert_store_->IsInitialized());
ASSERT_TRUE(succeeded);
}
TEST_F(NearbyShareCertificateStorageImplTest, InitializeFailed) {
if (cert_store_->IsInitialized())
SetUp();
ASSERT_FALSE(cert_store_->IsInitialized());
bool succeeded = Initialize(leveldb_proto::Enums::InitStatus::kError);
ASSERT_FALSE(cert_store_->IsInitialized());
ASSERT_FALSE(succeeded);
}
TEST_F(NearbyShareCertificateStorageImplTest, GetPublicCertificateIds) {
Initialize(leveldb_proto::Enums::InitStatus::kOK);
ASSERT_TRUE(cert_store_->IsInitialized());
auto ids = cert_store_->GetPublicCertificateIds();
ASSERT_EQ(3u, ids.size());
EXPECT_EQ(ids[0], kSecretId1);
......@@ -244,9 +206,6 @@ TEST_F(NearbyShareCertificateStorageImplTest, GetPublicCertificateIds) {
}
TEST_F(NearbyShareCertificateStorageImplTest, GetPublicCertificates) {
Initialize(leveldb_proto::Enums::InitStatus::kOK);
ASSERT_TRUE(cert_store_->IsInitialized());
std::vector<nearbyshare::proto::PublicCertificate> public_certificates;
cert_store_->GetPublicCertificates(base::BindOnce(
&NearbyShareCertificateStorageImplTest::PublicCertificateCallback,
......@@ -264,9 +223,6 @@ TEST_F(NearbyShareCertificateStorageImplTest, GetPublicCertificates) {
}
TEST_F(NearbyShareCertificateStorageImplTest, ReplacePublicCertificates) {
Initialize(leveldb_proto::Enums::InitStatus::kOK);
ASSERT_TRUE(cert_store_->IsInitialized());
std::vector<nearbyshare::proto::PublicCertificate> new_certs = {
CreatePublicCertificate(kSecretId4, kSecretKey4, kPublicKey4,
kStartSeconds4, kStartNanos4, kEndSeconds4,
......@@ -301,9 +257,6 @@ TEST_F(NearbyShareCertificateStorageImplTest, ReplacePublicCertificates) {
}
TEST_F(NearbyShareCertificateStorageImplTest, AddPublicCertificates) {
Initialize(leveldb_proto::Enums::InitStatus::kOK);
ASSERT_TRUE(cert_store_->IsInitialized());
std::vector<nearbyshare::proto::PublicCertificate> new_certs = {
CreatePublicCertificate(kSecretId3, kSecretKey2, kPublicKey2,
kStartSeconds2, kStartNanos2, kEndSeconds2,
......@@ -354,9 +307,6 @@ TEST_F(NearbyShareCertificateStorageImplTest, AddPublicCertificates) {
}
TEST_F(NearbyShareCertificateStorageImplTest, ClearPublicCertificates) {
Initialize(leveldb_proto::Enums::InitStatus::kOK);
ASSERT_TRUE(cert_store_->IsInitialized());
bool succeeded = false;
cert_store_->ClearPublicCertificates(base::BindOnce(
&NearbyShareCertificateStorageImplTest::CaptureBoolCallback,
......@@ -368,9 +318,6 @@ TEST_F(NearbyShareCertificateStorageImplTest, ClearPublicCertificates) {
}
TEST_F(NearbyShareCertificateStorageImplTest, RemoveExpiredPublicCertificates) {
Initialize(leveldb_proto::Enums::InitStatus::kOK);
ASSERT_TRUE(cert_store_->IsInitialized());
std::vector<base::Time> expiration_times;
for (const auto& pair : db_entries_) {
expiration_times.emplace_back(TimestampToTime(pair.second.end_time()));
......@@ -393,9 +340,6 @@ TEST_F(NearbyShareCertificateStorageImplTest, RemoveExpiredPublicCertificates) {
}
TEST_F(NearbyShareCertificateStorageImplTest, ReplaceGetPrivateCertificates) {
Initialize(leveldb_proto::Enums::InitStatus::kOK);
ASSERT_TRUE(cert_store_->IsInitialized());
auto certs_before = CreatePrivateCertificates(3);
cert_store_->ReplacePrivateCertificates(certs_before);
auto certs_after = cert_store_->GetPrivateCertificates();
......@@ -419,9 +363,6 @@ TEST_F(NearbyShareCertificateStorageImplTest, ReplaceGetPrivateCertificates) {
TEST_F(NearbyShareCertificateStorageImplTest,
NextPrivateCertificateExpirationTime) {
Initialize(leveldb_proto::Enums::InitStatus::kOK);
ASSERT_TRUE(cert_store_->IsInitialized());
auto certs = CreatePrivateCertificates(3);
cert_store_->ReplacePrivateCertificates(certs);
base::Optional<base::Time> next_expiration =
......@@ -439,9 +380,6 @@ TEST_F(NearbyShareCertificateStorageImplTest,
TEST_F(NearbyShareCertificateStorageImplTest,
NextPublicCertificateExpirationTime) {
Initialize(leveldb_proto::Enums::InitStatus::kOK);
ASSERT_TRUE(cert_store_->IsInitialized());
base::Optional<base::Time> next_expiration =
cert_store_->NextPublicCertificateExpirationTime();
......@@ -457,9 +395,6 @@ TEST_F(NearbyShareCertificateStorageImplTest,
}
TEST_F(NearbyShareCertificateStorageImplTest, ClearPrivateCertificates) {
Initialize(leveldb_proto::Enums::InitStatus::kOK);
ASSERT_TRUE(cert_store_->IsInitialized());
std::vector<NearbySharePrivateCertificate> certs_before =
CreatePrivateCertificates(3);
cert_store_->ReplacePrivateCertificates(certs_before);
......
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