[DanglingPtr][wallet] Prefer non-nullable states pt.1 (#26332)

This change touches several parts of wallets where we are passing pointers around that are expected to be in a valid state, and remain so for the duration of the lifetime of the object receiving them. Historically, chromium had a guideline that required passing non-owning references as raw pointers. This has changed for a while now, and there are even supporting types like `raw_ref` for it. Raw pointers should be used for cases where flexibility is required about the lifetime of what is being pointed at. However, we should avoid what is in fact an anti-pattern, of passing pointers for the lifetime of an object, and then CHECKing the pointer for its validity. This change also removes certain uses of `std::unique_ptr` that suffers of the same issues. Resolves brave/brave-browser#42016
brave · Nov 1, 2024 · 6d0ffe2 · 6d0ffe2
1 parent 68f4047
commit 6d0ffe2
Show file tree

Hide file tree

Showing 71 changed files with 491 additions and 558 deletions.
diff --git a/browser/brave_wallet/eth_pending_tx_tracker_unittest.cc b/browser/brave_wallet/eth_pending_tx_tracker_unittest.cc
@@ -59,7 +59,7 @@ class EthPendingTxTrackerUnitTest : public testing::Test {
     account_resolver_delegate_ =
         std::make_unique<AccountResolverDelegateForTest>();
     tx_state_manager_ = std::make_unique<EthTxStateManager>(
-        GetPrefs(), delegate_.get(), account_resolver_delegate_.get());
+        *delegate_, *account_resolver_delegate_);
 
     eth_account_id_ = account_resolver_delegate_->RegisterAccount(
         MakeAccountId(mojom::CoinType::ETH, mojom::KeyringId::kDefault,

diff --git a/components/brave_wallet/browser/account_discovery_manager_unittest.cc b/components/brave_wallet/browser/account_discovery_manager_unittest.cc
@@ -45,7 +45,7 @@ class AccountDiscoveryManagerUnitTest : public testing::Test {
         std::make_unique<KeyringService>(nullptr, &prefs_, &local_state_);
     bitcoin_test_rpc_server_ = std::make_unique<BitcoinTestRpcServer>();
     bitcoin_wallet_service_ = std::make_unique<BitcoinWalletService>(
-        keyring_service_.get(), &prefs_, network_manager_.get(),
+        *keyring_service_, *network_manager_,
         bitcoin_test_rpc_server_->GetURLLoaderFactory());
     bitcoin_wallet_service_->SetArrangeTransactionsForTesting(true);
 

diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_block_tracker.cc b/components/brave_wallet/browser/bitcoin/bitcoin_block_tracker.cc
@@ -15,7 +15,7 @@
 
 namespace brave_wallet {
 
-BitcoinBlockTracker::BitcoinBlockTracker(bitcoin_rpc::BitcoinRpc* bitcoin_rpc)
+BitcoinBlockTracker::BitcoinBlockTracker(bitcoin_rpc::BitcoinRpc& bitcoin_rpc)
     : bitcoin_rpc_(bitcoin_rpc) {}
 
 BitcoinBlockTracker::~BitcoinBlockTracker() = default;

diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_block_tracker.h b/components/brave_wallet/browser/bitcoin/bitcoin_block_tracker.h
@@ -10,6 +10,7 @@
 #include <optional>
 #include <string>
 
+#include "base/memory/raw_ref.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/time/time.h"
@@ -24,7 +25,7 @@ class BitcoinRpc;
 
 class BitcoinBlockTracker : public BlockTracker {
  public:
-  explicit BitcoinBlockTracker(bitcoin_rpc::BitcoinRpc* bitcoin_rpc);
+  explicit BitcoinBlockTracker(bitcoin_rpc::BitcoinRpc& bitcoin_rpc);
   ~BitcoinBlockTracker() override;
   BitcoinBlockTracker(const BitcoinBlockTracker&) = delete;
   BitcoinBlockTracker operator=(const BitcoinBlockTracker&) = delete;
@@ -50,7 +51,7 @@ class BitcoinBlockTracker : public BlockTracker {
   std::map<std::string, uint32_t> latest_height_map_;
   base::ObserverList<Observer> observers_;
 
-  raw_ptr<bitcoin_rpc::BitcoinRpc> bitcoin_rpc_ = nullptr;
+  const raw_ref<bitcoin_rpc::BitcoinRpc> bitcoin_rpc_;
 
   base::WeakPtrFactory<BitcoinBlockTracker> weak_ptr_factory_{this};
 };

diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_block_tracker_unittest.cc b/components/brave_wallet/browser/bitcoin/bitcoin_block_tracker_unittest.cc
@@ -60,8 +60,8 @@ class BitcoinBlockTrackerUnitTest : public testing::Test {
     brave_wallet::RegisterProfilePrefs(prefs_.registry());
     network_manager_ = std::make_unique<NetworkManager>(&prefs_);
     bitcoin_rpc_ = std::make_unique<bitcoin_rpc::BitcoinRpc>(
-        network_manager_.get(), shared_url_loader_factory_);
-    tracker_ = std::make_unique<BitcoinBlockTracker>(bitcoin_rpc_.get());
+        *network_manager_, shared_url_loader_factory_);
+    tracker_ = std::make_unique<BitcoinBlockTracker>(*bitcoin_rpc_);
   }
 
   std::string GetResponseString() const {

diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_discover_account_task.cc b/components/brave_wallet/browser/bitcoin/bitcoin_discover_account_task.cc
@@ -64,7 +64,7 @@ bool DiscoveredBitcoinAccount::operator==(
 }
 
 DiscoverAccountTaskBase::DiscoverAccountTaskBase(
-    BitcoinWalletService* bitcoin_wallet_service,
+    BitcoinWalletService& bitcoin_wallet_service,
     const std::string& network_id)
     : bitcoin_wallet_service_(bitcoin_wallet_service),
       network_id_(network_id),
@@ -211,7 +211,7 @@ void DiscoverAccountTaskBase::OnGetAddressStats(
 }
 
 DiscoverWalletAccountTask::DiscoverWalletAccountTask(
-    BitcoinWalletService* bitcoin_wallet_service,
+    BitcoinWalletService& bitcoin_wallet_service,
     mojom::KeyringId keyring_id,
     uint32_t account_index)
     : DiscoverAccountTaskBase(bitcoin_wallet_service,
@@ -224,12 +224,13 @@ DiscoverWalletAccountTask::~DiscoverWalletAccountTask() = default;
 
 mojom::BitcoinAddressPtr DiscoverWalletAccountTask::GetAddressById(
     const mojom::BitcoinKeyIdPtr& key_id) {
-  return bitcoin_wallet_service_->keyring_service()
-      ->GetBitcoinAccountDiscoveryAddress(keyring_id_, account_index_, key_id);
+  return bitcoin_wallet_service()
+      .keyring_service()
+      .GetBitcoinAccountDiscoveryAddress(keyring_id_, account_index_, key_id);
 }
 
 DiscoverExtendedKeyAccountTask::DiscoverExtendedKeyAccountTask(
-    BitcoinWalletService* bitcoin_wallet_service,
+    BitcoinWalletService& bitcoin_wallet_service,
     const std::string& network_id,
     const std::string& extended_key)
     : DiscoverAccountTaskBase(bitcoin_wallet_service, network_id),

diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_discover_account_task.h b/components/brave_wallet/browser/bitcoin/bitcoin_discover_account_task.h
@@ -8,7 +8,7 @@
 
 #include <string>
 
-#include "base/memory/raw_ptr.h"
+#include "base/memory/raw_ref.h"
 #include "base/memory/weak_ptr.h"
 #include "base/types/expected.h"
 #include "brave/components/brave_wallet/browser/bitcoin/bitcoin_rpc.h"
@@ -17,7 +17,6 @@
 namespace brave_wallet {
 
 class BitcoinWalletService;
-class KeyringService;
 class HDKey;
 
 struct DiscoveredBitcoinAccount {
@@ -37,7 +36,7 @@ class DiscoverAccountTaskBase {
   using DiscoverAccountCallback = base::OnceCallback<void(
       base::expected<DiscoveredBitcoinAccount, std::string>)>;
 
-  DiscoverAccountTaskBase(BitcoinWalletService* bitcoin_wallet_service,
+  DiscoverAccountTaskBase(BitcoinWalletService& bitcoin_wallet_service,
                           const std::string& network_id);
   virtual ~DiscoverAccountTaskBase();
 
@@ -64,8 +63,12 @@ class DiscoverAccountTaskBase {
       mojom::BitcoinAddressPtr address,
       base::expected<bitcoin_rpc::AddressStats, std::string> stats);
 
-  raw_ptr<BitcoinWalletService> bitcoin_wallet_service_;
-  raw_ptr<KeyringService> keyring_service_;
+  BitcoinWalletService& bitcoin_wallet_service() {
+    return *bitcoin_wallet_service_;
+  }
+
+ private:
+  const raw_ref<BitcoinWalletService> bitcoin_wallet_service_;
   std::string network_id_;
 
   uint32_t active_requests_ = 0;
@@ -82,7 +85,7 @@ class DiscoverAccountTaskBase {
 
 class DiscoverWalletAccountTask : public DiscoverAccountTaskBase {
  public:
-  DiscoverWalletAccountTask(BitcoinWalletService* bitcoin_wallet_service,
+  DiscoverWalletAccountTask(BitcoinWalletService& bitcoin_wallet_service,
                             mojom::KeyringId keyring_id,
                             uint32_t account_index);
   ~DiscoverWalletAccountTask() override;
@@ -96,7 +99,7 @@ class DiscoverWalletAccountTask : public DiscoverAccountTaskBase {
 
 class DiscoverExtendedKeyAccountTask : public DiscoverAccountTaskBase {
  public:
-  DiscoverExtendedKeyAccountTask(BitcoinWalletService* bitcoin_wallet_service,
+  DiscoverExtendedKeyAccountTask(BitcoinWalletService& bitcoin_wallet_service,
                                  const std::string& network_id,
                                  const std::string& extended_key);
   ~DiscoverExtendedKeyAccountTask() override;

diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_fetch_raw_transactions_task.cc b/components/brave_wallet/browser/bitcoin/bitcoin_fetch_raw_transactions_task.cc
@@ -23,7 +23,7 @@
 namespace brave_wallet {
 
 FetchRawTransactionsTask::FetchRawTransactionsTask(
-    BitcoinWalletService* bitcoin_wallet_service,
+    BitcoinWalletService& bitcoin_wallet_service,
     const std::string& network_id,
     const std::vector<SHA256HashArray>& txids)
     : bitcoin_wallet_service_(bitcoin_wallet_service),

diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_fetch_raw_transactions_task.h b/components/brave_wallet/browser/bitcoin/bitcoin_fetch_raw_transactions_task.h
@@ -11,7 +11,7 @@
 #include <utility>
 #include <vector>
 
-#include "base/memory/raw_ptr.h"
+#include "base/memory/raw_ref.h"
 #include "base/memory/weak_ptr.h"
 #include "base/types/expected.h"
 #include "brave/components/brave_wallet/common/brave_wallet.mojom.h"
@@ -26,7 +26,7 @@ class FetchRawTransactionsTask {
   using FetchRawTransactionsTaskCallback = base::OnceCallback<void(
       base::expected<std::vector<std::vector<uint8_t>>, std::string>)>;
 
-  FetchRawTransactionsTask(BitcoinWalletService* bitcoin_wallet_service,
+  FetchRawTransactionsTask(BitcoinWalletService& bitcoin_wallet_service,
                            const std::string& network_id,
                            const std::vector<SHA256HashArray>& txids);
   virtual ~FetchRawTransactionsTask();
@@ -44,7 +44,7 @@ class FetchRawTransactionsTask {
       SHA256HashArray txid,
       base::expected<std::vector<uint8_t>, std::string> raw_tx);
 
-  raw_ptr<BitcoinWalletService> bitcoin_wallet_service_;
+  const raw_ref<BitcoinWalletService> bitcoin_wallet_service_;
   std::string network_id_;
   std::vector<SHA256HashArray> txids_;
   bool requests_queued_ = false;

diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_rpc.cc b/components/brave_wallet/browser/bitcoin/bitcoin_rpc.cc
@@ -227,11 +227,11 @@ struct EndpointQueue {
 };
 
 BitcoinRpc::BitcoinRpc(
-    NetworkManager* network_manager,
+    NetworkManager& network_manager,
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
     : network_manager_(network_manager),
-      api_request_helper_(new APIRequestHelper(GetNetworkTrafficAnnotationTag(),
-                                               url_loader_factory)) {}
+      api_request_helper_(GetNetworkTrafficAnnotationTag(),
+                          url_loader_factory) {}
 
 BitcoinRpc::~BitcoinRpc() = default;
 
@@ -461,10 +461,10 @@ void BitcoinRpc::PostTransaction(const std::string& chain_id,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback));
 
   auto conversion_callback = base::BindOnce(&ConvertPlainStringToJsonArray);
-  api_request_helper_->Request(
-      net::HttpRequestHeaders::kPostMethod, request_url, payload, "",
-      std::move(internal_callback), {}, {.auto_retry_on_network_change = true},
-      std::move(conversion_callback));
+  api_request_helper_.Request(net::HttpRequestHeaders::kPostMethod, request_url,
+                              payload, "", std::move(internal_callback), {},
+                              {.auto_retry_on_network_change = true},
+                              std::move(conversion_callback));
 }
 
 void BitcoinRpc::OnPostTransaction(PostTransactionCallback callback,
@@ -537,7 +537,7 @@ void BitcoinRpc::MaybeStartQueuedRequest(const GURL& endpoint_host) {
   endpoint.requests_queue.pop_front();
 
   endpoint.active_requests++;
-  api_request_helper_->Request(
+  api_request_helper_.Request(
       net::HttpRequestHeaders::kGetMethod, request.request_url, "", "",
       base::BindOnce(&BitcoinRpc::OnRequestInternalDone,
                      weak_ptr_factory_.GetWeakPtr(), endpoint_host,
@@ -548,7 +548,7 @@ void BitcoinRpc::MaybeStartQueuedRequest(const GURL& endpoint_host) {
 
 void BitcoinRpc::SetUrlLoaderFactoryForTesting(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory) {
-  api_request_helper_->SetUrlLoaderFactoryForTesting(  // IN-TEST
+  api_request_helper_.SetUrlLoaderFactoryForTesting(  // IN-TEST
       std::move(url_loader_factory));
 }
 

diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_rpc.h b/components/brave_wallet/browser/bitcoin/bitcoin_rpc.h
@@ -8,11 +8,11 @@
 
 #include <list>
 #include <map>
-#include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
+#include "base/memory/raw_ref.h"
 #include "base/memory/weak_ptr.h"
 #include "base/types/expected.h"
 #include "brave/components/api_request_helper/api_request_helper.h"
@@ -30,7 +30,7 @@ struct EndpointQueue;
 
 class BitcoinRpc {
  public:
-  BitcoinRpc(NetworkManager* network_manager,
+  BitcoinRpc(NetworkManager& network_manager,
              scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
   ~BitcoinRpc();
 
@@ -107,11 +107,11 @@ class BitcoinRpc {
 
   GURL GetNetworkURL(const std::string& chain_id);
 
-  const raw_ptr<NetworkManager> network_manager_;
+  const raw_ref<NetworkManager> network_manager_;
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
   // Uses hostname as key. Tracks request throttling(if required) per host.
   std::map<std::string, EndpointQueue> endpoints_;
-  std::unique_ptr<APIRequestHelper> api_request_helper_;
+  APIRequestHelper api_request_helper_;
   base::WeakPtrFactory<BitcoinRpc> weak_ptr_factory_{this};
 };
 

diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_rpc_unittest.cc b/components/brave_wallet/browser/bitcoin/bitcoin_rpc_unittest.cc
@@ -59,7 +59,7 @@ class BitcoinRpcUnitTest : public testing::Test {
     brave_wallet::RegisterProfilePrefs(prefs_.registry());
     network_manager_ = std::make_unique<NetworkManager>(&prefs_);
     bitcoin_rpc_ = std::make_unique<bitcoin_rpc::BitcoinRpc>(
-        network_manager_.get(), shared_url_loader_factory_);
+        *network_manager_, shared_url_loader_factory_);
 
     mainnet_rpc_url_ =
         network_manager_

diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_test_utils.cc b/components/brave_wallet/browser/bitcoin/bitcoin_test_utils.cc
@@ -119,9 +119,9 @@ BitcoinTestRpcServer::BitcoinTestRpcServer()
 }
 
 BitcoinTestRpcServer::BitcoinTestRpcServer(
-    BitcoinWalletService* bitcoin_wallet_service)
+    BitcoinWalletService& bitcoin_wallet_service)
     : BitcoinTestRpcServer() {
-  bitcoin_wallet_service->SetUrlLoaderFactoryForTesting(GetURLLoaderFactory());
+  bitcoin_wallet_service.SetUrlLoaderFactoryForTesting(GetURLLoaderFactory());
 }
 
 BitcoinTestRpcServer::~BitcoinTestRpcServer() = default;

diff --git a/components/brave_wallet/browser/bitcoin/bitcoin_test_utils.h b/components/brave_wallet/browser/bitcoin/bitcoin_test_utils.h
@@ -60,7 +60,7 @@ inline constexpr char kBtcTestnetHardwareAccount1[] =
 class BitcoinTestRpcServer {
  public:
   BitcoinTestRpcServer();
-  explicit BitcoinTestRpcServer(BitcoinWalletService* bitcoin_wallet_service);
+  explicit BitcoinTestRpcServer(BitcoinWalletService& bitcoin_wallet_service);
   ~BitcoinTestRpcServer();
 
   static bitcoin_rpc::AddressStats EmptyAddressStats(