From 4b89860411ab0bbaf6d92cf601887a65a80abc94 Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Wed, 15 Jan 2025 13:05:26 -0500 Subject: [PATCH 01/10] [ads] General code health --- ..._search_result_ad_mojom_web_page_entities_test_util.cc | 2 +- .../queue/confirmation_queue_database_table.cc | 2 -- .../conversions/creative_set_conversion_database_table.cc | 2 -- .../core/internal/creatives/creatives_builder.cc | 8 ++++---- .../core/internal/deprecated/client/client_info.cc | 1 - .../confirmations/confirmation_state_manager.cc | 1 - .../core/internal/history/ad_history_database_table.cc | 2 -- .../user_engagement/ad_events/ad_events_database_table.cc | 2 -- 8 files changed, 5 insertions(+), 15 deletions(-) diff --git a/components/brave_ads/content/browser/creatives/search_result_ad/creative_search_result_ad_mojom_web_page_entities_test_util.cc b/components/brave_ads/content/browser/creatives/search_result_ad/creative_search_result_ad_mojom_web_page_entities_test_util.cc index d53aabd4e185..c6ff653400fd 100644 --- a/components/brave_ads/content/browser/creatives/search_result_ad/creative_search_result_ad_mojom_web_page_entities_test_util.cc +++ b/components/brave_ads/content/browser/creatives/search_result_ad/creative_search_result_ad_mojom_web_page_entities_test_util.cc @@ -110,7 +110,7 @@ class CreativeAdMojomWebPageEntitiesConstructor final { const std::string& name, T value) { if (!base::Contains(excluded_property_names_, name)) { - AddMojomProperty(mojom_properties, name, value); + AddMojomProperty(mojom_properties, name, std::move(value)); } } diff --git a/components/brave_ads/core/internal/account/confirmations/queue/confirmation_queue_database_table.cc b/components/brave_ads/core/internal/account/confirmations/queue/confirmation_queue_database_table.cc index c1c10f856a89..3983974e6fd6 100644 --- a/components/brave_ads/core/internal/account/confirmations/queue/confirmation_queue_database_table.cc +++ b/components/brave_ads/core/internal/account/confirmations/queue/confirmation_queue_database_table.cc @@ -76,7 +76,6 @@ size_t BindColumns(const mojom::DBActionInfoPtr& mojom_db_action, for (const auto& confirmation_queue_item : confirmation_queue_items) { if (!confirmation_queue_item.IsValid()) { BLOG(0, "Invalid confirmation queue item"); - continue; } @@ -230,7 +229,6 @@ void GetCallback( FromMojomRow(mojom_db_row); if (!confirmation_queue_item.IsValid()) { BLOG(0, "Invalid confirmation queue item"); - continue; } diff --git a/components/brave_ads/core/internal/creatives/conversions/creative_set_conversion_database_table.cc b/components/brave_ads/core/internal/creatives/conversions/creative_set_conversion_database_table.cc index 9c3b5f9b18bb..3e2671c50de7 100644 --- a/components/brave_ads/core/internal/creatives/conversions/creative_set_conversion_database_table.cc +++ b/components/brave_ads/core/internal/creatives/conversions/creative_set_conversion_database_table.cc @@ -70,7 +70,6 @@ size_t BindColumns(const mojom::DBActionInfoPtr& mojom_db_action, base::debug::DumpWithoutCrashing(); BLOG(0, "Invalid creative set conversion"); - continue; } @@ -151,7 +150,6 @@ void GetCallback( base::debug::DumpWithoutCrashing(); BLOG(0, "Invalid creative set conversion"); - continue; } diff --git a/components/brave_ads/core/internal/creatives/creatives_builder.cc b/components/brave_ads/core/internal/creatives/creatives_builder.cc index b2546c2953de..126869ec4611 100644 --- a/components/brave_ads/core/internal/creatives/creatives_builder.cc +++ b/components/brave_ads/core/internal/creatives/creatives_builder.cc @@ -119,7 +119,7 @@ CreativesInfo BuildCreatives(const CatalogInfo& catalog) { creatives.notification_ads.push_back(creative_ad); ++entries; - const std::string top_level_segment_name = + const std::string& top_level_segment_name = segment_name_hierarchy.front(); CHECK(!top_level_segment_name.empty()); @@ -187,7 +187,7 @@ CreativesInfo BuildCreatives(const CatalogInfo& catalog) { creatives.inline_content_ads.push_back(creative_ad); ++entries; - const std::string top_level_segment_name = + const std::string& top_level_segment_name = segment_name_hierarchy.front(); CHECK(!top_level_segment_name.empty()); @@ -266,7 +266,7 @@ CreativesInfo BuildCreatives(const CatalogInfo& catalog) { creatives.new_tab_page_ads.push_back(creative_ad); ++entries; - const std::string top_level_segment_name = + const std::string& top_level_segment_name = segment_name_hierarchy.front(); CHECK(!top_level_segment_name.empty()); @@ -331,7 +331,7 @@ CreativesInfo BuildCreatives(const CatalogInfo& catalog) { creatives.promoted_content_ads.push_back(creative_ad); ++entries; - const std::string top_level_segment_name = + const std::string& top_level_segment_name = segment_name_hierarchy.front(); CHECK(!top_level_segment_name.empty()); diff --git a/components/brave_ads/core/internal/deprecated/client/client_info.cc b/components/brave_ads/core/internal/deprecated/client/client_info.cc index e925786f3e54..29335002f110 100644 --- a/components/brave_ads/core/internal/deprecated/client/client_info.cc +++ b/components/brave_ads/core/internal/deprecated/client/client_info.cc @@ -165,7 +165,6 @@ bool ClientInfo::FromJson(const std::string& json) { base::debug::DumpWithoutCrashing(); BLOG(0, "Malformed client JSON state"); - return false; } diff --git a/components/brave_ads/core/internal/deprecated/confirmations/confirmation_state_manager.cc b/components/brave_ads/core/internal/deprecated/confirmations/confirmation_state_manager.cc index 5287c6ed7b64..ba9afb4dac60 100644 --- a/components/brave_ads/core/internal/deprecated/confirmations/confirmation_state_manager.cc +++ b/components/brave_ads/core/internal/deprecated/confirmations/confirmation_state_manager.cc @@ -120,7 +120,6 @@ bool ConfirmationStateManager::FromJson(const std::string& json) { base::debug::DumpWithoutCrashing(); BLOG(0, "Malformed confirmation JSON state"); - return false; } diff --git a/components/brave_ads/core/internal/history/ad_history_database_table.cc b/components/brave_ads/core/internal/history/ad_history_database_table.cc index 1ac6c65d337f..9aa3474ff341 100644 --- a/components/brave_ads/core/internal/history/ad_history_database_table.cc +++ b/components/brave_ads/core/internal/history/ad_history_database_table.cc @@ -79,7 +79,6 @@ size_t BindColumns(const mojom::DBActionInfoPtr& mojom_db_action, base::debug::DumpWithoutCrashing(); BLOG(0, "Invalid ad history item"); - continue; } @@ -160,7 +159,6 @@ void GetCallback( base::debug::DumpWithoutCrashing(); BLOG(0, "Invalid ad history item"); - continue; } diff --git a/components/brave_ads/core/internal/user_engagement/ad_events/ad_events_database_table.cc b/components/brave_ads/core/internal/user_engagement/ad_events/ad_events_database_table.cc index 1b4d0979c3bb..da0bb9428f55 100644 --- a/components/brave_ads/core/internal/user_engagement/ad_events/ad_events_database_table.cc +++ b/components/brave_ads/core/internal/user_engagement/ad_events/ad_events_database_table.cc @@ -57,7 +57,6 @@ size_t BindColumns(const mojom::DBActionInfoPtr& mojom_db_action, for (const auto& ad_event : ad_events) { if (!ad_event.IsValid()) { BLOG(0, "Invalid ad event"); - continue; } @@ -117,7 +116,6 @@ void GetCallback( const AdEventInfo ad_event = FromMojomRow(mojom_db_row); if (!ad_event.IsValid()) { BLOG(0, "Invalid ad event"); - continue; } From 6606bc75221c55c44e9b2a2f6d882ea591e5962c Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Wed, 15 Jan 2025 13:08:05 -0500 Subject: [PATCH 02/10] [ads] DumpWithoutCrashing code health Malformed JSON state will be resolved by https://github.com/brave/brave-browser/issues/39795 --- .../transactions/transactions_database_table.cc | 3 --- components/brave_ads/core/internal/ads_impl.cc | 13 ------------- .../core/internal/deprecated/client/client_info.cc | 7 ------- .../confirmations/confirmation_state_manager.cc | 7 ------- .../client/legacy_client_migration.cc | 3 --- 5 files changed, 33 deletions(-) diff --git a/components/brave_ads/core/internal/account/transactions/transactions_database_table.cc b/components/brave_ads/core/internal/account/transactions/transactions_database_table.cc index 85e4bd6db89d..a9bbf6741b40 100644 --- a/components/brave_ads/core/internal/account/transactions/transactions_database_table.cc +++ b/components/brave_ads/core/internal/account/transactions/transactions_database_table.cc @@ -10,7 +10,6 @@ #include #include "base/check.h" -#include "base/debug/dump_without_crashing.h" #include "base/functional/bind.h" #include "base/location.h" #include "base/strings/string_util.h" @@ -56,7 +55,6 @@ size_t BindColumns(const mojom::DBActionInfoPtr& mojom_db_action, int index = 0; for (const auto& transaction : transactions) { if (!transaction.IsValid()) { - base::debug::DumpWithoutCrashing(); BLOG(0, "Invalid transaction"); continue; } @@ -120,7 +118,6 @@ void GetCallback( mojom_db_transaction_result->rows_union->get_rows()) { const TransactionInfo transaction = FromMojomRow(mojom_db_row); if (!transaction.IsValid()) { - base::debug::DumpWithoutCrashing(); BLOG(0, "Invalid transaction"); continue; } diff --git a/components/brave_ads/core/internal/ads_impl.cc b/components/brave_ads/core/internal/ads_impl.cc index 9016916d68ae..d742d78f782f 100644 --- a/components/brave_ads/core/internal/ads_impl.cc +++ b/components/brave_ads/core/internal/ads_impl.cc @@ -9,7 +9,6 @@ #include #include -#include "base/debug/dump_without_crashing.h" #include "base/functional/bind.h" #include "base/trace_event/trace_event.h" #include "brave/components/brave_ads/core/internal/account/wallet/wallet_util.h" @@ -454,12 +453,6 @@ void AdsImpl::LoadClientStateCallback(mojom::WalletInfoPtr mojom_wallet, InitializeCallback callback, bool success) { if (!success) { - // TODO(https://github.com/brave/brave-browser/issues/32066): Detect - // potential defects using `DumpWithoutCrashing`. - SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason", - "Failed to load client state"); - base::debug::DumpWithoutCrashing(); - return FailedToInitialize(std::move(callback)); } @@ -495,12 +488,6 @@ void AdsImpl::LoadConfirmationStateCallback(mojom::WalletInfoPtr mojom_wallet, InitializeCallback callback, bool success) { if (!success) { - // TODO(https://github.com/brave/brave-browser/issues/32066): Detect - // potential defects using `DumpWithoutCrashing`. - SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason", - "Failed to load confirmation state"); - base::debug::DumpWithoutCrashing(); - BLOG(0, "Failed to load confirmation state"); return FailedToInitialize(std::move(callback)); } diff --git a/components/brave_ads/core/internal/deprecated/client/client_info.cc b/components/brave_ads/core/internal/deprecated/client/client_info.cc index 29335002f110..f603cc724d40 100644 --- a/components/brave_ads/core/internal/deprecated/client/client_info.cc +++ b/components/brave_ads/core/internal/deprecated/client/client_info.cc @@ -11,7 +11,6 @@ #include "base/check.h" #include "base/debug/crash_logging.h" -#include "base/debug/dump_without_crashing.h" #include "base/json/json_reader.h" #include "base/json/json_writer.h" #include "base/strings/string_number_conversions.h" @@ -158,12 +157,6 @@ bool ClientInfo::FromJson(const std::string& json) { json, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); if (!dict) { - // TODO(https://github.com/brave/brave-browser/issues/32066): Detect - // potential defects using `DumpWithoutCrashing`. - SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason", - "Malformed client JSON state"); - base::debug::DumpWithoutCrashing(); - BLOG(0, "Malformed client JSON state"); return false; } diff --git a/components/brave_ads/core/internal/deprecated/confirmations/confirmation_state_manager.cc b/components/brave_ads/core/internal/deprecated/confirmations/confirmation_state_manager.cc index ba9afb4dac60..77a876fc8e57 100644 --- a/components/brave_ads/core/internal/deprecated/confirmations/confirmation_state_manager.cc +++ b/components/brave_ads/core/internal/deprecated/confirmations/confirmation_state_manager.cc @@ -8,7 +8,6 @@ #include #include "base/check.h" -#include "base/debug/dump_without_crashing.h" #include "base/functional/bind.h" #include "base/json/json_reader.h" #include "base/json/json_writer.h" @@ -113,12 +112,6 @@ bool ConfirmationStateManager::FromJson(const std::string& json) { payment_tokens_.RemoveAllTokens(); if (!dict) { - // TODO(https://github.com/brave/brave-browser/issues/32066): Detect - // potential defects using `DumpWithoutCrashing`. - SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason", - "Malformed confirmation JSON state"); - base::debug::DumpWithoutCrashing(); - BLOG(0, "Malformed confirmation JSON state"); return false; } diff --git a/components/brave_ads/core/internal/legacy_migration/client/legacy_client_migration.cc b/components/brave_ads/core/internal/legacy_migration/client/legacy_client_migration.cc index 612aea454490..9e89b0f3a2e4 100644 --- a/components/brave_ads/core/internal/legacy_migration/client/legacy_client_migration.cc +++ b/components/brave_ads/core/internal/legacy_migration/client/legacy_client_migration.cc @@ -9,7 +9,6 @@ #include #include -#include "base/debug/dump_without_crashing.h" #include "base/functional/bind.h" #include "brave/components/brave_ads/core/internal/ads_client/ads_client_util.h" #include "brave/components/brave_ads/core/internal/common/logging_util.h" @@ -28,8 +27,6 @@ namespace brave_ads { namespace { void FailedToMigrate(const std::string& reason, InitializeCallback callback) { - SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason", reason); - base::debug::DumpWithoutCrashing(); BLOG(0, reason); std::move(callback).Run(/*success=*/false); } From f027e0a1032c02899a39e032be2fc9c064fdbce3 Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Wed, 15 Jan 2025 13:29:37 -0500 Subject: [PATCH 03/10] [ads] Invalid creative set conversion DumpWithoutCrashing code health --- .../creative_set_conversion_database_table.cc | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/components/brave_ads/core/internal/creatives/conversions/creative_set_conversion_database_table.cc b/components/brave_ads/core/internal/creatives/conversions/creative_set_conversion_database_table.cc index 3e2671c50de7..60362b262c6c 100644 --- a/components/brave_ads/core/internal/creatives/conversions/creative_set_conversion_database_table.cc +++ b/components/brave_ads/core/internal/creatives/conversions/creative_set_conversion_database_table.cc @@ -52,20 +52,20 @@ size_t BindColumns(const mojom::DBActionInfoPtr& mojom_db_action, int index = 0; for (const auto& creative_set_conversion : creative_set_conversions) { if (!creative_set_conversion.IsValid()) { - // TODO(https://github.com/brave/brave-browser/issues/32066): Detect - // potential defects using `DumpWithoutCrashing`. - SCOPED_CRASH_KEY_STRING64("Issue32066", "creative_set_conversion_id", + // TODO(https://github.com/brave/brave-browser/issues/43314): Invalid + // creative set conversion. + SCOPED_CRASH_KEY_STRING64("Issue43314", "creative_set_conversion_id", creative_set_conversion.id); - SCOPED_CRASH_KEY_STRING256("Issue32066", "url_pattern", + SCOPED_CRASH_KEY_STRING256("Issue43314", "url_pattern", creative_set_conversion.url_pattern); SCOPED_CRASH_KEY_NUMBER( - "Issue32066", "observation_window", + "Issue43314", "observation_window", creative_set_conversion.observation_window.InDays()); SCOPED_CRASH_KEY_NUMBER( - "Issue32066", "expire_at", + "Issue43314", "expire_at", creative_set_conversion.expire_at->ToDeltaSinceWindowsEpoch() .InMicroseconds()); - SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason", + SCOPED_CRASH_KEY_STRING64("Issue43314", "failure_reason", "Invalid creative set conversion"); base::debug::DumpWithoutCrashing(); @@ -132,20 +132,20 @@ void GetCallback( const CreativeSetConversionInfo creative_set_conversion = FromMojomRow(mojom_db_row); if (!creative_set_conversion.IsValid()) { - // TODO(https://github.com/brave/brave-browser/issues/32066): Detect - // potential defects using `DumpWithoutCrashing`. - SCOPED_CRASH_KEY_STRING64("Issue32066", "creative_set_conversion_id", + // TODO(https://github.com/brave/brave-browser/issues/43314): Invalid + // creative set conversion. + SCOPED_CRASH_KEY_STRING64("Issue43314", "creative_set_conversion_id", creative_set_conversion.id); - SCOPED_CRASH_KEY_STRING256("Issue32066", "url_pattern", + SCOPED_CRASH_KEY_STRING256("Issue43314", "url_pattern", creative_set_conversion.url_pattern); SCOPED_CRASH_KEY_NUMBER( - "Issue32066", "observation_window", + "Issue43314", "observation_window", creative_set_conversion.observation_window.InDays()); SCOPED_CRASH_KEY_NUMBER( - "Issue32066", "expire_at", + "Issue43314", "expire_at", creative_set_conversion.expire_at->ToDeltaSinceWindowsEpoch() .InMicroseconds()); - SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason", + SCOPED_CRASH_KEY_STRING64("Issue43314", "failure_reason", "Invalid creative set conversion"); base::debug::DumpWithoutCrashing(); From a7b88427400e207cd7839f79248c74076037201f Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Wed, 15 Jan 2025 13:29:56 -0500 Subject: [PATCH 04/10] [ads] Failed to create database DumpWithoutCrashing code health --- .../core/internal/database/database_manager.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/components/brave_ads/core/internal/database/database_manager.cc b/components/brave_ads/core/internal/database/database_manager.cc index b9416829c680..c8420ec998c1 100644 --- a/components/brave_ads/core/internal/database/database_manager.cc +++ b/components/brave_ads/core/internal/database/database_manager.cc @@ -125,11 +125,11 @@ void DatabaseManager::Create(ResultCallback callback) const { void DatabaseManager::CreateCallback(ResultCallback callback, bool success) const { if (!success) { - // TODO(https://github.com/brave/brave-browser/issues/32066): Detect - // potential defects using `DumpWithoutCrashing`. - SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason", + // TODO(https://github.com/brave/brave-browser/issues/43317): Failed to + // create database. + SCOPED_CRASH_KEY_STRING64("Issue43317", "failure_reason", "Failed to create database"); - SCOPED_CRASH_KEY_NUMBER("Issue32066", "sqlite_schema_version", + SCOPED_CRASH_KEY_NUMBER("Issue43317", "sqlite_schema_version", database::kVersionNumber); base::debug::DumpWithoutCrashing(); @@ -216,13 +216,13 @@ void DatabaseManager::MigrateFromVersionCallback(int from_version, const int to_version = database::kVersionNumber; if (!success) { - // TODO(https://github.com/brave/brave-browser/issues/32066): Detect - // potential defects using `DumpWithoutCrashing`. - SCOPED_CRASH_KEY_NUMBER("Issue32066", "from_sqlite_schema_version", + // TODO(https://github.com/brave/brave-browser/issues/43326): Database + // migration failed. + SCOPED_CRASH_KEY_NUMBER("Issue43326", "from_sqlite_schema_version", from_version); - SCOPED_CRASH_KEY_NUMBER("Issue32066", "to_sqlite_schema_version", + SCOPED_CRASH_KEY_NUMBER("Issue43326", "to_sqlite_schema_version", to_version); - SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason", + SCOPED_CRASH_KEY_STRING64("Issue43326", "failure_reason", "Database migration failed"); base::debug::DumpWithoutCrashing(); From 9e59bc9631a836f1ab804a203f335a1dbadb7432 Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Wed, 15 Jan 2025 13:30:19 -0500 Subject: [PATCH 05/10] [ads] Failed to reconcile transactions DumpWithoutCrashing code health --- .../core/internal/account/user_rewards/user_rewards.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/components/brave_ads/core/internal/account/user_rewards/user_rewards.cc b/components/brave_ads/core/internal/account/user_rewards/user_rewards.cc index 52d33efc3137..c673617912df 100644 --- a/components/brave_ads/core/internal/account/user_rewards/user_rewards.cc +++ b/components/brave_ads/core/internal/account/user_rewards/user_rewards.cc @@ -69,9 +69,9 @@ void UserRewards::OnDidRedeemPaymentTokens( transactions_database_table_.Reconcile( payment_tokens, base::BindOnce([](bool success) { if (!success) { - // TODO(https://github.com/brave/brave-browser/issues/32066): - // Detect potential defects using `DumpWithoutCrashing`. - SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason", + // TODO(https://github.com/brave/brave-browser/issues/43316): + // Failed to reconcile transactions. + SCOPED_CRASH_KEY_STRING64("Issue43316", "failure_reason", "Failed to reconcile transactions"); base::debug::DumpWithoutCrashing(); From 11bb6c53db929c3d8364cad6a806ccb50ffc41f7 Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Wed, 15 Jan 2025 13:32:06 -0500 Subject: [PATCH 06/10] [ads] Invalid ad history item DumpWithoutCrashing code health --- .../history/ad_history_database_table.cc | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/components/brave_ads/core/internal/history/ad_history_database_table.cc b/components/brave_ads/core/internal/history/ad_history_database_table.cc index 9aa3474ff341..7bd71edfafb5 100644 --- a/components/brave_ads/core/internal/history/ad_history_database_table.cc +++ b/components/brave_ads/core/internal/history/ad_history_database_table.cc @@ -62,19 +62,19 @@ size_t BindColumns(const mojom::DBActionInfoPtr& mojom_db_action, int index = 0; for (const auto& ad_history_item : ad_history) { if (!ad_history_item.IsValid()) { - // TODO(https://github.com/brave/brave-browser/issues/32066): Detect - // potential defects using `DumpWithoutCrashing`. - SCOPED_CRASH_KEY_STRING64("Issue32066", "ad_type", + // TODO(https://github.com/brave/brave-browser/issues/43328): Invalid ad + // history item. + SCOPED_CRASH_KEY_STRING64("Issue43328", "ad_type", ToString(ad_history_item.type)); - SCOPED_CRASH_KEY_STRING64("Issue32066", "confirmation_type", + SCOPED_CRASH_KEY_STRING64("Issue43328", "confirmation_type", ToString(ad_history_item.confirmation_type)); - SCOPED_CRASH_KEY_STRING64("Issue32066", "creative_instance_id", + SCOPED_CRASH_KEY_STRING64("Issue43328", "creative_instance_id", ad_history_item.creative_instance_id); - SCOPED_CRASH_KEY_STRING64("Issue32066", "advertiser_id", + SCOPED_CRASH_KEY_STRING64("Issue43328", "advertiser_id", ad_history_item.advertiser_id); - SCOPED_CRASH_KEY_STRING64("Issue32066", "segment", + SCOPED_CRASH_KEY_STRING64("Issue43328", "segment", ad_history_item.segment); - SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason", + SCOPED_CRASH_KEY_STRING64("Issue43328", "failure_reason", "Invalid ad history item"); base::debug::DumpWithoutCrashing(); @@ -142,19 +142,19 @@ void GetCallback( mojom_db_transaction_result->rows_union->get_rows()) { const AdHistoryItemInfo ad_history_item = FromMojomRow(mojom_db_row); if (!ad_history_item.IsValid()) { - // TODO(https://github.com/brave/brave-browser/issues/32066): Detect - // potential defects using `DumpWithoutCrashing`. - SCOPED_CRASH_KEY_STRING64("Issue32066", "ad_type", + // TODO(https://github.com/brave/brave-browser/issues/43328): Invalid ad + // history item. + SCOPED_CRASH_KEY_STRING64("Issue43328", "ad_type", ToString(ad_history_item.type)); - SCOPED_CRASH_KEY_STRING64("Issue32066", "confirmation_type", + SCOPED_CRASH_KEY_STRING64("Issue43328", "confirmation_type", ToString(ad_history_item.confirmation_type)); - SCOPED_CRASH_KEY_STRING64("Issue32066", "creative_instance_id", + SCOPED_CRASH_KEY_STRING64("Issue43328", "creative_instance_id", ad_history_item.creative_instance_id); - SCOPED_CRASH_KEY_STRING64("Issue32066", "advertiser_id", + SCOPED_CRASH_KEY_STRING64("Issue43328", "advertiser_id", ad_history_item.advertiser_id); - SCOPED_CRASH_KEY_STRING64("Issue32066", "segment", + SCOPED_CRASH_KEY_STRING64("Issue43328", "segment", ad_history_item.segment); - SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason", + SCOPED_CRASH_KEY_STRING64("Issue43328", "failure_reason", "Invalid ad history item"); base::debug::DumpWithoutCrashing(); From 6f152eb64e7259c980267324fa72404bdf405906 Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Wed, 15 Jan 2025 13:35:36 -0500 Subject: [PATCH 07/10] [ads] Database DumpWithoutCrashing code health --- .../brave_ads/core/internal/database/database.cc | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/components/brave_ads/core/internal/database/database.cc b/components/brave_ads/core/internal/database/database.cc index 303b41f3af23..c37433c4df8e 100644 --- a/components/brave_ads/core/internal/database/database.cc +++ b/components/brave_ads/core/internal/database/database.cc @@ -391,16 +391,14 @@ void Database::ErrorCallback(int extended_error, result_code != sql::SqliteResultCode::kIoWrite && result_code != sql::SqliteResultCode::kIoFsync && result_code != sql::SqliteResultCode::kIoTruncate) { - // TODO(https://github.com/brave/brave-browser/issues/32066): Detect - // potential defects using `DumpWithoutCrashing`. - SCOPED_CRASH_KEY_NUMBER("Issue32066", "sqlite_schema_version", + SCOPED_CRASH_KEY_NUMBER("BraveAds", "sqlite_schema_version", database::kVersionNumber); SCOPED_CRASH_KEY_STRING1024( - "Issue32066", "sqlite_diagnostic_info", + "BraveAds", "sqlite_diagnostic_info", db_.GetDiagnosticInfo(extended_error, statement)); - SCOPED_CRASH_KEY_STRING1024("Issue32066", "sqlite_error_message", + SCOPED_CRASH_KEY_STRING1024("BraveAds", "sqlite_error_message", db_.GetErrorMessage()); - SCOPED_CRASH_KEY_NUMBER("Issue32066", "sqlite_result_code", + SCOPED_CRASH_KEY_NUMBER("BraveAds", "sqlite_result_code", static_cast(result_code)); base::debug::DumpWithoutCrashing(); } From a3951398e62777646e9fc89fea079fc2ecadf5c5 Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Wed, 15 Jan 2025 13:38:25 -0500 Subject: [PATCH 08/10] [ads] Failed to raze database DumpWithoutCrashing code health --- .../brave_ads/core/internal/database/database_manager.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/components/brave_ads/core/internal/database/database_manager.cc b/components/brave_ads/core/internal/database/database_manager.cc index c8420ec998c1..3eca6107b539 100644 --- a/components/brave_ads/core/internal/database/database_manager.cc +++ b/components/brave_ads/core/internal/database/database_manager.cc @@ -162,11 +162,11 @@ void DatabaseManager::RazeAndCreateCallback(ResultCallback callback, int from_version, bool success) const { if (!success) { - // TODO(https://github.com/brave/brave-browser/issues/32066): Detect - // potential defects using `DumpWithoutCrashing`. - SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason", + // TODO(https://github.com/brave/brave-browser/issues/43331): Failed to raze + // database. + SCOPED_CRASH_KEY_STRING64("Issue43331", "failure_reason", "Failed to raze database"); - SCOPED_CRASH_KEY_NUMBER("Issue32066", "from_sqlite_schema_version", + SCOPED_CRASH_KEY_NUMBER("Issue43331", "from_sqlite_schema_version", from_version); base::debug::DumpWithoutCrashing(); From 735e55b1249a97f43ffa8d9e95dd0a13e9f456e0 Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Wed, 15 Jan 2025 13:44:02 -0500 Subject: [PATCH 09/10] [ads] Invalid random timer delay DumpWithoutCrashing code health --- components/brave_ads/core/internal/common/timer/timer.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/components/brave_ads/core/internal/common/timer/timer.cc b/components/brave_ads/core/internal/common/timer/timer.cc index 7c4d65ef6291..b3c15f84de6c 100644 --- a/components/brave_ads/core/internal/common/timer/timer.cc +++ b/components/brave_ads/core/internal/common/timer/timer.cc @@ -42,8 +42,9 @@ base::Time Timer::StartWithPrivacy(const base::Location& location, base::OnceClosure user_task) { base::TimeDelta rand_delay = RandTimeDelta(delay); if (rand_delay.is_negative()) { - // TODO(https://github.com/brave/brave-browser/issues/32066): - // Detect potential defects using `DumpWithoutCrashing`. + // TODO(https://github.com/brave/brave-browser/issues/43332): Invalid random + // timer delay. + SCOPED_CRASH_KEY_STRING256("Issue32066", "location", location.ToString()); SCOPED_CRASH_KEY_NUMBER("Issue32066", "rand_delay", rand_delay.InMicroseconds()); SCOPED_CRASH_KEY_STRING64("Issue32066", "failure_reason", From fe64c6f24e2a325b4eb53a954c80723ea69f80ea Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Wed, 15 Jan 2025 13:55:28 -0500 Subject: [PATCH 10/10] [ads] IWYU code health --- .../search_result_ad/creative_search_result_ad_handler.cc | 1 - components/brave_ads/core/internal/common/test/test_base.cc | 1 - components/brave_ads/core/internal/common/test/test_base.h | 3 --- components/brave_ads/core/internal/database/database.cc | 1 - .../brave_ads/core/internal/history/ad_history_manager.h | 2 +- components/brave_ads/core/internal/reminders/reminders.h | 2 +- 6 files changed, 2 insertions(+), 8 deletions(-) diff --git a/components/brave_ads/content/browser/creatives/search_result_ad/creative_search_result_ad_handler.cc b/components/brave_ads/content/browser/creatives/search_result_ad/creative_search_result_ad_handler.cc index 2f268163fc78..500e7de3ffa6 100644 --- a/components/brave_ads/content/browser/creatives/search_result_ad/creative_search_result_ad_handler.cc +++ b/components/brave_ads/content/browser/creatives/search_result_ad/creative_search_result_ad_handler.cc @@ -5,7 +5,6 @@ #include "brave/components/brave_ads/content/browser/creatives/search_result_ad/creative_search_result_ad_handler.h" -#include #include #include "base/functional/bind.h" diff --git a/components/brave_ads/core/internal/common/test/test_base.cc b/components/brave_ads/core/internal/common/test/test_base.cc index 5af58ef80761..2c2556f9c954 100644 --- a/components/brave_ads/core/internal/common/test/test_base.cc +++ b/components/brave_ads/core/internal/common/test/test_base.cc @@ -27,7 +27,6 @@ #include "brave/components/brave_ads/core/internal/common/test/test_constants.h" #include "brave/components/brave_ads/core/internal/common/test/test_types.h" #include "brave/components/brave_ads/core/internal/common/test/time_test_util.h" -#include "brave/components/brave_ads/core/internal/database/database.h" #include "brave/components/brave_ads/core/internal/database/database_manager.h" #include "brave/components/brave_ads/core/internal/deprecated/client/client_state_manager.h" #include "brave/components/brave_ads/core/internal/deprecated/confirmations/confirmation_state_manager.h" diff --git a/components/brave_ads/core/internal/common/test/test_base.h b/components/brave_ads/core/internal/common/test/test_base.h index 8149e0d5f7b1..b91efb151a9d 100644 --- a/components/brave_ads/core/internal/common/test/test_base.h +++ b/components/brave_ads/core/internal/common/test/test_base.h @@ -30,7 +30,6 @@ class TimeDelta; namespace brave_ads { class Ads; -class Database; class GlobalState; namespace test { @@ -159,8 +158,6 @@ class TestBase : public AdsClientNotifierForTesting, public ::testing::Test { ScopedBrowserVersionNumberForTesting scoped_browser_version_number_; - std::unique_ptr database_; - bool is_integration_test_ = false; // Integration tests only. diff --git a/components/brave_ads/core/internal/database/database.cc b/components/brave_ads/core/internal/database/database.cc index c37433c4df8e..771ce83cbbae 100644 --- a/components/brave_ads/core/internal/database/database.cc +++ b/components/brave_ads/core/internal/database/database.cc @@ -14,7 +14,6 @@ #include "base/debug/dump_without_crashing.h" #include "base/files/file_path.h" #include "base/functional/bind.h" -#include "base/location.h" #include "base/logging.h" #include "base/trace_event/trace_event.h" #include "brave/components/brave_ads/core/internal/common/database/database_column_util.h" diff --git a/components/brave_ads/core/internal/history/ad_history_manager.h b/components/brave_ads/core/internal/history/ad_history_manager.h index e2082393c179..f7048052ab2f 100644 --- a/components/brave_ads/core/internal/history/ad_history_manager.h +++ b/components/brave_ads/core/internal/history/ad_history_manager.h @@ -6,10 +6,10 @@ #ifndef BRAVE_COMPONENTS_BRAVE_ADS_CORE_INTERNAL_HISTORY_AD_HISTORY_MANAGER_H_ #define BRAVE_COMPONENTS_BRAVE_ADS_CORE_INTERNAL_HISTORY_AD_HISTORY_MANAGER_H_ +#include #include #include "base/observer_list.h" -#include "base/types/optional_ref.h" #include "brave/components/brave_ads/core/internal/history/ad_history_manager_observer.h" #include "brave/components/brave_ads/core/mojom/brave_ads.mojom-forward.h" #include "brave/components/brave_ads/core/public/ads_callback.h" diff --git a/components/brave_ads/core/internal/reminders/reminders.h b/components/brave_ads/core/internal/reminders/reminders.h index 2de7aa0312a4..98307613cec6 100644 --- a/components/brave_ads/core/internal/reminders/reminders.h +++ b/components/brave_ads/core/internal/reminders/reminders.h @@ -6,11 +6,11 @@ #ifndef BRAVE_COMPONENTS_BRAVE_ADS_CORE_INTERNAL_REMINDERS_REMINDERS_H_ #define BRAVE_COMPONENTS_BRAVE_ADS_CORE_INTERNAL_REMINDERS_REMINDERS_H_ +#include #include #include "base/memory/weak_ptr.h" #include "base/timer/timer.h" -#include "base/types/optional_ref.h" #include "brave/components/brave_ads/core/internal/history/ad_history_manager_observer.h" #include "brave/components/brave_ads/core/public/history/ad_history_item_info.h"