Merge pull request #27253 from brave/issues/43333

[ads] DumpWithoutCrashing code health

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 <string>
 #include <utility>
 
 #include "base/functional/bind.h"

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<T>(mojom_properties, name, value);
+      AddMojomProperty<T>(mojom_properties, name, std::move(value));
     }
   }
 

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;
     }
 

components/brave_ads/core/internal/account/transactions/transactions_database_table.cc

@@ -10,7 +10,6 @@
 #include <vector>
 
 #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;
     }

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();
 

components/brave_ads/core/internal/ads_impl.cc

@@ -9,7 +9,6 @@
 #include <optional>
 #include <utility>
 
-#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));
   }

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"

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> database_;
-
   bool is_integration_test_ = false;
 
   // Integration tests only.

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",

components/brave_ads/core/internal/creatives/conversions/creative_set_conversion_database_table.cc

@@ -52,25 +52,24 @@ 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();
 
       BLOG(0, "Invalid creative set conversion");
-
       continue;
     }
 
@@ -133,25 +132,24 @@ 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();
 
       BLOG(0, "Invalid creative set conversion");
-
       continue;
     }
 

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());
 

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"
@@ -391,16 +390,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<int>(result_code));
       base::debug::DumpWithoutCrashing();
     }

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();
 
@@ -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();
 
@@ -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();
 

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,14 +157,7 @@ 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;
   }
 

components/brave_ads/core/internal/deprecated/confirmations/confirmation_state_manager.cc

@@ -8,7 +8,6 @@
 #include <utility>
 
 #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,14 +112,7 @@ 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;
   }