Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ads] Fix intermittent crash on database call during shutdown #25438

Merged
merged 1 commit into from
Sep 6, 2024
Merged

[ads] Fix intermittent crash on database call during shutdown #25438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 19 additions & 19 deletions .../brave_ads/core/internal/account/confirmations/queue/confirmation_queue_database_table.cc
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading