From 71a75daedc10f26018ac6899d2d30055e67c1fb3 Mon Sep 17 00:00:00 2001 From: vadims <118171981+vadimstruts@users.noreply.github.com> Date: Thu, 14 Sep 2023 22:35:37 +0200 Subject: [PATCH] Refactor IpfsService to use single ApiRequestHelper (#20089) * refactored ipfs_service h/cc * changed to use weak reference instead of base::Unretained --------- Signed-off-by: Vadym Struts --- components/ipfs/ipfs_service.cc | 187 ++++++++++---------------------- components/ipfs/ipfs_service.h | 43 +++----- 2 files changed, 73 insertions(+), 157 deletions(-) diff --git a/components/ipfs/ipfs_service.cc b/components/ipfs/ipfs_service.cc index 413640ab9ae0..035d49bb445f 100644 --- a/components/ipfs/ipfs_service.cc +++ b/components/ipfs/ipfs_service.cc @@ -9,11 +9,9 @@ #include #include "base/files/file_util.h" -#include "base/json/json_reader.h" #include "base/memory/raw_ref.h" #include "base/rand_util.h" #include "base/strings/strcat.h" -#include "base/strings/string_util.h" #include "base/strings/stringprintf.h" #include "base/task/thread_pool.h" #include "brave/base/process/process_launcher.h" @@ -31,14 +29,10 @@ #include "components/grit/brave_components_strings.h" #include "components/prefs/pref_registry_simple.h" #include "components/prefs/pref_service.h" -#include "components/user_prefs/user_prefs.h" #include "net/base/url_util.h" #include "net/http/http_request_headers.h" -#include "net/http/http_response_headers.h" -#include "net/http/http_status_code.h" #include "services/network/public/cpp/shared_url_loader_factory.h" #include "services/network/public/cpp/simple_url_loader.h" -#include "services/network/public/mojom/url_response_head.mojom.h" #include "url/gurl.h" #if BUILDFLAG(ENABLE_IPFS_LOCAL_NODE) @@ -48,7 +42,6 @@ #include "brave/components/ipfs/keys/ipns_keys_manager.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/service_process_host.h" -#include "content/public/browser/storage_partition.h" #endif namespace { @@ -100,6 +93,11 @@ base::flat_map GetHeaders(const GURL& url) { {net::HttpRequestHeaders::kOrigin, url::Origin::Create(url).Serialize()}}; } +absl::optional ConvertPlainStringToJsonArray( + const std::string& json) { + return base::StrCat({"[\"", json, "\"]"}); +} + } // namespace namespace ipfs { @@ -125,10 +123,12 @@ IpfsService::IpfsService( file_task_runner_(base::ThreadPool::CreateSequencedTaskRunner( {base::MayBlock(), base::TaskPriority::BEST_EFFORT, base::TaskShutdownBehavior::BLOCK_SHUTDOWN})), - ipfs_p3a_(this, prefs), - weak_factory_(this) { + ipfs_p3a_(this, prefs) { DCHECK(!user_data_dir.empty()); + api_request_helper_ = std::make_unique( + GetIpfsNetworkTrafficAnnotationTag(), url_loader_factory); + // Return early since g_brave_browser_process and ipfs_client_updater are not // available in unit tests. if (ipfs_client_updater_) { @@ -144,7 +144,7 @@ IpfsService::IpfsService( ipfs_dns_resolver_subscription_ = ipfs_dns_resolver_->AddObserver(base::BindRepeating( - &IpfsService::OnDnsConfigChanged, base::Unretained(this))); + &IpfsService::OnDnsConfigChanged, weak_factory_.GetWeakPtr())); } IpfsService::~IpfsService() { @@ -231,9 +231,9 @@ void IpfsService::LaunchIfNotRunning(const base::FilePath& executable_path) { .Pass()); ipfs_service_.set_disconnect_handler( - base::BindOnce(&IpfsService::OnIpfsCrashed, base::Unretained(this))); + base::BindOnce(&IpfsService::OnIpfsCrashed, weak_factory_.GetWeakPtr())); ipfs_service_->SetCrashHandler(base::BindOnce( - &IpfsService::OnIpfsDaemonCrashed, base::Unretained(this))); + &IpfsService::OnIpfsDaemonCrashed, weak_factory_.GetWeakPtr())); auto config = mojom::IpfsConfig::New( executable_path, GetConfigFilePath(), GetDataPath(), @@ -242,7 +242,7 @@ void IpfsService::LaunchIfNotRunning(const base::FilePath& executable_path) { ipfs_service_->Launch( std::move(config), - base::BindOnce(&IpfsService::OnIpfsLaunched, base::Unretained(this))); + base::BindOnce(&IpfsService::OnIpfsLaunched, weak_factory_.GetWeakPtr())); #endif } @@ -250,7 +250,7 @@ void IpfsService::RestartDaemon() { if (!IsDaemonLaunched()) return; auto launch_callback = - base::BindOnce(&IpfsService::LaunchDaemon, base::Unretained(this)); + base::BindOnce(&IpfsService::LaunchDaemon, weak_factory_.GetWeakPtr()); ShutdownDaemon(base::BindOnce( [](base::OnceCallback launch_callback, const bool success) { @@ -428,16 +428,10 @@ void IpfsService::AddPin(const std::vector& cids, } gurl = net::AppendQueryParameter(gurl, "recursive", recursive ? "true" : "false"); - - auto url_loader = std::make_unique( - GetIpfsNetworkTrafficAnnotationTag(), url_loader_factory_); - auto iter = - requests_list_.insert(requests_list_.begin(), std::move(url_loader)); - - iter->get()->Request( + api_request_helper_->Request( "POST", gurl, std::string(), std::string(), - base::BindOnce(&IpfsService::OnPinAddResult, base::Unretained(this), - cids.size(), recursive, iter, std::move(callback)), + base::BindOnce(&IpfsService::OnPinAddResult, weak_factory_.GetWeakPtr(), + cids.size(), recursive, std::move(callback)), GetHeaders(gurl), api_request_helper::APIRequestOptions{.timeout = base::Minutes(2)}); } @@ -454,15 +448,10 @@ void IpfsService::RemovePin(const std::vector& cids, gurl = net::AppendQueryParameter(gurl, kArgQueryParam, cid); } - auto url_loader = std::make_unique( - GetIpfsNetworkTrafficAnnotationTag(), url_loader_factory_); - auto iter = - requests_list_.insert(requests_list_.begin(), std::move(url_loader)); - - iter->get()->Request( + api_request_helper_->Request( "POST", gurl, std::string(), std::string(), - base::BindOnce(&IpfsService::OnPinRemoveResult, base::Unretained(this), - iter, std::move(callback)), + base::BindOnce(&IpfsService::OnPinRemoveResult, + weak_factory_.GetWeakPtr(), std::move(callback)), GetHeaders(gurl)); } @@ -550,14 +539,10 @@ void IpfsService::GetPins(const absl::optional>& cids, gurl = net::AppendQueryParameter(gurl, "type", type); gurl = net::AppendQueryParameter(gurl, "quiet", quiet ? "true" : "false"); - auto url_loader = std::make_unique( - GetIpfsNetworkTrafficAnnotationTag(), url_loader_factory_); - auto iter = - requests_list_.insert(requests_list_.begin(), std::move(url_loader)); - iter->get()->Request( + api_request_helper_->Request( "POST", gurl, std::string(), std::string(), - base::BindOnce(&IpfsService::OnGetPinsResult, base::Unretained(this), - iter, std::move(callback)), + base::BindOnce(&IpfsService::OnGetPinsResult, weak_factory_.GetWeakPtr(), + std::move(callback)), GetHeaders(gurl)); } @@ -697,15 +682,10 @@ void IpfsService::GetConnectedPeers(GetConnectedPeersCallback callback, return; } auto gurl = server_endpoint_.Resolve(kSwarmPeersPath); - auto url_loader = std::make_unique( - GetIpfsNetworkTrafficAnnotationTag(), url_loader_factory_); - - auto iter = - requests_list_.insert(requests_list_.begin(), std::move(url_loader)); - iter->get()->Request( + api_request_helper_->Request( "POST", gurl, std::string(), std::string(), - base::BindOnce(&IpfsService::OnGetConnectedPeers, base::Unretained(this), - iter, std::move(callback), + base::BindOnce(&IpfsService::OnGetConnectedPeers, + weak_factory_.GetWeakPtr(), std::move(callback), retries.value_or(kPeersDefaultRetries)), GetHeaders(gurl)); } @@ -719,12 +699,10 @@ base::TimeDelta IpfsService::CalculatePeersRetryTime() { } void IpfsService::OnGetConnectedPeers( - APIRequestList::iterator iter, GetConnectedPeersCallback callback, int retry_number, api_request_helper::APIRequestResult response) { int response_code = response.response_code(); - requests_list_.erase(iter); bool success = response.Is2XXResponseCode(); last_peers_retry_value_for_test_ = retry_number; @@ -762,26 +740,18 @@ void IpfsService::GetAddressesConfig(GetAddressesConfigCallback callback) { GURL gurl = net::AppendQueryParameter(server_endpoint_.Resolve(kConfigPath), kArgQueryParam, kAddressesField); - - auto url_loader = std::make_unique( - GetIpfsNetworkTrafficAnnotationTag(), url_loader_factory_); - - auto iter = - requests_list_.insert(requests_list_.begin(), std::move(url_loader)); - iter->get()->Request( + api_request_helper_->Request( "POST", gurl, std::string(), std::string(), - base::BindOnce(&IpfsService::OnGetAddressesConfig, base::Unretained(this), - iter, std::move(callback)), + base::BindOnce(&IpfsService::OnGetAddressesConfig, + weak_factory_.GetWeakPtr(), std::move(callback)), GetHeaders(gurl)); } void IpfsService::OnGetAddressesConfig( - APIRequestList::iterator iter, GetAddressesConfigCallback callback, api_request_helper::APIRequestResult response) { int response_code = response.response_code(); bool success = response.Is2XXResponseCode(); - requests_list_.erase(iter); ipfs::AddressesConfig addresses_config; if (!success) { @@ -935,23 +905,16 @@ void IpfsService::GetRepoStats(GetRepoStatsCallback callback) { net::AppendQueryParameter(server_endpoint_.Resolve(ipfs::kRepoStatsPath), ipfs::kRepoStatsHumanReadableParamName, ipfs::kRepoStatsHumanReadableParamValue); - auto url_loader = std::make_unique( - GetIpfsNetworkTrafficAnnotationTag(), url_loader_factory_); - - auto iter = - requests_list_.insert(requests_list_.begin(), std::move(url_loader)); - iter->get()->Request( + api_request_helper_->Request( "POST", gurl, std::string(), std::string(), - base::BindOnce(&IpfsService::OnRepoStats, base::Unretained(this), iter, + base::BindOnce(&IpfsService::OnRepoStats, weak_factory_.GetWeakPtr(), std::move(callback)), GetHeaders(gurl)); } -void IpfsService::OnRepoStats(APIRequestList::iterator iter, - GetRepoStatsCallback callback, +void IpfsService::OnRepoStats(GetRepoStatsCallback callback, api_request_helper::APIRequestResult response) { int response_code = response.response_code(); - requests_list_.erase(iter); bool success = response.Is2XXResponseCode(); @@ -975,23 +938,16 @@ void IpfsService::GetNodeInfo(GetNodeInfoCallback callback) { GURL gurl = server_endpoint_.Resolve(ipfs::kNodeInfoPath); - auto url_loader = std::make_unique( - GetIpfsNetworkTrafficAnnotationTag(), url_loader_factory_); - - auto iter = - requests_list_.insert(requests_list_.begin(), std::move(url_loader)); - iter->get()->Request( + api_request_helper_->Request( "POST", gurl, std::string(), std::string(), - base::BindOnce(&IpfsService::OnNodeInfo, base::Unretained(this), iter, + base::BindOnce(&IpfsService::OnNodeInfo, weak_factory_.GetWeakPtr(), std::move(callback)), GetHeaders(gurl)); } -void IpfsService::OnNodeInfo(APIRequestList::iterator iter, - GetNodeInfoCallback callback, +void IpfsService::OnNodeInfo(GetNodeInfoCallback callback, api_request_helper::APIRequestResult response) { int response_code = response.response_code(); - requests_list_.erase(iter); bool success = response.Is2XXResponseCode(); @@ -1015,24 +971,17 @@ void IpfsService::RunGarbageCollection(GarbageCollectionCallback callback) { GURL gurl = server_endpoint_.Resolve(ipfs::kGarbageCollectionPath); - auto url_loader = std::make_unique( - GetIpfsNetworkTrafficAnnotationTag(), url_loader_factory_); - - auto iter = - requests_list_.insert(requests_list_.begin(), std::move(url_loader)); - iter->get()->Request( + api_request_helper_->Request( "POST", gurl, std::string(), std::string(), - base::BindOnce(&IpfsService::OnGarbageCollection, base::Unretained(this), - iter, std::move(callback)), + base::BindOnce(&IpfsService::OnGarbageCollection, + weak_factory_.GetWeakPtr(), std::move(callback)), GetHeaders(gurl)); } void IpfsService::OnGarbageCollection( - APIRequestList::iterator iter, GarbageCollectionCallback callback, api_request_helper::APIRequestResult response) { int response_code = response.response_code(); - requests_list_.erase(iter); bool success = response.Is2XXResponseCode(); if (!success) { @@ -1048,21 +997,14 @@ void IpfsService::OnGarbageCollection( } void IpfsService::PreWarmShareableLink(const GURL& url) { - auto url_loader = std::make_unique( - GetIpfsNetworkTrafficAnnotationTag(), url_loader_factory_); - - auto iter = - requests_list_.insert(requests_list_.begin(), std::move(url_loader)); - iter->get()->Request("HEAD", url, std::string(), std::string(), - base::BindOnce(&IpfsService::OnPreWarmComplete, - base::Unretained(this), iter), - GetHeaders(url)); + api_request_helper_->Request("HEAD", url, std::string(), std::string(), + base::BindOnce(&IpfsService::OnPreWarmComplete, + weak_factory_.GetWeakPtr()), + GetHeaders(url)); } void IpfsService::OnPreWarmComplete( - APIRequestList::iterator iter, - api_request_helper::APIRequestResult response) { - requests_list_.erase(iter); + [[maybe_unused]] api_request_helper::APIRequestResult response) { if (prewarm_callback_for_testing_) std::move(prewarm_callback_for_testing_).Run(); } @@ -1082,11 +1024,9 @@ void IpfsService::OnPreWarmComplete( // } //} void IpfsService::OnGetPinsResult( - APIRequestList::iterator iter, GetPinsCallback callback, api_request_helper::APIRequestResult response) { int response_code = response.response_code(); - requests_list_.erase(iter); if (!response.Is2XXResponseCode()) { VLOG(1) << "Fail to get pins, response_code = " << response_code; @@ -1108,11 +1048,9 @@ void IpfsService::OnGetPinsResult( void IpfsService::OnPinAddResult( size_t cids_count_in_request, bool recursive, - APIRequestList::iterator iter, AddPinCallback callback, api_request_helper::APIRequestResult response) { int response_code = response.response_code(); - requests_list_.erase(iter); if (!response.Is2XXResponseCode()) { VLOG(1) << "Fail to add pin, response_code = " << response_code; @@ -1140,11 +1078,9 @@ void IpfsService::OnPinAddResult( } void IpfsService::OnPinRemoveResult( - APIRequestList::iterator iter, RemovePinCallback callback, api_request_helper::APIRequestResult response) { int response_code = response.response_code(); - requests_list_.erase(iter); if (!response.Is2XXResponseCode()) { VLOG(1) << "Fail to remove pin, response_code = " << response_code; @@ -1169,39 +1105,34 @@ void IpfsService::ValidateGateway(const GURL& url, BoolCallback callback) { std::string path = "/ipfs/"; path += kGatewayValidationCID; replacements.SetPathStr(path); - GURL validationUrl = url.ReplaceComponents(replacements); - auto url_loader = CreateURLLoader(validationUrl, "GET"); - auto iter = url_loaders_.insert(url_loaders_.begin(), std::move(url_loader)); - iter->get()->DownloadToStringOfUnboundedSizeUntilCrashAndDie( - url_loader_factory_.get(), + GURL validation_url = url.ReplaceComponents(replacements); + + auto conversion_callback = base::BindOnce(&ConvertPlainStringToJsonArray); + api_request_helper_->Request( + "GET", validation_url, "", "", base::BindOnce(&IpfsService::OnGatewayValidationComplete, - base::Unretained(this), iter, std::move(callback), url)); + weak_factory_.GetWeakPtr(), std::move(callback), url), + {}, {}, std::move(conversion_callback)); } void IpfsService::OnGatewayValidationComplete( - SimpleURLLoaderList::iterator iter, BoolCallback callback, const GURL& initial_url, - std::unique_ptr response_body) { - auto* url_loader = iter->get(); - auto final_url = url_loader->GetFinalURL(); - - int error_code = url_loader->NetError(); - int response_code = -1; - if (url_loader->ResponseInfo() && url_loader->ResponseInfo()->headers) - response_code = url_loader->ResponseInfo()->headers->response_code(); - url_loaders_.erase(iter); + api_request_helper::APIRequestResult response) const { + int response_code = response.response_code(); - bool success = (error_code == net::OK && response_code == net::HTTP_OK); + bool success = response.Is2XXResponseCode(); if (!success) { - VLOG(1) << "Fail to validate gateway, error_code = " << error_code - << " response_code = " << response_code; + VLOG(1) << "Fail to validate gateway, response_code = " << response_code; } + std::string error; if (success) { - std::string valid_host = base::StringPrintf( + const auto final_url = response.final_url(); + const std::string valid_host = base::StringPrintf( "%s.ipfs.%s", kGatewayValidationCID, initial_url.host().c_str()); - success = (*response_body == kGatewayValidationResult) && + success = (response.body() == + ConvertPlainStringToJsonArray(kGatewayValidationResult)) && (initial_url.host() != final_url.host()) && (initial_url.scheme() == final_url.scheme()) && (final_url.host() == valid_host); diff --git a/components/ipfs/ipfs_service.h b/components/ipfs/ipfs_service.h index bf325737f78f..4649809b8a1a 100644 --- a/components/ipfs/ipfs_service.h +++ b/components/ipfs/ipfs_service.h @@ -49,7 +49,6 @@ class SequencedTaskRunner; namespace network { class SharedURLLoaderFactory; -class SimpleURLLoader; } // namespace network class PrefRegistrySimple; @@ -191,10 +190,6 @@ class IpfsService : public KeyedService, void OnConfigLoaded(GetConfigCallback, const std::pair&); private: - using APIRequestList = - std::list>; - using SimpleURLLoaderList = - std::list>; FRIEND_TEST_ALL_PREFIXES(IpfsServiceBrowserTest, UpdaterRegistrationSuccessLaunch); FRIEND_TEST_ALL_PREFIXES(IpfsServiceBrowserTest, @@ -221,16 +216,13 @@ class IpfsService : public KeyedService, NodeCallback callback); // Local pins - void OnGetPinsResult(APIRequestList::iterator iter, - GetPinsCallback callback, + void OnGetPinsResult(GetPinsCallback callback, api_request_helper::APIRequestResult response); void OnPinAddResult(size_t cids_count_in_request, bool recursive, - APIRequestList::iterator iter, AddPinCallback callback, api_request_helper::APIRequestResult response); - void OnPinRemoveResult(APIRequestList::iterator iter, - RemovePinCallback callback, + void OnPinRemoveResult(RemovePinCallback callback, api_request_helper::APIRequestResult response); void OnRemovePinCli(BoolCallback callback, std::set cids, @@ -238,29 +230,23 @@ class IpfsService : public KeyedService, #endif base::TimeDelta CalculatePeersRetryTime(); - void OnGatewayValidationComplete(SimpleURLLoaderList::iterator iter, - BoolCallback callback, - const GURL& initial_url, - std::unique_ptr response_body); + void OnGatewayValidationComplete( + BoolCallback callback, + const GURL& initial_url, + api_request_helper::APIRequestResult response) const; - void OnGetConnectedPeers(APIRequestList::iterator iter, - GetConnectedPeersCallback, + void OnGetConnectedPeers(GetConnectedPeersCallback, int retries, api_request_helper::APIRequestResult response); - void OnGetAddressesConfig(APIRequestList::iterator iter, - GetAddressesConfigCallback callback, + void OnGetAddressesConfig(GetAddressesConfigCallback callback, api_request_helper::APIRequestResult response); - void OnRepoStats(APIRequestList::iterator iter, - GetRepoStatsCallback callback, + void OnRepoStats(GetRepoStatsCallback callback, api_request_helper::APIRequestResult response); - void OnNodeInfo(APIRequestList::iterator iter, - GetNodeInfoCallback callback, + void OnNodeInfo(GetNodeInfoCallback callback, api_request_helper::APIRequestResult response); - void OnGarbageCollection(APIRequestList::iterator iter, - GarbageCollectionCallback callback, + void OnGarbageCollection(GarbageCollectionCallback callback, api_request_helper::APIRequestResult responsey); - void OnPreWarmComplete(APIRequestList::iterator iter, - api_request_helper::APIRequestResult response); + void OnPreWarmComplete(api_request_helper::APIRequestResult response); std::string GetStorageSize(); void OnDnsConfigChanged(absl::optional dns_server); @@ -275,8 +261,7 @@ class IpfsService : public KeyedService, const raw_ptr prefs_ = nullptr; scoped_refptr url_loader_factory_; - APIRequestList requests_list_; - SimpleURLLoaderList url_loaders_; + std::unique_ptr api_request_helper_; BlobContextGetterFactoryPtr blob_context_getter_factory_; base::queue pending_launch_callbacks_; @@ -303,7 +288,7 @@ class IpfsService : public KeyedService, #endif scoped_refptr file_task_runner_; IpfsP3A ipfs_p3a_; - base::WeakPtrFactory weak_factory_; + base::WeakPtrFactory weak_factory_{this}; }; } // namespace ipfs