From 88ac18941523f60acac5603efb28d2ea680ea061 Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Tue, 3 Dec 2024 16:32:10 -0800 Subject: [PATCH 1/3] Upgrade News direct feed requests to HTTPS when possible --- .../brave_news_controller_factory.cc | 9 +++- .../direct_feed_fetcher_delegate_impl.cc | 32 +++++++++++++++ .../direct_feed_fetcher_delegate_impl.h | 41 +++++++++++++++++++ browser/brave_news/sources.gni | 3 ++ .../browser/brave_news_controller.cc | 18 +++++--- .../browser/brave_news_controller.h | 9 +++- .../brave_news/browser/brave_news_engine.cc | 10 +++-- .../brave_news/browser/brave_news_engine.h | 6 ++- .../browser/direct_feed_controller.cc | 11 ++--- .../browser/direct_feed_controller.h | 3 +- .../direct_feed_controller_unittest.cc | 27 +++++++++++- .../brave_news/browser/direct_feed_fetcher.cc | 33 ++++++++++++--- .../brave_news/browser/direct_feed_fetcher.h | 20 +++++++-- .../brave_news/browser/feed_controller.cc | 7 +++- .../brave_news/browser/feed_controller.h | 3 +- components/brave_news/browser/feed_fetcher.cc | 7 ++-- components/brave_news/browser/feed_fetcher.h | 6 +-- .../brave_news/browser/feed_v2_builder.cc | 7 +++- .../brave_news/browser/feed_v2_builder.h | 3 +- 19 files changed, 213 insertions(+), 42 deletions(-) create mode 100644 browser/brave_news/direct_feed_fetcher_delegate_impl.cc create mode 100644 browser/brave_news/direct_feed_fetcher_delegate_impl.h diff --git a/browser/brave_news/brave_news_controller_factory.cc b/browser/brave_news/brave_news_controller_factory.cc index 129ac072830e..c4869a68bf09 100644 --- a/browser/brave_news/brave_news_controller_factory.cc +++ b/browser/brave_news/brave_news_controller_factory.cc @@ -9,7 +9,9 @@ #include "base/no_destructor.h" #include "brave/browser/brave_ads/ads_service_factory.h" +#include "brave/browser/brave_news/direct_feed_fetcher_delegate_impl.h" #include "brave/components/brave_news/browser/brave_news_controller.h" +#include "chrome/browser/content_settings/host_content_settings_map_factory.h" #include "chrome/browser/favicon/favicon_service_factory.h" #include "chrome/browser/history/history_service_factory.h" #include "chrome/browser/profiles/incognito_helpers.h" @@ -48,6 +50,7 @@ BraveNewsControllerFactory::BraveNewsControllerFactory() DependsOn(brave_ads::AdsServiceFactory::GetInstance()); DependsOn(FaviconServiceFactory::GetInstance()); DependsOn(HistoryServiceFactory::GetInstance()); + DependsOn(HostContentSettingsMapFactory::GetInstance()); } BraveNewsControllerFactory::~BraveNewsControllerFactory() = default; @@ -71,9 +74,13 @@ BraveNewsControllerFactory::BuildServiceInstanceForBrowserContext( auto* ads_service = brave_ads::AdsServiceFactory::GetForProfile(profile); auto* history_service = HistoryServiceFactory::GetForProfile( profile, ServiceAccessType::EXPLICIT_ACCESS); + auto* host_content_settings_map = + HostContentSettingsMapFactory::GetForProfile(profile); return std::make_unique( profile->GetPrefs(), favicon_service, ads_service, history_service, - profile->GetURLLoaderFactory()); + profile->GetURLLoaderFactory(), + std::make_unique( + host_content_settings_map)); } bool BraveNewsControllerFactory::ServiceIsNULLWhileTesting() const { diff --git a/browser/brave_news/direct_feed_fetcher_delegate_impl.cc b/browser/brave_news/direct_feed_fetcher_delegate_impl.cc new file mode 100644 index 000000000000..64cbb76519e0 --- /dev/null +++ b/browser/brave_news/direct_feed_fetcher_delegate_impl.cc @@ -0,0 +1,32 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +#include "brave/browser/brave_news/direct_feed_fetcher_delegate_impl.h" + +#include "brave/browser/brave_browser_process.h" +#include "brave/components/brave_shields/content/browser/brave_shields_util.h" + +namespace brave_news { + +DirectFeedFetcherDelegateImpl::DirectFeedFetcherDelegateImpl( + HostContentSettingsMap* host_content_settings_map) + : host_content_settings_map_(host_content_settings_map), + https_upgrade_exceptions_service_( + g_brave_browser_process->https_upgrade_exceptions_service()) {} + +bool DirectFeedFetcherDelegateImpl::ShouldUpgradeToHttps(const GURL& url) { + if (!host_content_settings_map_ || !https_upgrade_exceptions_service_) { + return true; + } + return brave_shields::ShouldUpgradeToHttps(host_content_settings_map_, url, + https_upgrade_exceptions_service_); +} + +void DirectFeedFetcherDelegateImpl::Shutdown() { + host_content_settings_map_ = nullptr; + https_upgrade_exceptions_service_ = nullptr; +} + +} // namespace brave_news diff --git a/browser/brave_news/direct_feed_fetcher_delegate_impl.h b/browser/brave_news/direct_feed_fetcher_delegate_impl.h new file mode 100644 index 000000000000..a7fda987a884 --- /dev/null +++ b/browser/brave_news/direct_feed_fetcher_delegate_impl.h @@ -0,0 +1,41 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +#ifndef BRAVE_BROWSER_BRAVE_NEWS_DIRECT_FEED_FETCHER_DELEGATE_IMPL_H_ +#define BRAVE_BROWSER_BRAVE_NEWS_DIRECT_FEED_FETCHER_DELEGATE_IMPL_H_ + +#include "brave/components/brave_news/browser/direct_feed_fetcher.h" +#include "url/gurl.h" + +namespace https_upgrade_exceptions { +class HttpsUpgradeExceptionsService; +} // namespace https_upgrade_exceptions + +class HostContentSettingsMap; + +namespace brave_news { + +class DirectFeedFetcherDelegateImpl : public DirectFeedFetcher::Delegate { + public: + explicit DirectFeedFetcherDelegateImpl( + HostContentSettingsMap* host_content_settings_map); + ~DirectFeedFetcherDelegateImpl() override = default; + + DirectFeedFetcherDelegateImpl(const DirectFeedFetcherDelegateImpl&) = delete; + DirectFeedFetcherDelegateImpl& operator=( + const DirectFeedFetcherDelegateImpl&) = delete; + + bool ShouldUpgradeToHttps(const GURL& url) override; + void Shutdown() override; + + private: + raw_ptr host_content_settings_map_; + raw_ptr + https_upgrade_exceptions_service_; +}; + +} // namespace brave_news + +#endif // BRAVE_BROWSER_BRAVE_NEWS_DIRECT_FEED_FETCHER_DELEGATE_IMPL_H_ diff --git a/browser/brave_news/sources.gni b/browser/brave_news/sources.gni index a7350e03430e..8b5e3af0d57a 100644 --- a/browser/brave_news/sources.gni +++ b/browser/brave_news/sources.gni @@ -14,12 +14,15 @@ brave_browser_brave_news_sources += [ "//brave/browser/brave_news/brave_news_controller_factory.h", "//brave/browser/brave_news/brave_news_tab_helper.cc", "//brave/browser/brave_news/brave_news_tab_helper.h", + "//brave/browser/brave_news/direct_feed_fetcher_delegate_impl.cc", + "//brave/browser/brave_news/direct_feed_fetcher_delegate_impl.h", ] brave_browser_brave_news_deps += [ "//base", "//brave/components/brave_news/browser", "//brave/components/brave_news/common", + "//brave/components/brave_shields/content/browser", "//chrome/browser/profiles:profile", "//components/keyed_service/content", "//content/public/browser", diff --git a/components/brave_news/browser/brave_news_controller.cc b/components/brave_news/browser/brave_news_controller.cc index e74d9b7aecfb..f8cca44fbb6a 100644 --- a/components/brave_news/browser/brave_news_controller.cc +++ b/components/brave_news/browser/brave_news_controller.cc @@ -108,7 +108,8 @@ BraveNewsController::BraveNewsController( favicon::FaviconService* favicon_service, brave_ads::AdsService* ads_service, history::HistoryService* history_service, - scoped_refptr url_loader_factory) + scoped_refptr url_loader_factory, + std::unique_ptr direct_feed_fetcher_delegate) : favicon_service_(favicon_service), ads_service_(ads_service), api_request_helper_(GetNetworkTrafficAnnotationTag(), url_loader_factory), @@ -116,12 +117,15 @@ BraveNewsController::BraveNewsController( url_loader_factory), history_service_(history_service), url_loader_factory_(url_loader_factory), - pref_manager_(*prefs), - news_metrics_(prefs, pref_manager_), - direct_feed_controller_(url_loader_factory), task_runner_(base::ThreadPool::CreateSingleThreadTaskRunner( {base::MayBlock(), base::TaskPriority::BEST_EFFORT, base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})), + direct_feed_fetcher_delegate_(direct_feed_fetcher_delegate.release(), + base::OnTaskRunnerDeleter(task_runner_)), + pref_manager_(*prefs), + news_metrics_(prefs, pref_manager_), + direct_feed_controller_(url_loader_factory, + direct_feed_fetcher_delegate_.get()), engine_(nullptr, base::OnTaskRunnerDeleter(task_runner_)), initialization_promise_( 3, @@ -143,6 +147,7 @@ BraveNewsController::BraveNewsController( } BraveNewsController::~BraveNewsController() { + direct_feed_fetcher_delegate_->Shutdown(); net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this); } @@ -795,8 +800,9 @@ void BraveNewsController::Prefetch() { } void BraveNewsController::ResetEngine() { - engine_.reset( - new BraveNewsEngine(url_loader_factory_->Clone(), MakeHistoryQuerier())); + engine_.reset(new BraveNewsEngine(url_loader_factory_->Clone(), + MakeHistoryQuerier(), + direct_feed_fetcher_delegate_.get())); } void BraveNewsController::ConditionallyStartOrStopTimer() { diff --git a/components/brave_news/browser/brave_news_controller.h b/components/brave_news/browser/brave_news_controller.h index aaaf68ae4a49..29682a87fe9c 100644 --- a/components/brave_news/browser/brave_news_controller.h +++ b/components/brave_news/browser/brave_news_controller.h @@ -68,7 +68,9 @@ class BraveNewsController favicon::FaviconService* favicon_service, brave_ads::AdsService* ads_service, history::HistoryService* history_service, - scoped_refptr url_loader_factory); + scoped_refptr url_loader_factory, + std::unique_ptr + direct_feed_fetcher_delegate); ~BraveNewsController() override; BraveNewsController(const BraveNewsController&) = delete; BraveNewsController& operator=(const BraveNewsController&) = delete; @@ -183,6 +185,10 @@ class BraveNewsController raw_ptr history_service_; scoped_refptr url_loader_factory_; + scoped_refptr task_runner_; + std::unique_ptr + direct_feed_fetcher_delegate_; + BraveNewsPrefManager pref_manager_; SubscriptionsSnapshot last_subscriptions_; @@ -190,7 +196,6 @@ class BraveNewsController DirectFeedController direct_feed_controller_; - scoped_refptr task_runner_; // Created on this sequence but lives on |task_runner_|. std::unique_ptr engine_; diff --git a/components/brave_news/browser/brave_news_engine.cc b/components/brave_news/browser/brave_news_engine.cc index 2e2655e6a43c..e1ebf8d07fea 100644 --- a/components/brave_news/browser/brave_news_engine.cc +++ b/components/brave_news/browser/brave_news_engine.cc @@ -32,10 +32,12 @@ namespace brave_news { BraveNewsEngine::BraveNewsEngine( std::unique_ptr pending_shared_url_loader_factory, - BackgroundHistoryQuerier history_querier) + BackgroundHistoryQuerier history_querier, + DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate) : pending_shared_url_loader_factory_( std::move(pending_shared_url_loader_factory)), - history_querier_(std::move(history_querier)) { + history_querier_(std::move(history_querier)), + direct_feed_fetcher_delegate_(direct_feed_fetcher_delegate) { DETACH_FROM_SEQUENCE(sequence_checker_); } @@ -187,7 +189,7 @@ FeedV2Builder* BraveNewsEngine::MaybeFeedV2Builder() { feed_v2_builder_ = std::make_unique( *GetPublishersController(), *GetChannelsController(), *GetSuggestionsController(), history_querier_, - GetSharedURLLoaderFactory()); + GetSharedURLLoaderFactory(), direct_feed_fetcher_delegate_); } return feed_v2_builder_.get(); @@ -202,7 +204,7 @@ FeedController* BraveNewsEngine::MaybeFeedV1Builder() { if (!feed_controller_) { feed_controller_ = std::make_unique( GetPublishersController(), history_querier_, - GetSharedURLLoaderFactory()); + GetSharedURLLoaderFactory(), direct_feed_fetcher_delegate_); } return feed_controller_.get(); diff --git a/components/brave_news/browser/brave_news_engine.h b/components/brave_news/browser/brave_news_engine.h index 5912fcf5178c..2e6f9d7e4b3a 100644 --- a/components/brave_news/browser/brave_news_engine.h +++ b/components/brave_news/browser/brave_news_engine.h @@ -15,6 +15,7 @@ #include "base/sequence_checker.h" #include "base/thread_annotations.h" #include "brave/components/brave_news/browser/background_history_querier.h" +#include "brave/components/brave_news/browser/direct_feed_fetcher.h" #include "brave/components/brave_news/common/brave_news.mojom.h" namespace api_request_helper { @@ -49,7 +50,8 @@ class BraveNewsEngine { explicit BraveNewsEngine( std::unique_ptr pending_shared_url_loader_factory, - BackgroundHistoryQuerier history_querier); + BackgroundHistoryQuerier history_querier, + DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate); BraveNewsEngine(const BraveNewsEngine&) = delete; BraveNewsEngine& operator=(const BraveNewsEngine&) = delete; ~BraveNewsEngine(); @@ -105,6 +107,8 @@ class BraveNewsEngine { BackgroundHistoryQuerier history_querier_ GUARDED_BY_CONTEXT(sequence_checker_); + raw_ptr direct_feed_fetcher_delegate_; + std::unique_ptr api_request_helper_ GUARDED_BY_CONTEXT(sequence_checker_); diff --git a/components/brave_news/browser/direct_feed_controller.cc b/components/brave_news/browser/direct_feed_controller.cc index ec02a9f6b075..b1688f3cd4ec 100644 --- a/components/brave_news/browser/direct_feed_controller.cc +++ b/components/brave_news/browser/direct_feed_controller.cc @@ -36,8 +36,9 @@ DirectFeedController::FindFeedRequest::operator=( DirectFeedController::FindFeedRequest::~FindFeedRequest() = default; DirectFeedController::DirectFeedController( - scoped_refptr url_loader_factory) - : fetcher_(url_loader_factory) {} + scoped_refptr url_loader_factory, + DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate) + : fetcher_(url_loader_factory, direct_feed_fetcher_delegate) {} DirectFeedController::~DirectFeedController() = default; @@ -70,7 +71,7 @@ void DirectFeedController::FindFeedsImpl( const GURL& possible_feed_or_site_url) { DVLOG(2) << __FUNCTION__ << " " << possible_feed_or_site_url.spec(); fetcher_.DownloadFeed( - possible_feed_or_site_url, "", + possible_feed_or_site_url, "", false, base::BindOnce(&DirectFeedController::OnFindFeedsImplDownloadedFeed, weak_ptr_factory_.GetWeakPtr(), possible_feed_or_site_url)); @@ -122,7 +123,7 @@ void DirectFeedController::OnFindFeedsImplDownloadedFeed( auto feed_handler = base::BarrierCallback( feed_urls.size(), std::move(all_done_handler)); for (auto& url : feed_urls) { - fetcher_.DownloadFeed(url, "", feed_handler); + fetcher_.DownloadFeed(url, "", false, feed_handler); } return; } @@ -178,7 +179,7 @@ void DirectFeedController::VerifyFeedUrl(const GURL& feed_url, // will likely add to their user feed sources. Unless this is already // cached via network service? fetcher_.DownloadFeed( - feed_url, "", + feed_url, "", false, base::BindOnce( [](IsValidCallback callback, DirectFeedResponse response) { // Handle response diff --git a/components/brave_news/browser/direct_feed_controller.h b/components/brave_news/browser/direct_feed_controller.h index 41c03a10e3bc..c90313325a4f 100644 --- a/components/brave_news/browser/direct_feed_controller.h +++ b/components/brave_news/browser/direct_feed_controller.h @@ -29,7 +29,8 @@ using IsValidCallback = class DirectFeedController { public: explicit DirectFeedController( - scoped_refptr url_loader_factory); + scoped_refptr url_loader_factory, + DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate); ~DirectFeedController(); DirectFeedController(const DirectFeedController&) = delete; DirectFeedController& operator=(const DirectFeedController&) = delete; diff --git a/components/brave_news/browser/direct_feed_controller_unittest.cc b/components/brave_news/browser/direct_feed_controller_unittest.cc index 7d6c72b9b949..7b6236dd89a8 100644 --- a/components/brave_news/browser/direct_feed_controller_unittest.cc +++ b/components/brave_news/browser/direct_feed_controller_unittest.cc @@ -78,10 +78,21 @@ constexpr char kFeedURL[] = "https://example.com/feed"; class BraveNewsDirectFeedControllerTest : public testing::Test { public: BraveNewsDirectFeedControllerTest() - : direct_feed_controller_(test_url_loader_factory_.GetSafeWeakWrapper()) { - } + : direct_feed_controller_( + + test_url_loader_factory_.GetSafeWeakWrapper(), + &direct_feed_fetcher_delegate_) {} protected: + class MockDirectFeedFetcherDelegate : public DirectFeedFetcher::Delegate { + public: + ~MockDirectFeedFetcherDelegate() override = default; + + bool ShouldUpgradeToHttps(const GURL& url) override { return true; } + + void Shutdown() override {} + }; + std::tuple VerifyFeedUrl(GURL feed_url) { return WaitForCallback(base::BindOnce( &DirectFeedController::VerifyFeedUrl, @@ -99,6 +110,7 @@ class BraveNewsDirectFeedControllerTest : public testing::Test { content::BrowserTaskEnvironment task_environment_; data_decoder::test::InProcessDataDecoder data_decoder_; network::TestURLLoaderFactory test_url_loader_factory_; + MockDirectFeedFetcherDelegate direct_feed_fetcher_delegate_; DirectFeedController direct_feed_controller_; }; @@ -141,6 +153,17 @@ TEST_F(BraveNewsDirectFeedControllerTest, CanFindFeedFromFeedURL) { EXPECT_EQ("Hacker News", feed->feed_title); } +TEST_F(BraveNewsDirectFeedControllerTest, CanUpgradeToHTTPS) { + // Find an RSS feed + test_url_loader_factory_.AddResponse(kFeedURL, GetBasicFeed()); + auto result = FindFeeds(GURL("http://example.com/feed")); + + ASSERT_EQ(1u, result.size()); + auto feed = std::move(result.at(0)); + EXPECT_EQ("http://example.com/feed", feed->feed_url.spec()); + EXPECT_EQ("Hacker News", feed->feed_title); +} + TEST_F(BraveNewsDirectFeedControllerTest, CanFindFeedFromPageWithFeedURL) { // Fetch a page with an RSS feed test_url_loader_factory_.AddResponse(kPageUrl, GetHtmlPageWithFeed()); diff --git a/components/brave_news/browser/direct_feed_fetcher.cc b/components/brave_news/browser/direct_feed_fetcher.cc index ec724adf9db7..e0342ad0724c 100644 --- a/components/brave_news/browser/direct_feed_fetcher.cc +++ b/components/brave_news/browser/direct_feed_fetcher.cc @@ -22,6 +22,8 @@ #include "services/network/public/mojom/url_response_head.mojom.h" #include "third_party/abseil-cpp/absl/types/variant.h" #include "ui/base/l10n/time_format.h" +#include "url/gurl.h" +#include "url/url_constants.h" namespace brave_news { @@ -143,15 +145,27 @@ DirectFeedResponse::~DirectFeedResponse() = default; DirectFeedResponse::DirectFeedResponse(DirectFeedResponse&&) = default; DirectFeedFetcher::DirectFeedFetcher( - scoped_refptr url_loader_factory) - : url_loader_factory_(url_loader_factory) {} + scoped_refptr url_loader_factory, + Delegate* delegate) + : url_loader_factory_(url_loader_factory), delegate_(delegate) {} DirectFeedFetcher::~DirectFeedFetcher() = default; -void DirectFeedFetcher::DownloadFeed(const GURL& url, +void DirectFeedFetcher::DownloadFeed(GURL url, std::string publisher_id, + bool no_https_upgrade, DownloadFeedCallback callback) { // Make request auto request = std::make_unique(); + bool https_upgraded = false; + + if (!no_https_upgrade && url.SchemeIs(url::kHttpScheme) && + delegate_->ShouldUpgradeToHttps(url)) { + GURL::Replacements replacements; + replacements.SetSchemeStr(url::kHttpsScheme); + url = url.ReplaceComponents(replacements); + https_upgraded = true; + } + request->url = url; request->load_flags = net::LOAD_DO_NOT_SAVE_COOKIES; request->credentials_mode = network::mojom::CredentialsMode::kOmit; @@ -164,20 +178,22 @@ void DirectFeedFetcher::DownloadFeed(const GURL& url, url_loader->SetTimeoutDuration(GetDefaultRequestTimeout()); url_loader->SetAllowHttpErrorResults(true); auto iter = url_loaders_.insert(url_loaders_.begin(), std::move(url_loader)); + iter->get()->DownloadToString( url_loader_factory_.get(), // Handle response base::BindOnce(&DirectFeedFetcher::OnFeedDownloaded, weak_ptr_factory_.GetWeakPtr(), iter, std::move(callback), - url, std::move(publisher_id)), + url, std::move(publisher_id), https_upgraded), 5 * 1024 * 1024); } void DirectFeedFetcher::OnFeedDownloaded( SimpleURLLoaderList::iterator iter, DownloadFeedCallback callback, - const GURL& feed_url, + GURL feed_url, std::string publisher_id, + bool https_upgraded, std::unique_ptr response_body) { auto* loader = iter->get(); auto response_code = -1; @@ -201,6 +217,13 @@ void DirectFeedFetcher::OnFeedDownloaded( if (response_code < 200 || response_code >= 300 || body_content.empty()) { VLOG(1) << feed_url.spec() << " invalid response, state: " << response_code; + if (https_upgraded) { + GURL::Replacements replacements; + replacements.SetSchemeStr(url::kHttpScheme); + feed_url = feed_url.ReplaceComponents(replacements); + DownloadFeed(feed_url, publisher_id, true, std::move(callback)); + return; + } DirectFeedError error; error.body_content = std::move(body_content); result.result = std::move(error); diff --git a/components/brave_news/browser/direct_feed_fetcher.h b/components/brave_news/browser/direct_feed_fetcher.h index 7e5780aa3fae..b0aee3562f8e 100644 --- a/components/brave_news/browser/direct_feed_fetcher.h +++ b/components/brave_news/browser/direct_feed_fetcher.h @@ -67,16 +67,26 @@ void ConvertFeedDataToArticles(std::vector& articles, class DirectFeedFetcher { public: + class Delegate { + public: + virtual ~Delegate() = default; + + virtual bool ShouldUpgradeToHttps(const GURL& url) = 0; + virtual void Shutdown(); + }; + explicit DirectFeedFetcher( - scoped_refptr url_loader_factory); + scoped_refptr url_loader_factory, + Delegate* delegate); DirectFeedFetcher(const DirectFeedFetcher&) = delete; DirectFeedFetcher& operator=(const DirectFeedFetcher&) = delete; ~DirectFeedFetcher(); // |publisher_id| can be empty, if one we're speculatively downloading a feed. // This |publisher_id| will be used for any returned articles. - void DownloadFeed(const GURL& url, + void DownloadFeed(GURL url, std::string publisher_id, + bool no_https_upgrade, DownloadFeedCallback callback); private: @@ -84,8 +94,9 @@ class DirectFeedFetcher { std::list>; void OnFeedDownloaded(SimpleURLLoaderList::iterator iter, DownloadFeedCallback callback, - const GURL& feed_url, + GURL feed_url, std::string publisher_id, + bool https_upgraded, const std::unique_ptr response_body); void OnParsedFeedData(DownloadFeedCallback callback, DirectFeedResponse result, @@ -94,6 +105,9 @@ class DirectFeedFetcher { SimpleURLLoaderList url_loaders_; scoped_refptr url_loader_factory_; + + raw_ptr delegate_; + base::WeakPtrFactory weak_ptr_factory_{this}; }; diff --git a/components/brave_news/browser/feed_controller.cc b/components/brave_news/browser/feed_controller.cc index 393df43950c7..29c7fc282688 100644 --- a/components/brave_news/browser/feed_controller.cc +++ b/components/brave_news/browser/feed_controller.cc @@ -34,10 +34,13 @@ namespace brave_news { FeedController::FeedController( PublishersController* publishers_controller, BackgroundHistoryQuerier& history_querier, - scoped_refptr url_loader_factory) + scoped_refptr url_loader_factory, + DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate) : publishers_controller_(publishers_controller), history_querier_(history_querier), - feed_fetcher_(*publishers_controller, url_loader_factory), + feed_fetcher_(*publishers_controller, + url_loader_factory, + direct_feed_fetcher_delegate), on_current_update_complete_(new base::OneShotEvent()) {} FeedController::~FeedController() = default; diff --git a/components/brave_news/browser/feed_controller.h b/components/brave_news/browser/feed_controller.h index 38f719919611..983411578b0a 100644 --- a/components/brave_news/browser/feed_controller.h +++ b/components/brave_news/browser/feed_controller.h @@ -37,7 +37,8 @@ class FeedController { FeedController( PublishersController* publishers_controller, BackgroundHistoryQuerier& history_querier, - scoped_refptr url_loader_factory); + scoped_refptr url_loader_factory, + DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate); ~FeedController(); FeedController(const FeedController&) = delete; FeedController& operator=(const FeedController&) = delete; diff --git a/components/brave_news/browser/feed_fetcher.cc b/components/brave_news/browser/feed_fetcher.cc index 7501b40ecd60..54d4866619c1 100644 --- a/components/brave_news/browser/feed_fetcher.cc +++ b/components/brave_news/browser/feed_fetcher.cc @@ -105,10 +105,11 @@ std::tuple FeedFetcher::CombineFeedSourceResults( FeedFetcher::FeedFetcher( PublishersController& publishers_controller, - scoped_refptr url_loader_factory) + scoped_refptr url_loader_factory, + DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate) : publishers_controller_(publishers_controller), api_request_helper_(GetNetworkTrafficAnnotationTag(), url_loader_factory), - direct_feed_fetcher_(url_loader_factory) {} + direct_feed_fetcher_(url_loader_factory, direct_feed_fetcher_delegate) {} FeedFetcher::~FeedFetcher() = default; @@ -161,7 +162,7 @@ void FeedFetcher::OnFetchFeedFetchedPublishers( for (const auto& direct_publisher : direct_publishers) { direct_feed_fetcher_.DownloadFeed( - direct_publisher->feed_source, direct_publisher->publisher_id, + direct_publisher->feed_source, direct_publisher->publisher_id, false, base::BindOnce( [](base::RepeatingCallback cb, std::string publisher_id, DirectFeedResponse response) { diff --git a/components/brave_news/browser/feed_fetcher.h b/components/brave_news/browser/feed_fetcher.h index a3c07ada6395..e4a566e6b66c 100644 --- a/components/brave_news/browser/feed_fetcher.h +++ b/components/brave_news/browser/feed_fetcher.h @@ -28,9 +28,9 @@ class SubscriptionsSnapshot; class FeedFetcher { public: - FeedFetcher( - PublishersController& publishers_controller, - scoped_refptr url_loader_factory); + FeedFetcher(PublishersController& publishers_controller, + scoped_refptr url_loader_factory, + DirectFeedFetcher::Delegate* delegate); ~FeedFetcher(); FeedFetcher(const FeedFetcher&) = delete; FeedFetcher& operator=(const FeedFetcher&) = delete; diff --git a/components/brave_news/browser/feed_v2_builder.cc b/components/brave_news/browser/feed_v2_builder.cc index 17bd59f93e9e..1a99e2d29d13 100644 --- a/components/brave_news/browser/feed_v2_builder.cc +++ b/components/brave_news/browser/feed_v2_builder.cc @@ -613,11 +613,14 @@ FeedV2Builder::FeedV2Builder( ChannelsController& channels_controller, SuggestionsController& suggestions_controller, BackgroundHistoryQuerier& history_querier, - scoped_refptr url_loader_factory) + scoped_refptr url_loader_factory, + DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate) : publishers_controller_(publishers_controller), channels_controller_(channels_controller), suggestions_controller_(suggestions_controller), - fetcher_(publishers_controller, url_loader_factory), + fetcher_(publishers_controller, + url_loader_factory, + direct_feed_fetcher_delegate), topics_fetcher_(url_loader_factory), signal_calculator_(publishers_controller, channels_controller, diff --git a/components/brave_news/browser/feed_v2_builder.h b/components/brave_news/browser/feed_v2_builder.h index fd295f616f8d..64b0aab54b2b 100644 --- a/components/brave_news/browser/feed_v2_builder.h +++ b/components/brave_news/browser/feed_v2_builder.h @@ -44,7 +44,8 @@ class FeedV2Builder { ChannelsController& channels_controller, SuggestionsController& suggestions_controller, BackgroundHistoryQuerier& history_querier, - scoped_refptr url_loader_factory); + scoped_refptr url_loader_factory, + DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate); FeedV2Builder(const FeedV2Builder&) = delete; FeedV2Builder& operator=(const FeedV2Builder&) = delete; ~FeedV2Builder(); From 9f3d6a8523608491cfa7238dd0cc9bf9596decb4 Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Fri, 6 Dec 2024 12:05:38 -0800 Subject: [PATCH 2/3] Call News fetcher delegate on UI thread --- .../direct_feed_fetcher_delegate_impl.cc | 3 +++ .../direct_feed_fetcher_delegate_impl.h | 1 + .../browser/direct_feed_controller.cc | 6 ++--- .../brave_news/browser/direct_feed_fetcher.cc | 24 +++++++++++++++---- .../brave_news/browser/direct_feed_fetcher.h | 6 ++++- components/brave_news/browser/feed_fetcher.cc | 2 +- 6 files changed, 33 insertions(+), 9 deletions(-) diff --git a/browser/brave_news/direct_feed_fetcher_delegate_impl.cc b/browser/brave_news/direct_feed_fetcher_delegate_impl.cc index 64cbb76519e0..160180aa09cc 100644 --- a/browser/brave_news/direct_feed_fetcher_delegate_impl.cc +++ b/browser/brave_news/direct_feed_fetcher_delegate_impl.cc @@ -7,6 +7,7 @@ #include "brave/browser/brave_browser_process.h" #include "brave/components/brave_shields/content/browser/brave_shields_util.h" +#include "content/public/browser/browser_thread.h" namespace brave_news { @@ -17,6 +18,7 @@ DirectFeedFetcherDelegateImpl::DirectFeedFetcherDelegateImpl( g_brave_browser_process->https_upgrade_exceptions_service()) {} bool DirectFeedFetcherDelegateImpl::ShouldUpgradeToHttps(const GURL& url) { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); if (!host_content_settings_map_ || !https_upgrade_exceptions_service_) { return true; } @@ -25,6 +27,7 @@ bool DirectFeedFetcherDelegateImpl::ShouldUpgradeToHttps(const GURL& url) { } void DirectFeedFetcherDelegateImpl::Shutdown() { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); host_content_settings_map_ = nullptr; https_upgrade_exceptions_service_ = nullptr; } diff --git a/browser/brave_news/direct_feed_fetcher_delegate_impl.h b/browser/brave_news/direct_feed_fetcher_delegate_impl.h index a7fda987a884..d72daf8f6329 100644 --- a/browser/brave_news/direct_feed_fetcher_delegate_impl.h +++ b/browser/brave_news/direct_feed_fetcher_delegate_impl.h @@ -27,6 +27,7 @@ class DirectFeedFetcherDelegateImpl : public DirectFeedFetcher::Delegate { DirectFeedFetcherDelegateImpl& operator=( const DirectFeedFetcherDelegateImpl&) = delete; + // Must be called on UI thread bool ShouldUpgradeToHttps(const GURL& url) override; void Shutdown() override; diff --git a/components/brave_news/browser/direct_feed_controller.cc b/components/brave_news/browser/direct_feed_controller.cc index b1688f3cd4ec..bfca6f8c695d 100644 --- a/components/brave_news/browser/direct_feed_controller.cc +++ b/components/brave_news/browser/direct_feed_controller.cc @@ -71,7 +71,7 @@ void DirectFeedController::FindFeedsImpl( const GURL& possible_feed_or_site_url) { DVLOG(2) << __FUNCTION__ << " " << possible_feed_or_site_url.spec(); fetcher_.DownloadFeed( - possible_feed_or_site_url, "", false, + possible_feed_or_site_url, "", base::BindOnce(&DirectFeedController::OnFindFeedsImplDownloadedFeed, weak_ptr_factory_.GetWeakPtr(), possible_feed_or_site_url)); @@ -123,7 +123,7 @@ void DirectFeedController::OnFindFeedsImplDownloadedFeed( auto feed_handler = base::BarrierCallback( feed_urls.size(), std::move(all_done_handler)); for (auto& url : feed_urls) { - fetcher_.DownloadFeed(url, "", false, feed_handler); + fetcher_.DownloadFeed(url, "", feed_handler); } return; } @@ -179,7 +179,7 @@ void DirectFeedController::VerifyFeedUrl(const GURL& feed_url, // will likely add to their user feed sources. Unless this is already // cached via network service? fetcher_.DownloadFeed( - feed_url, "", false, + feed_url, "", base::BindOnce( [](IsValidCallback callback, DirectFeedResponse response) { // Handle response diff --git a/components/brave_news/browser/direct_feed_fetcher.cc b/components/brave_news/browser/direct_feed_fetcher.cc index e0342ad0724c..1d1f4fb66e36 100644 --- a/components/brave_news/browser/direct_feed_fetcher.cc +++ b/components/brave_news/browser/direct_feed_fetcher.cc @@ -16,6 +16,7 @@ #include "brave/components/brave_news/browser/network.h" #include "brave/components/brave_news/common/brave_news.mojom.h" #include "brave/components/brave_news/rust/lib.rs.h" +#include "content/public/browser/browser_thread.h" #include "net/base/load_flags.h" #include "services/network/public/cpp/resource_request.h" #include "services/network/public/cpp/simple_url_loader.h" @@ -152,14 +153,29 @@ DirectFeedFetcher::~DirectFeedFetcher() = default; void DirectFeedFetcher::DownloadFeed(GURL url, std::string publisher_id, - bool no_https_upgrade, DownloadFeedCallback callback) { + if (url.SchemeIs(url::kHttpScheme)) { + content::GetUIThreadTaskRunner({})->PostTaskAndReplyWithResult( + FROM_HERE, + base::BindOnce(&Delegate::ShouldUpgradeToHttps, + base::Unretained(delegate_), url), + base::BindOnce(&DirectFeedFetcher::DownloadFeedHelper, + weak_ptr_factory_.GetWeakPtr(), url, publisher_id, + std::move(callback))); + return; + } + DownloadFeedHelper(url, publisher_id, std::move(callback), false); +} + +void DirectFeedFetcher::DownloadFeedHelper(GURL url, + std::string publisher_id, + DownloadFeedCallback callback, + bool should_https_upgrade) { // Make request auto request = std::make_unique(); bool https_upgraded = false; - if (!no_https_upgrade && url.SchemeIs(url::kHttpScheme) && - delegate_->ShouldUpgradeToHttps(url)) { + if (should_https_upgrade) { GURL::Replacements replacements; replacements.SetSchemeStr(url::kHttpsScheme); url = url.ReplaceComponents(replacements); @@ -221,7 +237,7 @@ void DirectFeedFetcher::OnFeedDownloaded( GURL::Replacements replacements; replacements.SetSchemeStr(url::kHttpScheme); feed_url = feed_url.ReplaceComponents(replacements); - DownloadFeed(feed_url, publisher_id, true, std::move(callback)); + DownloadFeedHelper(feed_url, publisher_id, std::move(callback), false); return; } DirectFeedError error; diff --git a/components/brave_news/browser/direct_feed_fetcher.h b/components/brave_news/browser/direct_feed_fetcher.h index b0aee3562f8e..a8a89b042876 100644 --- a/components/brave_news/browser/direct_feed_fetcher.h +++ b/components/brave_news/browser/direct_feed_fetcher.h @@ -86,12 +86,16 @@ class DirectFeedFetcher { // This |publisher_id| will be used for any returned articles. void DownloadFeed(GURL url, std::string publisher_id, - bool no_https_upgrade, DownloadFeedCallback callback); private: using SimpleURLLoaderList = std::list>; + + void DownloadFeedHelper(GURL url, + std::string publisher_id, + DownloadFeedCallback callback, + bool should_https_upgrade); void OnFeedDownloaded(SimpleURLLoaderList::iterator iter, DownloadFeedCallback callback, GURL feed_url, diff --git a/components/brave_news/browser/feed_fetcher.cc b/components/brave_news/browser/feed_fetcher.cc index 54d4866619c1..fa702a990c35 100644 --- a/components/brave_news/browser/feed_fetcher.cc +++ b/components/brave_news/browser/feed_fetcher.cc @@ -162,7 +162,7 @@ void FeedFetcher::OnFetchFeedFetchedPublishers( for (const auto& direct_publisher : direct_publishers) { direct_feed_fetcher_.DownloadFeed( - direct_publisher->feed_source, direct_publisher->publisher_id, false, + direct_publisher->feed_source, direct_publisher->publisher_id, base::BindOnce( [](base::RepeatingCallback cb, std::string publisher_id, DirectFeedResponse response) { From 071fe11d36dd100339a1bd56103f7a7351e77f1f Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Mon, 9 Dec 2024 16:23:36 -0800 Subject: [PATCH 3/3] Use `base::WeakPtr` for `DirectFeedFetcher::Delegate` call --- .../direct_feed_fetcher_delegate_impl.cc | 9 +++--- .../direct_feed_fetcher_delegate_impl.h | 7 +++-- .../browser/brave_news_controller.cc | 18 +++++------ .../browser/brave_news_controller.h | 5 ++-- .../brave_news/browser/brave_news_engine.cc | 2 +- .../brave_news/browser/brave_news_engine.h | 4 +-- .../browser/direct_feed_controller.cc | 2 +- .../browser/direct_feed_controller.h | 2 +- .../direct_feed_controller_unittest.cc | 13 ++++---- .../brave_news/browser/direct_feed_fetcher.cc | 30 ++++++++++++++----- .../brave_news/browser/direct_feed_fetcher.h | 6 ++-- .../brave_news/browser/feed_controller.cc | 2 +- .../brave_news/browser/feed_controller.h | 2 +- components/brave_news/browser/feed_fetcher.cc | 2 +- components/brave_news/browser/feed_fetcher.h | 2 +- .../brave_news/browser/feed_v2_builder.cc | 2 +- .../brave_news/browser/feed_v2_builder.h | 2 +- 17 files changed, 65 insertions(+), 45 deletions(-) diff --git a/browser/brave_news/direct_feed_fetcher_delegate_impl.cc b/browser/brave_news/direct_feed_fetcher_delegate_impl.cc index 160180aa09cc..22ce8cfeeaf5 100644 --- a/browser/brave_news/direct_feed_fetcher_delegate_impl.cc +++ b/browser/brave_news/direct_feed_fetcher_delegate_impl.cc @@ -17,6 +17,8 @@ DirectFeedFetcherDelegateImpl::DirectFeedFetcherDelegateImpl( https_upgrade_exceptions_service_( g_brave_browser_process->https_upgrade_exceptions_service()) {} +DirectFeedFetcherDelegateImpl::~DirectFeedFetcherDelegateImpl() = default; + bool DirectFeedFetcherDelegateImpl::ShouldUpgradeToHttps(const GURL& url) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); if (!host_content_settings_map_ || !https_upgrade_exceptions_service_) { @@ -26,10 +28,9 @@ bool DirectFeedFetcherDelegateImpl::ShouldUpgradeToHttps(const GURL& url) { https_upgrade_exceptions_service_); } -void DirectFeedFetcherDelegateImpl::Shutdown() { - DCHECK_CURRENTLY_ON(content::BrowserThread::UI); - host_content_settings_map_ = nullptr; - https_upgrade_exceptions_service_ = nullptr; +base::WeakPtr +DirectFeedFetcherDelegateImpl::AsWeakPtr() { + return weak_ptr_factory_.GetWeakPtr(); } } // namespace brave_news diff --git a/browser/brave_news/direct_feed_fetcher_delegate_impl.h b/browser/brave_news/direct_feed_fetcher_delegate_impl.h index d72daf8f6329..339dd4af5a55 100644 --- a/browser/brave_news/direct_feed_fetcher_delegate_impl.h +++ b/browser/brave_news/direct_feed_fetcher_delegate_impl.h @@ -21,7 +21,7 @@ class DirectFeedFetcherDelegateImpl : public DirectFeedFetcher::Delegate { public: explicit DirectFeedFetcherDelegateImpl( HostContentSettingsMap* host_content_settings_map); - ~DirectFeedFetcherDelegateImpl() override = default; + ~DirectFeedFetcherDelegateImpl() override; DirectFeedFetcherDelegateImpl(const DirectFeedFetcherDelegateImpl&) = delete; DirectFeedFetcherDelegateImpl& operator=( @@ -29,12 +29,15 @@ class DirectFeedFetcherDelegateImpl : public DirectFeedFetcher::Delegate { // Must be called on UI thread bool ShouldUpgradeToHttps(const GURL& url) override; - void Shutdown() override; + + base::WeakPtr AsWeakPtr() override; private: raw_ptr host_content_settings_map_; raw_ptr https_upgrade_exceptions_service_; + + base::WeakPtrFactory weak_ptr_factory_{this}; }; } // namespace brave_news diff --git a/components/brave_news/browser/brave_news_controller.cc b/components/brave_news/browser/brave_news_controller.cc index f8cca44fbb6a..082264f325de 100644 --- a/components/brave_news/browser/brave_news_controller.cc +++ b/components/brave_news/browser/brave_news_controller.cc @@ -117,15 +117,14 @@ BraveNewsController::BraveNewsController( url_loader_factory), history_service_(history_service), url_loader_factory_(url_loader_factory), - task_runner_(base::ThreadPool::CreateSingleThreadTaskRunner( - {base::MayBlock(), base::TaskPriority::BEST_EFFORT, - base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})), - direct_feed_fetcher_delegate_(direct_feed_fetcher_delegate.release(), - base::OnTaskRunnerDeleter(task_runner_)), + direct_feed_fetcher_delegate_(std::move(direct_feed_fetcher_delegate)), pref_manager_(*prefs), news_metrics_(prefs, pref_manager_), direct_feed_controller_(url_loader_factory, - direct_feed_fetcher_delegate_.get()), + direct_feed_fetcher_delegate_->AsWeakPtr()), + task_runner_(base::ThreadPool::CreateSingleThreadTaskRunner( + {base::MayBlock(), base::TaskPriority::BEST_EFFORT, + base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})), engine_(nullptr, base::OnTaskRunnerDeleter(task_runner_)), initialization_promise_( 3, @@ -147,7 +146,6 @@ BraveNewsController::BraveNewsController( } BraveNewsController::~BraveNewsController() { - direct_feed_fetcher_delegate_->Shutdown(); net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this); } @@ -800,9 +798,9 @@ void BraveNewsController::Prefetch() { } void BraveNewsController::ResetEngine() { - engine_.reset(new BraveNewsEngine(url_loader_factory_->Clone(), - MakeHistoryQuerier(), - direct_feed_fetcher_delegate_.get())); + engine_.reset( + new BraveNewsEngine(url_loader_factory_->Clone(), MakeHistoryQuerier(), + direct_feed_fetcher_delegate_->AsWeakPtr())); } void BraveNewsController::ConditionallyStartOrStopTimer() { diff --git a/components/brave_news/browser/brave_news_controller.h b/components/brave_news/browser/brave_news_controller.h index 29682a87fe9c..bfda58fae042 100644 --- a/components/brave_news/browser/brave_news_controller.h +++ b/components/brave_news/browser/brave_news_controller.h @@ -185,9 +185,7 @@ class BraveNewsController raw_ptr history_service_; scoped_refptr url_loader_factory_; - scoped_refptr task_runner_; - std::unique_ptr - direct_feed_fetcher_delegate_; + std::unique_ptr direct_feed_fetcher_delegate_; BraveNewsPrefManager pref_manager_; SubscriptionsSnapshot last_subscriptions_; @@ -196,6 +194,7 @@ class BraveNewsController DirectFeedController direct_feed_controller_; + scoped_refptr task_runner_; // Created on this sequence but lives on |task_runner_|. std::unique_ptr engine_; diff --git a/components/brave_news/browser/brave_news_engine.cc b/components/brave_news/browser/brave_news_engine.cc index e1ebf8d07fea..c513f29c8438 100644 --- a/components/brave_news/browser/brave_news_engine.cc +++ b/components/brave_news/browser/brave_news_engine.cc @@ -33,7 +33,7 @@ BraveNewsEngine::BraveNewsEngine( std::unique_ptr pending_shared_url_loader_factory, BackgroundHistoryQuerier history_querier, - DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate) + base::WeakPtr direct_feed_fetcher_delegate) : pending_shared_url_loader_factory_( std::move(pending_shared_url_loader_factory)), history_querier_(std::move(history_querier)), diff --git a/components/brave_news/browser/brave_news_engine.h b/components/brave_news/browser/brave_news_engine.h index 2e6f9d7e4b3a..dff7d35bbd67 100644 --- a/components/brave_news/browser/brave_news_engine.h +++ b/components/brave_news/browser/brave_news_engine.h @@ -51,7 +51,7 @@ class BraveNewsEngine { std::unique_ptr pending_shared_url_loader_factory, BackgroundHistoryQuerier history_querier, - DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate); + base::WeakPtr direct_feed_fetcher_delegate); BraveNewsEngine(const BraveNewsEngine&) = delete; BraveNewsEngine& operator=(const BraveNewsEngine&) = delete; ~BraveNewsEngine(); @@ -107,7 +107,7 @@ class BraveNewsEngine { BackgroundHistoryQuerier history_querier_ GUARDED_BY_CONTEXT(sequence_checker_); - raw_ptr direct_feed_fetcher_delegate_; + base::WeakPtr direct_feed_fetcher_delegate_; std::unique_ptr api_request_helper_ GUARDED_BY_CONTEXT(sequence_checker_); diff --git a/components/brave_news/browser/direct_feed_controller.cc b/components/brave_news/browser/direct_feed_controller.cc index bfca6f8c695d..d8081ca4c620 100644 --- a/components/brave_news/browser/direct_feed_controller.cc +++ b/components/brave_news/browser/direct_feed_controller.cc @@ -37,7 +37,7 @@ DirectFeedController::FindFeedRequest::~FindFeedRequest() = default; DirectFeedController::DirectFeedController( scoped_refptr url_loader_factory, - DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate) + base::WeakPtr direct_feed_fetcher_delegate) : fetcher_(url_loader_factory, direct_feed_fetcher_delegate) {} DirectFeedController::~DirectFeedController() = default; diff --git a/components/brave_news/browser/direct_feed_controller.h b/components/brave_news/browser/direct_feed_controller.h index c90313325a4f..430fafa63ea6 100644 --- a/components/brave_news/browser/direct_feed_controller.h +++ b/components/brave_news/browser/direct_feed_controller.h @@ -30,7 +30,7 @@ class DirectFeedController { public: explicit DirectFeedController( scoped_refptr url_loader_factory, - DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate); + base::WeakPtr direct_feed_fetcher_delegate); ~DirectFeedController(); DirectFeedController(const DirectFeedController&) = delete; DirectFeedController& operator=(const DirectFeedController&) = delete; diff --git a/components/brave_news/browser/direct_feed_controller_unittest.cc b/components/brave_news/browser/direct_feed_controller_unittest.cc index 7b6236dd89a8..04f49b0651ed 100644 --- a/components/brave_news/browser/direct_feed_controller_unittest.cc +++ b/components/brave_news/browser/direct_feed_controller_unittest.cc @@ -78,10 +78,8 @@ constexpr char kFeedURL[] = "https://example.com/feed"; class BraveNewsDirectFeedControllerTest : public testing::Test { public: BraveNewsDirectFeedControllerTest() - : direct_feed_controller_( - - test_url_loader_factory_.GetSafeWeakWrapper(), - &direct_feed_fetcher_delegate_) {} + : direct_feed_controller_(test_url_loader_factory_.GetSafeWeakWrapper(), + direct_feed_fetcher_delegate_.AsWeakPtr()) {} protected: class MockDirectFeedFetcherDelegate : public DirectFeedFetcher::Delegate { @@ -90,7 +88,12 @@ class BraveNewsDirectFeedControllerTest : public testing::Test { bool ShouldUpgradeToHttps(const GURL& url) override { return true; } - void Shutdown() override {} + base::WeakPtr AsWeakPtr() override { + return weak_ptr_factory_.GetWeakPtr(); + } + + private: + base::WeakPtrFactory weak_ptr_factory_{this}; }; std::tuple VerifyFeedUrl(GURL feed_url) { diff --git a/components/brave_news/browser/direct_feed_fetcher.cc b/components/brave_news/browser/direct_feed_fetcher.cc index 1d1f4fb66e36..d3e7a7c0b1b3 100644 --- a/components/brave_news/browser/direct_feed_fetcher.cc +++ b/components/brave_news/browser/direct_feed_fetcher.cc @@ -147,7 +147,7 @@ DirectFeedResponse::DirectFeedResponse(DirectFeedResponse&&) = default; DirectFeedFetcher::DirectFeedFetcher( scoped_refptr url_loader_factory, - Delegate* delegate) + base::WeakPtr delegate) : url_loader_factory_(url_loader_factory), delegate_(delegate) {} DirectFeedFetcher::~DirectFeedFetcher() = default; @@ -155,13 +155,29 @@ void DirectFeedFetcher::DownloadFeed(GURL url, std::string publisher_id, DownloadFeedCallback callback) { if (url.SchemeIs(url::kHttpScheme)) { - content::GetUIThreadTaskRunner({})->PostTaskAndReplyWithResult( + content::GetUIThreadTaskRunner({})->PostTask( FROM_HERE, - base::BindOnce(&Delegate::ShouldUpgradeToHttps, - base::Unretained(delegate_), url), - base::BindOnce(&DirectFeedFetcher::DownloadFeedHelper, - weak_ptr_factory_.GetWeakPtr(), url, publisher_id, - std::move(callback))); + base::BindOnce( + [](base::WeakPtr delegate, + scoped_refptr source_task_runner, + base::OnceCallback callback, GURL url) { + if (delegate.WasInvalidated()) { + return; + } + bool should_upgrade = delegate->ShouldUpgradeToHttps(url); + source_task_runner->PostTask( + FROM_HERE, + base::BindOnce( + [](base::OnceCallback callback, bool result) { + std::move(callback).Run(result); + }, + std::move(callback), should_upgrade)); + }, + delegate_, base::SingleThreadTaskRunner::GetCurrentDefault(), + base::BindOnce(&DirectFeedFetcher::DownloadFeedHelper, + weak_ptr_factory_.GetWeakPtr(), url, publisher_id, + std::move(callback)), + url)); return; } DownloadFeedHelper(url, publisher_id, std::move(callback), false); diff --git a/components/brave_news/browser/direct_feed_fetcher.h b/components/brave_news/browser/direct_feed_fetcher.h index a8a89b042876..5b24e7620f51 100644 --- a/components/brave_news/browser/direct_feed_fetcher.h +++ b/components/brave_news/browser/direct_feed_fetcher.h @@ -72,12 +72,12 @@ class DirectFeedFetcher { virtual ~Delegate() = default; virtual bool ShouldUpgradeToHttps(const GURL& url) = 0; - virtual void Shutdown(); + virtual base::WeakPtr AsWeakPtr() = 0; }; explicit DirectFeedFetcher( scoped_refptr url_loader_factory, - Delegate* delegate); + base::WeakPtr delegate); DirectFeedFetcher(const DirectFeedFetcher&) = delete; DirectFeedFetcher& operator=(const DirectFeedFetcher&) = delete; ~DirectFeedFetcher(); @@ -110,7 +110,7 @@ class DirectFeedFetcher { scoped_refptr url_loader_factory_; - raw_ptr delegate_; + base::WeakPtr delegate_; base::WeakPtrFactory weak_ptr_factory_{this}; }; diff --git a/components/brave_news/browser/feed_controller.cc b/components/brave_news/browser/feed_controller.cc index 29c7fc282688..56d1b662a6c8 100644 --- a/components/brave_news/browser/feed_controller.cc +++ b/components/brave_news/browser/feed_controller.cc @@ -35,7 +35,7 @@ FeedController::FeedController( PublishersController* publishers_controller, BackgroundHistoryQuerier& history_querier, scoped_refptr url_loader_factory, - DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate) + base::WeakPtr direct_feed_fetcher_delegate) : publishers_controller_(publishers_controller), history_querier_(history_querier), feed_fetcher_(*publishers_controller, diff --git a/components/brave_news/browser/feed_controller.h b/components/brave_news/browser/feed_controller.h index 983411578b0a..5322912f60c2 100644 --- a/components/brave_news/browser/feed_controller.h +++ b/components/brave_news/browser/feed_controller.h @@ -38,7 +38,7 @@ class FeedController { PublishersController* publishers_controller, BackgroundHistoryQuerier& history_querier, scoped_refptr url_loader_factory, - DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate); + base::WeakPtr direct_feed_fetcher_delegate); ~FeedController(); FeedController(const FeedController&) = delete; FeedController& operator=(const FeedController&) = delete; diff --git a/components/brave_news/browser/feed_fetcher.cc b/components/brave_news/browser/feed_fetcher.cc index fa702a990c35..d63795360e0a 100644 --- a/components/brave_news/browser/feed_fetcher.cc +++ b/components/brave_news/browser/feed_fetcher.cc @@ -106,7 +106,7 @@ std::tuple FeedFetcher::CombineFeedSourceResults( FeedFetcher::FeedFetcher( PublishersController& publishers_controller, scoped_refptr url_loader_factory, - DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate) + base::WeakPtr direct_feed_fetcher_delegate) : publishers_controller_(publishers_controller), api_request_helper_(GetNetworkTrafficAnnotationTag(), url_loader_factory), direct_feed_fetcher_(url_loader_factory, direct_feed_fetcher_delegate) {} diff --git a/components/brave_news/browser/feed_fetcher.h b/components/brave_news/browser/feed_fetcher.h index e4a566e6b66c..fc9a41abc29b 100644 --- a/components/brave_news/browser/feed_fetcher.h +++ b/components/brave_news/browser/feed_fetcher.h @@ -30,7 +30,7 @@ class FeedFetcher { public: FeedFetcher(PublishersController& publishers_controller, scoped_refptr url_loader_factory, - DirectFeedFetcher::Delegate* delegate); + base::WeakPtr delegate); ~FeedFetcher(); FeedFetcher(const FeedFetcher&) = delete; FeedFetcher& operator=(const FeedFetcher&) = delete; diff --git a/components/brave_news/browser/feed_v2_builder.cc b/components/brave_news/browser/feed_v2_builder.cc index 1a99e2d29d13..cb5883a2ee2f 100644 --- a/components/brave_news/browser/feed_v2_builder.cc +++ b/components/brave_news/browser/feed_v2_builder.cc @@ -614,7 +614,7 @@ FeedV2Builder::FeedV2Builder( SuggestionsController& suggestions_controller, BackgroundHistoryQuerier& history_querier, scoped_refptr url_loader_factory, - DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate) + base::WeakPtr direct_feed_fetcher_delegate) : publishers_controller_(publishers_controller), channels_controller_(channels_controller), suggestions_controller_(suggestions_controller), diff --git a/components/brave_news/browser/feed_v2_builder.h b/components/brave_news/browser/feed_v2_builder.h index 64b0aab54b2b..4770e4d4143e 100644 --- a/components/brave_news/browser/feed_v2_builder.h +++ b/components/brave_news/browser/feed_v2_builder.h @@ -45,7 +45,7 @@ class FeedV2Builder { SuggestionsController& suggestions_controller, BackgroundHistoryQuerier& history_querier, scoped_refptr url_loader_factory, - DirectFeedFetcher::Delegate* direct_feed_fetcher_delegate); + base::WeakPtr direct_feed_fetcher_delegate); FeedV2Builder(const FeedV2Builder&) = delete; FeedV2Builder& operator=(const FeedV2Builder&) = delete; ~FeedV2Builder();