Skip to content

Commit

Permalink
[ads] Fix intermittent crash on database call during shutdown
Browse files Browse the repository at this point in the history
  • Loading branch information
@aseren
aseren committed Sep 6, 2024
1 parent 321aba7 commit 89084b1
Show file tree
Hide file tree
Showing 46 changed files with 463 additions and 452 deletions.
38 changes: 19 additions & 19 deletions .../brave_ads/core/internal/account/confirmations/queue/confirmation_queue_database_table.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ constexpr int kDefaultBatchSize = 50;

constexpr base::TimeDelta kMaximumRetryDelay = base::Hours(1);

void BindColumnTypes(mojom::DBActionInfo* const mojom_db_action) {
void BindColumnTypes(const mojom::DBActionInfoPtr& mojom_db_action) {
CHECK(mojom_db_action);

mojom_db_action->bind_column_types = {
Expand All @@ -66,7 +66,7 @@ void BindColumnTypes(mojom::DBActionInfo* const mojom_db_action) {
};
}

size_t BindColumns(mojom::DBActionInfo* mojom_db_action,
size_t BindColumns(const mojom::DBActionInfoPtr& mojom_db_action,
const ConfirmationQueueItemList& confirmation_queue_items) {
CHECK(mojom_db_action);
CHECK(!confirmation_queue_items.empty());
Expand Down Expand Up @@ -148,7 +148,7 @@ size_t BindColumns(mojom::DBActionInfo* mojom_db_action,
}

ConfirmationQueueItemInfo FromMojomRow(
const mojom::DBRowInfo* const mojom_db_row) {
const mojom::DBRowInfoPtr& mojom_db_row) {
CHECK(mojom_db_row);

ConfirmationQueueItemInfo confirmation_queue_item;
Expand Down Expand Up @@ -215,7 +215,7 @@ ConfirmationQueueItemInfo FromMojomRow(
void GetCallback(
GetConfirmationQueueCallback callback,
mojom::DBTransactionResultInfoPtr mojom_db_transaction_result) {
if (IsError(&*mojom_db_transaction_result)) {
if (IsError(mojom_db_transaction_result)) {
BLOG(0, "Failed to get confirmation queue");

return std::move(callback).Run(/*success=*/false,
Expand All @@ -229,7 +229,7 @@ void GetCallback(
for (const auto& mojom_db_row :
mojom_db_transaction_result->rows_union->get_rows()) {
const ConfirmationQueueItemInfo confirmation_queue_item =
FromMojomRow(&*mojom_db_row);
FromMojomRow(mojom_db_row);
if (!confirmation_queue_item.IsValid()) {
BLOG(0, "Invalid confirmation queue item");

Expand All @@ -242,7 +242,7 @@ void GetCallback(
std::move(callback).Run(/*success=*/true, confirmation_queue_items);
}

void MigrateToV36(mojom::DBTransactionInfo* const mojom_db_transaction) {
void MigrateToV36(const mojom::DBTransactionInfoPtr& mojom_db_transaction) {
CHECK(mojom_db_transaction);

Execute(mojom_db_transaction, R"(
Expand All @@ -269,15 +269,15 @@ void MigrateToV36(mojom::DBTransactionInfo* const mojom_db_transaction) {
/*columns=*/{"process_at"});
}

void MigrateToV38(mojom::DBTransactionInfo* const mojom_db_transaction) {
void MigrateToV38(const mojom::DBTransactionInfoPtr& mojom_db_transaction) {
CHECK(mojom_db_transaction);

// The conversion queue is deprecated since all confirmations are now being
// added to the confirmation queue.
DropTable(mojom_db_transaction, "conversion_queue");
}

void MigrateToV43(mojom::DBTransactionInfo* const mojom_db_transaction) {
void MigrateToV43(const mojom::DBTransactionInfoPtr& mojom_db_transaction) {
CHECK(mojom_db_transaction);

// Optimize database query for `Delete`, and `Retry`.
Expand All @@ -303,7 +303,7 @@ void ConfirmationQueue::Save(
SplitVector(confirmation_queue_items, batch_size_);

for (const auto& batch : batches) {
Insert(&*mojom_db_transaction, batch);
Insert(mojom_db_transaction, batch);
}

RunDBTransaction(std::move(mojom_db_transaction), std::move(callback));
Expand All @@ -313,7 +313,7 @@ void ConfirmationQueue::DeleteAll(ResultCallback callback) const {
mojom::DBTransactionInfoPtr mojom_db_transaction =
mojom::DBTransactionInfo::New();

DeleteTable(&*mojom_db_transaction, GetTableName());
DeleteTable(mojom_db_transaction, GetTableName());

RunDBTransaction(std::move(mojom_db_transaction), std::move(callback));
}
Expand All @@ -322,7 +322,7 @@ void ConfirmationQueue::Delete(const std::string& transaction_id,
ResultCallback callback) const {
mojom::DBTransactionInfoPtr mojom_db_transaction =
mojom::DBTransactionInfo::New();
Execute(&*mojom_db_transaction, R"(
Execute(mojom_db_transaction, R"(
DELETE FROM
$1
WHERE
Expand All @@ -346,7 +346,7 @@ void ConfirmationQueue::Retry(const std::string& transaction_id,
mojom::DBTransactionInfoPtr mojom_db_transaction =
mojom::DBTransactionInfo::New();
Execute(
&*mojom_db_transaction, R"(
mojom_db_transaction, R"(
UPDATE
$1
SET
Expand Down Expand Up @@ -393,7 +393,7 @@ void ConfirmationQueue::GetAll(GetConfirmationQueueCallback callback) const {
ORDER BY
process_at ASC;)",
{GetTableName()}, nullptr);
BindColumnTypes(&*mojom_db_action);
BindColumnTypes(mojom_db_action);
mojom_db_transaction->actions.push_back(std::move(mojom_db_action));

GetAdsClient()->RunDBTransaction(
Expand Down Expand Up @@ -430,7 +430,7 @@ void ConfirmationQueue::GetNext(GetConfirmationQueueCallback callback) const {
LIMIT
1;)",
{GetTableName()}, nullptr);
BindColumnTypes(&*mojom_db_action);
BindColumnTypes(mojom_db_action);
mojom_db_transaction->actions.push_back(std::move(mojom_db_action));

GetAdsClient()->RunDBTransaction(
Expand All @@ -443,7 +443,7 @@ std::string ConfirmationQueue::GetTableName() const {
}

void ConfirmationQueue::Create(
mojom::DBTransactionInfo* const mojom_db_transaction) {
const mojom::DBTransactionInfoPtr& mojom_db_transaction) {
CHECK(mojom_db_transaction);

Execute(mojom_db_transaction, R"(
Expand Down Expand Up @@ -475,7 +475,7 @@ void ConfirmationQueue::Create(
}

void ConfirmationQueue::Migrate(
mojom::DBTransactionInfo* const mojom_db_transaction,
const mojom::DBTransactionInfoPtr& mojom_db_transaction,
const int to_version) {
CHECK(mojom_db_transaction);

Expand All @@ -500,7 +500,7 @@ void ConfirmationQueue::Migrate(
///////////////////////////////////////////////////////////////////////////////

void ConfirmationQueue::Insert(
mojom::DBTransactionInfo* mojom_db_transaction,
const mojom::DBTransactionInfoPtr& mojom_db_transaction,
const ConfirmationQueueItemList& confirmation_queue_items) const {
CHECK(mojom_db_transaction);

Expand All @@ -511,12 +511,12 @@ void ConfirmationQueue::Insert(
mojom::DBActionInfoPtr mojom_db_action = mojom::DBActionInfo::New();
mojom_db_action->type = mojom::DBActionInfo::Type::kRunStatement;
mojom_db_action->sql =
BuildInsertSql(&*mojom_db_action, confirmation_queue_items);
BuildInsertSql(mojom_db_action, confirmation_queue_items);
mojom_db_transaction->actions.push_back(std::move(mojom_db_action));
}

std::string ConfirmationQueue::BuildInsertSql(
mojom::DBActionInfo* mojom_db_action,
const mojom::DBActionInfoPtr& mojom_db_action,
const ConfirmationQueueItemList& confirmation_queue_items) const {
CHECK(mojom_db_action);
CHECK(!confirmation_queue_items.empty());
Expand Down
8 changes: 4 additions & 4 deletions ...s/brave_ads/core/internal/account/confirmations/queue/confirmation_queue_database_table.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,16 @@ class ConfirmationQueue final : public TableInterface {

std::string GetTableName() const override;

void Create(mojom::DBTransactionInfo* mojom_db_transaction) override;
void Migrate(mojom::DBTransactionInfo* mojom_db_transaction,
void Create(const mojom::DBTransactionInfoPtr& mojom_db_transaction) override;
void Migrate(const mojom::DBTransactionInfoPtr& mojom_db_transaction,
int to_version) override;

private:
void Insert(mojom::DBTransactionInfo* mojom_db_transaction,
void Insert(const mojom::DBTransactionInfoPtr& mojom_db_transaction,
const ConfirmationQueueItemList& confirmation_queue_items) const;

std::string BuildInsertSql(
mojom::DBActionInfo* mojom_db_action,
const mojom::DBActionInfoPtr& mojom_db_action,
const ConfirmationQueueItemList& confirmation_queue_items) const;

int batch_size_;
Expand Down
42 changes: 22 additions & 20 deletions components/brave_ads/core/internal/account/deposits/deposits_database_table.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ namespace {

constexpr char kTableName[] = "deposits";

void BindColumnTypes(mojom::DBActionInfo* const mojom_db_action) {
void BindColumnTypes(const mojom::DBActionInfoPtr& mojom_db_action) {
CHECK(mojom_db_action);

mojom_db_action->bind_column_types = {
Expand All @@ -37,7 +37,7 @@ void BindColumnTypes(mojom::DBActionInfo* const mojom_db_action) {
};
}

size_t BindColumns(mojom::DBActionInfo* mojom_db_action,
size_t BindColumns(const mojom::DBActionInfoPtr& mojom_db_action,
const CreativeAdList& creative_ads) {
CHECK(mojom_db_action);
CHECK(!creative_ads.empty());
Expand All @@ -58,7 +58,7 @@ size_t BindColumns(mojom::DBActionInfo* mojom_db_action,
return row_count;
}

void BindColumns(mojom::DBActionInfo* const mojom_db_action,
void BindColumns(const mojom::DBActionInfoPtr& mojom_db_action,
const DepositInfo& deposit) {
CHECK(mojom_db_action);
CHECK(deposit.IsValid());
Expand All @@ -68,7 +68,7 @@ void BindColumns(mojom::DBActionInfo* const mojom_db_action,
BindColumnTime(mojom_db_action, 2, deposit.expire_at.value_or(base::Time()));
}

DepositInfo FromMojomRow(const mojom::DBRowInfo* const mojom_db_row) {
DepositInfo FromMojomRow(const mojom::DBRowInfoPtr& mojom_db_row) {
CHECK(mojom_db_row);

DepositInfo deposit;
Expand All @@ -87,7 +87,7 @@ void GetForCreativeInstanceIdCallback(
const std::string& /*creative_instance_id*/,
GetDepositsCallback callback,
mojom::DBTransactionResultInfoPtr mojom_db_transaction_result) {
if (IsError(&*mojom_db_transaction_result)) {
if (IsError(mojom_db_transaction_result)) {
BLOG(0, "Failed to get deposit value");

return std::move(callback).Run(/*success=*/false,
Expand All @@ -102,7 +102,7 @@ void GetForCreativeInstanceIdCallback(

const mojom::DBRowInfoPtr mojom_db_row =
std::move(mojom_db_transaction_result->rows_union->get_rows().front());
DepositInfo deposit = FromMojomRow(&*mojom_db_row);
DepositInfo deposit = FromMojomRow(mojom_db_row);
if (!deposit.IsValid()) {
BLOG(0, "Invalid deposit");

Expand All @@ -112,7 +112,7 @@ void GetForCreativeInstanceIdCallback(
std::move(callback).Run(/*success=*/true, std::move(deposit));
}

void MigrateToV43(mojom::DBTransactionInfo* const mojom_db_transaction) {
void MigrateToV43(const mojom::DBTransactionInfoPtr& mojom_db_transaction) {
CHECK(mojom_db_transaction);

// Optimize database query for `GetForCreativeInstanceId`.
Expand All @@ -136,12 +136,12 @@ void Deposits::Save(const DepositInfo& deposit, ResultCallback callback) {
mojom::DBTransactionInfoPtr mojom_db_transaction =
mojom::DBTransactionInfo::New();

Insert(&*mojom_db_transaction, deposit);
Insert(mojom_db_transaction, deposit);

RunDBTransaction(std::move(mojom_db_transaction), std::move(callback));
}

void Deposits::Insert(mojom::DBTransactionInfo* mojom_db_transaction,
void Deposits::Insert(const mojom::DBTransactionInfoPtr& mojom_db_transaction,
const CreativeAdList& creative_ads) {
CHECK(mojom_db_transaction);

Expand All @@ -151,18 +151,18 @@ void Deposits::Insert(mojom::DBTransactionInfo* mojom_db_transaction,

mojom::DBActionInfoPtr mojom_db_action = mojom::DBActionInfo::New();
mojom_db_action->type = mojom::DBActionInfo::Type::kRunStatement;
mojom_db_action->sql = BuildInsertSql(&*mojom_db_action, creative_ads);
mojom_db_action->sql = BuildInsertSql(mojom_db_action, creative_ads);
mojom_db_transaction->actions.push_back(std::move(mojom_db_action));
}

void Deposits::Insert(mojom::DBTransactionInfo* mojom_db_transaction,
void Deposits::Insert(const mojom::DBTransactionInfoPtr& mojom_db_transaction,
const DepositInfo& deposit) {
CHECK(mojom_db_transaction);
CHECK(deposit.IsValid());

mojom::DBActionInfoPtr mojom_db_action = mojom::DBActionInfo::New();
mojom_db_action->type = mojom::DBActionInfo::Type::kRunStatement;
mojom_db_action->sql = BuildInsertSql(&*mojom_db_action, deposit);
mojom_db_action->sql = BuildInsertSql(mojom_db_action, deposit);
mojom_db_transaction->actions.push_back(std::move(mojom_db_action));
}

Expand All @@ -188,7 +188,7 @@ void Deposits::GetForCreativeInstanceId(const std::string& creative_instance_id,
WHERE
creative_instance_id = '$2';)",
{GetTableName(), creative_instance_id}, nullptr);
BindColumnTypes(&*mojom_db_action);
BindColumnTypes(mojom_db_action);
mojom_db_transaction->actions.push_back(std::move(mojom_db_action));

GetAdsClient()->RunDBTransaction(
Expand All @@ -200,7 +200,7 @@ void Deposits::GetForCreativeInstanceId(const std::string& creative_instance_id,
void Deposits::PurgeExpired(ResultCallback callback) const {
mojom::DBTransactionInfoPtr mojom_db_transaction =
mojom::DBTransactionInfo::New();
Execute(&*mojom_db_transaction, R"(
Execute(mojom_db_transaction, R"(
DELETE FROM
$1
WHERE
Expand All @@ -214,7 +214,7 @@ std::string Deposits::GetTableName() const {
return kTableName;
}

void Deposits::Create(mojom::DBTransactionInfo* const mojom_db_transaction) {
void Deposits::Create(const mojom::DBTransactionInfoPtr& mojom_db_transaction) {
CHECK(mojom_db_transaction);

Execute(mojom_db_transaction, R"(
Expand All @@ -233,7 +233,7 @@ void Deposits::Create(mojom::DBTransactionInfo* const mojom_db_transaction) {
/*columns=*/{"expire_at"});
}

void Deposits::Migrate(mojom::DBTransactionInfo* mojom_db_transaction,
void Deposits::Migrate(const mojom::DBTransactionInfoPtr& mojom_db_transaction,
const int to_version) {
CHECK(mojom_db_transaction);

Expand All @@ -247,8 +247,9 @@ void Deposits::Migrate(mojom::DBTransactionInfo* mojom_db_transaction,

///////////////////////////////////////////////////////////////////////////////

std::string Deposits::BuildInsertSql(mojom::DBActionInfo* mojom_db_action,
const CreativeAdList& creative_ads) const {
std::string Deposits::BuildInsertSql(
const mojom::DBActionInfoPtr& mojom_db_action,
const CreativeAdList& creative_ads) const {
CHECK(mojom_db_action);
CHECK(!creative_ads.empty());

Expand All @@ -266,8 +267,9 @@ std::string Deposits::BuildInsertSql(mojom::DBActionInfo* mojom_db_action,
nullptr);
}

std::string Deposits::BuildInsertSql(mojom::DBActionInfo* mojom_db_action,
const DepositInfo& deposit) const {
std::string Deposits::BuildInsertSql(
const mojom::DBActionInfoPtr& mojom_db_action,
const DepositInfo& deposit) const {
CHECK(mojom_db_action);
CHECK(deposit.IsValid());

Expand Down
12 changes: 6 additions & 6 deletions components/brave_ads/core/internal/account/deposits/deposits_database_table.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ class Deposits final : public TableInterface {
public:
void Save(const DepositInfo& deposit, ResultCallback callback);

void Insert(mojom::DBTransactionInfo* mojom_db_transaction,
void Insert(const mojom::DBTransactionInfoPtr& mojom_db_transaction,
const CreativeAdList& creative_ads);
void Insert(mojom::DBTransactionInfo* mojom_db_transaction,
void Insert(const mojom::DBTransactionInfoPtr& mojom_db_transaction,
const DepositInfo& deposit);

void GetForCreativeInstanceId(const std::string& creative_instance_id,
Expand All @@ -38,14 +38,14 @@ class Deposits final : public TableInterface {

std::string GetTableName() const override;

void Create(mojom::DBTransactionInfo* mojom_db_transaction) override;
void Migrate(mojom::DBTransactionInfo* mojom_db_transaction,
void Create(const mojom::DBTransactionInfoPtr& mojom_db_transaction) override;
void Migrate(const mojom::DBTransactionInfoPtr& mojom_db_transaction,
int to_version) override;

private:
std::string BuildInsertSql(mojom::DBActionInfo* mojom_db_action,
std::string BuildInsertSql(const mojom::DBActionInfoPtr& mojom_db_action,
const CreativeAdList& creative_ads) const;
std::string BuildInsertSql(mojom::DBActionInfo* mojom_db_action,
std::string BuildInsertSql(const mojom::DBActionInfoPtr& mojom_db_action,
const DepositInfo& deposit) const;
};

Expand Down
Loading

0 comments on commit 89084b1

Please sign in to comment.