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.

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

Removes std::function<> from the utility process (Part5). #19792

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "base/json/json_writer.h"
#include "base/strings/string_number_conversions.h"
#include "base/uuid.h"
#include "brave/components/brave_rewards/core/common/legacy_callback_helpers.h"
#include "brave/components/brave_rewards/core/credentials/credentials_common.h"
#include "brave/components/brave_rewards/core/credentials/credentials_util.h"
#include "brave/components/brave_rewards/core/database/database.h"
Expand Down Expand Up @@ -115,8 +116,7 @@ void CredentialsCommon::OnSaveUnblindedCreds(ResultCallback callback,

engine_->database()->UpdateCredsBatchStatus(
trigger.id, trigger.type, mojom::CredsBatchStatus::FINISHED,
[callback = std::make_shared<decltype(callback)>(std::move(callback))](
mojom::Result result) { std::move(*callback).Run(result); });
ToLegacyCallback(std::move(callback)));
}

} // namespace credential
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include "base/json/json_writer.h"
#include "base/strings/string_number_conversions.h"
#include "brave/components/brave_rewards/core/common/legacy_callback_helpers.h"
#include "brave/components/brave_rewards/core/credentials/credentials_promotion.h"
#include "brave/components/brave_rewards/core/credentials/credentials_util.h"
#include "brave/components/brave_rewards/core/database/database.h"
Expand Down Expand Up @@ -416,9 +417,7 @@ void CredentialsPromotion::Completed(ResultCallback callback,
}

engine_->database()->PromotionCredentialCompleted(
trigger.id,
[callback = std::make_shared<decltype(callback)>(std::move(callback))](
mojom::Result result) { std::move(*callback).Run(result); });
trigger.id, ToLegacyCallback(std::move(callback)));
engine_->client()->UnblindedTokensReady();
}

Expand Down
13 changes: 7 additions & 6 deletions components/brave_rewards/core/database/database.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ Database::Database(RewardsEngineImpl& engine)

Database::~Database() = default;

void Database::Initialize(LegacyResultCallback callback) {
initialize_.Start(callback);
void Database::Initialize(ResultCallback callback) {
initialize_.Start(std::move(callback));
}

void Database::Close(LegacyResultCallback callback) {
Expand Down Expand Up @@ -106,10 +106,11 @@ void Database::SaveBalanceReportInfoItem(mojom::ActivityMonth month,
balance_report_.SetAmount(month, year, type, amount, callback);
}

void Database::GetBalanceReportInfo(mojom::ActivityMonth month,
int year,
GetBalanceReportCallback callback) {
balance_report_.GetRecord(month, year, callback);
void Database::GetBalanceReportInfo(
mojom::ActivityMonth month,
int year,
mojom::RewardsEngine::GetBalanceReportCallback callback) {
balance_report_.GetRecord(month, year, std::move(callback));
}

void Database::GetAllBalanceReports(GetBalanceReportListCallback callback) {
Expand Down
9 changes: 5 additions & 4 deletions components/brave_rewards/core/database/database.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class Database {
explicit Database(RewardsEngineImpl& engine);
virtual ~Database();

void Initialize(LegacyResultCallback callback);
void Initialize(ResultCallback callback);

void Close(LegacyResultCallback callback);

Expand Down Expand Up @@ -83,9 +83,10 @@ class Database {
double amount,
LegacyResultCallback callback);

void GetBalanceReportInfo(mojom::ActivityMonth month,
int year,
GetBalanceReportCallback callback);
void GetBalanceReportInfo(
mojom::ActivityMonth month,
int year,
mojom::RewardsEngine::GetBalanceReportCallback callback);

void GetAllBalanceReports(GetBalanceReportListCallback callback);

Expand Down
31 changes: 15 additions & 16 deletions components/brave_rewards/core/database/database_balance_report.cc
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,13 @@ void DatabaseBalanceReport::SetAmount(mojom::ActivityMonth month,
engine_->RunDBTransaction(std::move(transaction), transaction_callback);
}

void DatabaseBalanceReport::GetRecord(mojom::ActivityMonth month,
int year,
GetBalanceReportCallback callback) {
void DatabaseBalanceReport::GetRecord(
mojom::ActivityMonth month,
int year,
mojom::RewardsEngine::GetBalanceReportCallback callback) {
if (month == mojom::ActivityMonth::ANY || year == 0) {
BLOG(1, "Record size is not correct " << month << "/" << year);
callback(mojom::Result::FAILED, {});
return;
return std::move(callback).Run(mojom::Result::FAILED, {});
}

auto transaction = mojom::DBTransaction::New();
Expand Down Expand Up @@ -215,25 +215,24 @@ void DatabaseBalanceReport::GetRecord(mojom::ActivityMonth month,

transaction->commands.push_back(std::move(command));

auto transaction_callback =
std::bind(&DatabaseBalanceReport::OnGetRecord, this, _1, callback);

engine_->RunDBTransaction(std::move(transaction), transaction_callback);
engine_->RunDBTransaction(
std::move(transaction),
base::BindOnce(&DatabaseBalanceReport::OnGetRecord,
base::Unretained(this), std::move(callback)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reported by reviewdog 🐶
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:

- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated


Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/c/chromium-uaf.yaml


Cc @thypon @goodov @iefremov

}
void DatabaseBalanceReport::OnGetRecord(mojom::DBCommandResponsePtr response,
GetBalanceReportCallback callback) {
void DatabaseBalanceReport::OnGetRecord(
mojom::RewardsEngine::GetBalanceReportCallback callback,
mojom::DBCommandResponsePtr response) {
if (!response ||
response->status != mojom::DBCommandResponse::Status::RESPONSE_OK) {
BLOG(0, "Response is wrong");
callback(mojom::Result::FAILED, {});
return;
return std::move(callback).Run(mojom::Result::FAILED, {});
}

if (response->result->get_records().size() != 1) {
BLOG(1, "Record size is not correct: "
<< response->result->get_records().size());
callback(mojom::Result::FAILED, {});
return;
return std::move(callback).Run(mojom::Result::FAILED, {});
}

auto* record = response->result->get_records()[0].get();
Expand All @@ -246,7 +245,7 @@ void DatabaseBalanceReport::OnGetRecord(mojom::DBCommandResponsePtr response,
info->recurring_donation = GetDoubleColumn(record, 4);
info->one_time_donation = GetDoubleColumn(record, 5);

callback(mojom::Result::OK, std::move(info));
std::move(callback).Run(mojom::Result::OK, std::move(info));
}

void DatabaseBalanceReport::GetAllRecords(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,15 @@ class DatabaseBalanceReport : public DatabaseTable {

void GetRecord(mojom::ActivityMonth month,
int year,
GetBalanceReportCallback callback);
mojom::RewardsEngine::GetBalanceReportCallback callback);

void GetAllRecords(GetBalanceReportListCallback callback);

void DeleteAllRecords(LegacyResultCallback callback);

private:
void OnGetRecord(mojom::DBCommandResponsePtr response,
GetBalanceReportCallback callback);
void OnGetRecord(mojom::RewardsEngine::GetBalanceReportCallback callback,
mojom::DBCommandResponsePtr response);

void OnGetAllRecords(mojom::DBCommandResponsePtr response,
GetBalanceReportListCallback callback);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <string>
#include <utility>

#include "base/test/mock_callback.h"
#include "base/test/task_environment.h"
#include "brave/components/brave_rewards/core/database/database_balance_report.h"
#include "brave/components/brave_rewards/core/database/database_util.h"
Expand Down Expand Up @@ -102,10 +103,9 @@ TEST_F(DatabaseBalanceReportTest, GetRecordOk) {
std::move(callback).Run(db_error_response->Clone());
});

MockFunction<GetBalanceReportCallback> callback;
EXPECT_CALL(callback, Call).Times(1);
balance_report_.GetRecord(mojom::ActivityMonth::MAY, 2020,
callback.AsStdFunction());
base::MockCallback<mojom::RewardsEngine::GetBalanceReportCallback> callback;
EXPECT_CALL(callback, Run).Times(1);
balance_report_.GetRecord(mojom::ActivityMonth::MAY, 2020, callback.Get());

task_environment_.RunUntilIdle();
}
Expand Down
20 changes: 8 additions & 12 deletions components/brave_rewards/core/database/database_initialize.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@
#include "brave/components/brave_rewards/core/rewards_engine_impl.h"
#include "brave/components/brave_rewards/core/state/state_keys.h"

using std::placeholders::_1;
using std::placeholders::_2;

namespace brave_rewards::internal {
namespace database {

Expand All @@ -21,7 +18,7 @@ DatabaseInitialize::DatabaseInitialize(RewardsEngineImpl& engine)

DatabaseInitialize::~DatabaseInitialize() = default;

void DatabaseInitialize::Start(LegacyResultCallback callback) {
void DatabaseInitialize::Start(ResultCallback callback) {
auto transaction = mojom::DBTransaction::New();
transaction->version = GetCurrentVersion();
transaction->compatible_version = GetCompatibleVersion();
Expand All @@ -31,27 +28,26 @@ void DatabaseInitialize::Start(LegacyResultCallback callback) {

engine_->RunDBTransaction(
std::move(transaction),
std::bind(&DatabaseInitialize::OnInitialize, this, _1, callback));
base::BindOnce(&DatabaseInitialize::OnInitialize, base::Unretained(this),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reported by reviewdog 🐶
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:

- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated


Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/c/chromium-uaf.yaml


Cc @thypon @goodov @iefremov

std::move(callback)));
}

void DatabaseInitialize::OnInitialize(mojom::DBCommandResponsePtr response,
LegacyResultCallback callback) {
void DatabaseInitialize::OnInitialize(ResultCallback callback,
mojom::DBCommandResponsePtr response) {
if (!response ||
response->status != mojom::DBCommandResponse::Status::RESPONSE_OK) {
BLOG(0, "Response is wrong");
callback(mojom::Result::DATABASE_INIT_FAILED);
return;
return std::move(callback).Run(mojom::Result::DATABASE_INIT_FAILED);
}

if (!response->result || !response->result->get_value()->is_int_value()) {
BLOG(0, "DB init failed");
callback(mojom::Result::DATABASE_INIT_FAILED);
return;
return std::move(callback).Run(mojom::Result::DATABASE_INIT_FAILED);
}

const auto current_table_version =
response->result->get_value()->get_int_value();
migration_.Start(current_table_version, callback);
migration_.Start(current_table_version, std::move(callback));
}

} // namespace database
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ class DatabaseInitialize {
explicit DatabaseInitialize(RewardsEngineImpl& engine);
~DatabaseInitialize();

void Start(LegacyResultCallback callback);
void Start(ResultCallback callback);

private:
void OnInitialize(mojom::DBCommandResponsePtr response,
LegacyResultCallback callback);
void OnInitialize(ResultCallback callback,
mojom::DBCommandResponsePtr response);

const raw_ref<RewardsEngineImpl> engine_;
DatabaseMigration migration_;
Expand Down
47 changes: 26 additions & 21 deletions components/brave_rewards/core/database/database_migration.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,7 @@ DatabaseMigration::DatabaseMigration(RewardsEngineImpl& engine)

DatabaseMigration::~DatabaseMigration() = default;

void DatabaseMigration::Start(uint32_t table_version,
LegacyResultCallback callback) {
void DatabaseMigration::Start(uint32_t table_version, ResultCallback callback) {
const uint32_t start_version = table_version + 1;
DCHECK_GT(start_version, 0u);

Expand All @@ -83,8 +82,7 @@ void DatabaseMigration::Start(uint32_t table_version,
: database::GetCurrentVersion();

if (target_version == table_version) {
callback(mojom::Result::OK);
return;
return std::move(callback).Run(mojom::Result::OK);
}

// Migration 30 archives and clears the user's unblinded tokens table. It
Expand Down Expand Up @@ -164,24 +162,11 @@ void DatabaseMigration::Start(uint32_t table_version,
command->type = mojom::DBCommand::Type::VACUUM;
transaction->commands.push_back(std::move(command));

const std::string message =
base::StringPrintf("%d->%d", start_version, migrated_version);

engine_->RunDBTransaction(
std::move(transaction), [this, callback, message, migrated_version](
mojom::DBCommandResponsePtr response) {
if (response &&
response->status == mojom::DBCommandResponse::Status::RESPONSE_OK) {
// The event_log table was introduced in v29.
if (migrated_version >= 29) {
engine_->database()->SaveEventLog(log::kDatabaseMigrated, message);
}
callback(mojom::Result::OK);
return;
}

callback(mojom::Result::FAILED);
});
std::move(transaction),
base::BindOnce(&DatabaseMigration::RunDBTransactionCallback,
base::Unretained(this), std::move(callback), start_version,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reported by reviewdog 🐶
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:

- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated


Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/c/chromium-uaf.yaml


Cc @thypon @goodov @iefremov

migrated_version));
}

void DatabaseMigration::SetTargetVersionForTesting(uint32_t version) {
Expand All @@ -202,5 +187,25 @@ void DatabaseMigration::GenerateCommand(mojom::DBTransaction* transaction,
transaction->commands.push_back(std::move(command));
}

void DatabaseMigration::RunDBTransactionCallback(
ResultCallback callback,
uint32_t start_version,
int migrated_version,
mojom::DBCommandResponsePtr response) {
if (response &&
response->status == mojom::DBCommandResponse::Status::RESPONSE_OK) {
// The event_log table was introduced in v29.
if (migrated_version >= 29) {
engine_->database()->SaveEventLog(
log::kDatabaseMigrated,
base::StringPrintf("%d->%d", start_version, migrated_version));
}

return std::move(callback).Run(mojom::Result::OK);
}

std::move(callback).Run(mojom::Result::FAILED);
}

} // namespace database
} // namespace brave_rewards::internal
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,19 @@ class DatabaseMigration {
explicit DatabaseMigration(RewardsEngineImpl& engine);
~DatabaseMigration();

void Start(uint32_t table_version, LegacyResultCallback callback);
void Start(uint32_t table_version, ResultCallback callback);

static void SetTargetVersionForTesting(uint32_t version);

private:
void GenerateCommand(mojom::DBTransaction* transaction,
const std::string& query);

void RunDBTransactionCallback(ResultCallback callback,
uint32_t start_version,
int migrated_version,
mojom::DBCommandResponsePtr response);

const raw_ref<RewardsEngineImpl> engine_;
static uint32_t test_target_version_;
};
Expand Down
7 changes: 3 additions & 4 deletions components/brave_rewards/core/promotion/promotion.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "base/json/json_writer.h"
#include "base/strings/string_util.h"
#include "base/strings/stringprintf.h"
#include "brave/components/brave_rewards/core/common/legacy_callback_helpers.h"
#include "brave/components/brave_rewards/core/common/time_util.h"
#include "brave/components/brave_rewards/core/constants.h"
#include "brave/components/brave_rewards/core/credentials/credentials_util.h"
Expand Down Expand Up @@ -462,8 +463,7 @@ void Promotion::CredentialsProcessed(ResultCallback callback,
if (result == mojom::Result::NOT_FOUND) {
engine_->database()->UpdatePromotionStatus(
promotion_id, mojom::PromotionStatus::OVER,
[callback = std::make_shared<decltype(callback)>(std::move(callback))](
mojom::Result result) { std::move(*callback).Run(result); });
ToLegacyCallback(std::move(callback)));
return;
}

Expand All @@ -475,8 +475,7 @@ void Promotion::CredentialsProcessed(ResultCallback callback,

engine_->database()->UpdatePromotionStatus(
promotion_id, mojom::PromotionStatus::FINISHED,
[callback = std::make_shared<decltype(callback)>(std::move(callback))](
mojom::Result result) { std::move(*callback).Run(result); });
ToLegacyCallback(std::move(callback)));
}

void Promotion::Retry(
Expand Down
Loading