From 297a4b55b257aa1ea60dd83739357992471c1780 Mon Sep 17 00:00:00 2001 From: Aleksei Seren Date: Thu, 31 Oct 2024 15:10:29 -0500 Subject: [PATCH 1/5] [DanglingPtr] Fix dangling pointers in BraveStatsHelperBrowserTest --- .../p3a/brave_stats_helper_browsertest.cc | 98 +++++++++---------- 1 file changed, 44 insertions(+), 54 deletions(-) diff --git a/browser/brave_ads/analytics/p3a/brave_stats_helper_browsertest.cc b/browser/brave_ads/analytics/p3a/brave_stats_helper_browsertest.cc index f7fc7cc4290a..8061dfa0769f 100644 --- a/browser/brave_ads/analytics/p3a/brave_stats_helper_browsertest.cc +++ b/browser/brave_ads/analytics/p3a/brave_stats_helper_browsertest.cc @@ -6,7 +6,6 @@ #include "brave/browser/brave_ads/analytics/p3a/brave_stats_helper.h" #include "base/files/file_path.h" -#include "base/memory/raw_ptr.h" #include "base/test/metrics/histogram_tester.h" #include "brave/browser/brave_browser_process.h" #include "brave/components/brave_ads/core/public/prefs/pref_names.h" @@ -27,94 +26,85 @@ class BraveStatsHelperBrowserTest : public PlatformBrowserTest { BraveStatsHelperBrowserTest() = default; protected: - void SetUpOnMainThread() override { - PlatformBrowserTest::SetUpOnMainThread(); - profile_manager_ = g_browser_process->profile_manager(); - local_state_ = g_browser_process->local_state(); - brave_stats_helper_ = g_brave_browser_process->ads_brave_stats_helper(); + Profile& CreateProfile(base::FilePath& profile_path) { + profile_path = profile_manager()->GenerateNextProfileDirectoryPath(); + return profiles::testing::CreateProfileSync(profile_manager(), + profile_path); } - void PostRunTestOnMainThread() override { - PlatformBrowserTest::PostRunTestOnMainThread(); + ProfileManager* profile_manager() const { + return g_browser_process->profile_manager(); } - void CreateMultipleProfiles() { - profile_one_path_ = profile_manager_->GenerateNextProfileDirectoryPath(); - profile_one_ = &profiles::testing::CreateProfileSync(profile_manager_, - profile_one_path_); - profile_two_path_ = profile_manager_->GenerateNextProfileDirectoryPath(); - profile_two_ = &profiles::testing::CreateProfileSync(profile_manager_, - profile_two_path_); - } - - base::FilePath profile_one_path_; - raw_ptr profile_one_; - - base::FilePath profile_two_path_; - raw_ptr profile_two_; + PrefService* local_state() const { return g_browser_process->local_state(); } - raw_ptr profile_manager_; - raw_ptr local_state_; - raw_ptr brave_stats_helper_; + BraveStatsHelper* brave_stats_helper() const { + return g_brave_browser_process->ads_brave_stats_helper(); + } base::HistogramTester histogram_tester_; }; IN_PROC_BROWSER_TEST_F(BraveStatsHelperBrowserTest, PrimaryProfileEnabledUpdate) { - Profile* primary_profile = profile_manager_->GetLastUsedProfile(); + Profile* primary_profile = profile_manager()->GetLastUsedProfile(); - EXPECT_EQ(local_state_->GetBoolean(prefs::kEnabledForLastProfile), false); + EXPECT_EQ(local_state()->GetBoolean(prefs::kEnabledForLastProfile), false); primary_profile->GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, true); - EXPECT_EQ(local_state_->GetBoolean(prefs::kEnabledForLastProfile), true); + EXPECT_EQ(local_state()->GetBoolean(prefs::kEnabledForLastProfile), true); primary_profile->GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, false); - EXPECT_EQ(local_state_->GetBoolean(prefs::kEnabledForLastProfile), false); + EXPECT_EQ(local_state()->GetBoolean(prefs::kEnabledForLastProfile), false); } #if !BUILDFLAG(IS_ANDROID) IN_PROC_BROWSER_TEST_F(BraveStatsHelperBrowserTest, ProfileSwitch) { - CreateMultipleProfiles(); - - profile_one_->GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, true); + base::FilePath profile_one_path; + Profile& profile_one = CreateProfile(profile_one_path); + profile_one.GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, true); - profiles::testing::SwitchToProfileSync(profile_one_path_); - EXPECT_EQ(local_state_->GetBoolean(prefs::kEnabledForLastProfile), true); + profiles::testing::SwitchToProfileSync(profile_one_path); + EXPECT_EQ(local_state()->GetBoolean(prefs::kEnabledForLastProfile), true); - profiles::testing::SwitchToProfileSync(profile_two_path_); - EXPECT_EQ(local_state_->GetBoolean(prefs::kEnabledForLastProfile), false); + base::FilePath profile_two_path; + CreateProfile(profile_two_path); + profiles::testing::SwitchToProfileSync(profile_two_path); + EXPECT_EQ(local_state()->GetBoolean(prefs::kEnabledForLastProfile), false); - profiles::testing::SwitchToProfileSync(profile_one_path_); - EXPECT_EQ(local_state_->GetBoolean(prefs::kEnabledForLastProfile), true); + profiles::testing::SwitchToProfileSync(profile_one_path); + EXPECT_EQ(local_state()->GetBoolean(prefs::kEnabledForLastProfile), true); } IN_PROC_BROWSER_TEST_F(BraveStatsHelperBrowserTest, MultiProfileEnabledUpdate) { - CreateMultipleProfiles(); - profile_one_->GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, true); + base::FilePath profile_one_path; + Profile& profile_one = CreateProfile(profile_one_path); + profile_one.GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, true); - profiles::testing::SwitchToProfileSync(profile_one_path_); - EXPECT_EQ(local_state_->GetBoolean(prefs::kEnabledForLastProfile), true); + profiles::testing::SwitchToProfileSync(profile_one_path); + EXPECT_EQ(local_state()->GetBoolean(prefs::kEnabledForLastProfile), true); - profile_two_->GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, true); - EXPECT_EQ(local_state_->GetBoolean(prefs::kEnabledForLastProfile), true); + base::FilePath profile_two_path; + Profile& profile_two = CreateProfile(profile_two_path); + profile_two.GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, true); + EXPECT_EQ(local_state()->GetBoolean(prefs::kEnabledForLastProfile), true); - profile_one_->GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, false); - EXPECT_EQ(local_state_->GetBoolean(prefs::kEnabledForLastProfile), false); + profile_one.GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, false); + EXPECT_EQ(local_state()->GetBoolean(prefs::kEnabledForLastProfile), false); - profiles::testing::SwitchToProfileSync(profile_two_path_); - EXPECT_EQ(local_state_->GetBoolean(prefs::kEnabledForLastProfile), true); + profiles::testing::SwitchToProfileSync(profile_two_path); + EXPECT_EQ(local_state()->GetBoolean(prefs::kEnabledForLastProfile), true); } #endif IN_PROC_BROWSER_TEST_F(BraveStatsHelperBrowserTest, AdsEnabledInstallationTime) { - brave_stats_helper_->SetFirstRunTimeForTesting(base::Time::Now() - - base::Minutes(45)); + brave_stats_helper()->SetFirstRunTimeForTesting(base::Time::Now() - + base::Minutes(45)); - Profile* primary_profile = profile_manager_->GetLastUsedProfile(); + Profile* primary_profile = profile_manager()->GetLastUsedProfile(); primary_profile->GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, true); @@ -132,9 +122,9 @@ IN_PROC_BROWSER_TEST_F(BraveStatsHelperBrowserTest, // Reset to test another bucket value primary_profile->GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, false); - local_state_->SetBoolean(prefs::kEverEnabledForAnyProfile, false); - brave_stats_helper_->SetFirstRunTimeForTesting(base::Time::Now() - - base::Minutes(70)); + local_state()->SetBoolean(prefs::kEverEnabledForAnyProfile, false); + brave_stats_helper()->SetFirstRunTimeForTesting(base::Time::Now() - + base::Minutes(70)); primary_profile->GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, true); From 47d7469e0576fbdf5ee26ce60208692574f21732 Mon Sep 17 00:00:00 2001 From: Aleksei Seren Date: Thu, 31 Oct 2024 15:10:58 -0500 Subject: [PATCH 2/5] [DanglingPtr] Fix dangling pointers in BraveStatsHelper --- browser/brave_ads/analytics/p3a/brave_stats_helper.cc | 7 +++---- browser/brave_ads/analytics/p3a/brave_stats_helper.h | 6 +++--- browser/brave_browser_process_impl.cc | 4 +++- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/browser/brave_ads/analytics/p3a/brave_stats_helper.cc b/browser/brave_ads/analytics/p3a/brave_stats_helper.cc index bab2df7767a3..6de6f4d74105 100644 --- a/browser/brave_ads/analytics/p3a/brave_stats_helper.cc +++ b/browser/brave_ads/analytics/p3a/brave_stats_helper.cc @@ -10,7 +10,6 @@ #include "brave/browser/brave_stats/first_run_util.h" #include "brave/components/brave_ads/core/public/prefs/pref_names.h" #include "brave/components/p3a_utils/bucket.h" -#include "chrome/browser/browser_process.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile_manager.h" #include "chrome/common/pref_names.h" @@ -25,9 +24,9 @@ const int kAdsEnabledInstallationHourBuckets[] = {0, 11, 23, 71}; } // namespace -BraveStatsHelper::BraveStatsHelper() - : local_state_(g_browser_process->local_state()), - profile_manager_(g_browser_process->profile_manager()) { +BraveStatsHelper::BraveStatsHelper(PrefService* local_state, + ProfileManager* profile_manager) + : local_state_(local_state), profile_manager_(profile_manager) { #if !BUILDFLAG(IS_ANDROID) last_used_profile_pref_change_registrar_.Init(local_state_); last_used_profile_pref_change_registrar_.Add( diff --git a/browser/brave_ads/analytics/p3a/brave_stats_helper.h b/browser/brave_ads/analytics/p3a/brave_stats_helper.h index fc2b21756362..9fcdb9d22d1b 100644 --- a/browser/brave_ads/analytics/p3a/brave_stats_helper.h +++ b/browser/brave_ads/analytics/p3a/brave_stats_helper.h @@ -23,7 +23,7 @@ inline constexpr char kAdsEnabledInstallationTimeHistogramName[] = class BraveStatsHelper : public ProfileManagerObserver, public ProfileObserver { public: - BraveStatsHelper(); + BraveStatsHelper(PrefService* local_state, ProfileManager* profile_manager); ~BraveStatsHelper() override; static void RegisterLocalStatePrefs(PrefRegistrySimple* registry); @@ -54,8 +54,8 @@ class BraveStatsHelper : public ProfileManagerObserver, public ProfileObserver { base::ScopedObservation profile_manager_observer_{this}; - raw_ptr local_state_; - raw_ptr profile_manager_; + raw_ptr local_state_ = nullptr; // NOT OWNED + raw_ptr profile_manager_ = nullptr; // NOT OWNED base::Time testing_first_run_time_; }; diff --git a/browser/brave_browser_process_impl.cc b/browser/brave_browser_process_impl.cc index 3791f9631860..6a5d49468cc6 100644 --- a/browser/brave_browser_process_impl.cc +++ b/browser/brave_browser_process_impl.cc @@ -181,6 +181,7 @@ void BraveBrowserProcessImpl::Init() { #if !BUILDFLAG(IS_ANDROID) void BraveBrowserProcessImpl::StartTearDown() { + brave_stats_helper_.reset(); brave_stats_updater_.reset(); brave_referrals_service_.reset(); #if BUILDFLAG(BRAVE_P3A_ENABLED) @@ -455,7 +456,8 @@ brave_stats::BraveStatsUpdater* BraveBrowserProcessImpl::brave_stats_updater() { brave_ads::BraveStatsHelper* BraveBrowserProcessImpl::ads_brave_stats_helper() { if (!brave_stats_helper_) { - brave_stats_helper_ = std::make_unique(); + brave_stats_helper_ = std::make_unique( + local_state(), profile_manager()); } return brave_stats_helper_.get(); } From 188ee6ec2f5991794f06ea2541325cbbdf61ff17 Mon Sep 17 00:00:00 2001 From: Aleksei Seren Date: Thu, 31 Oct 2024 15:56:20 -0500 Subject: [PATCH 3/5] [DanglingPtr] Add DanglingUntriaged notation for SplitViewBrowserTest --- .../ui/views/brave_javascript_tab_modal_dialog_view_views.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/browser/ui/views/brave_javascript_tab_modal_dialog_view_views.h b/browser/ui/views/brave_javascript_tab_modal_dialog_view_views.h index 0016101b8297..d40aaf4c3c06 100644 --- a/browser/ui/views/brave_javascript_tab_modal_dialog_view_views.h +++ b/browser/ui/views/brave_javascript_tab_modal_dialog_view_views.h @@ -6,6 +6,7 @@ #ifndef BRAVE_BROWSER_UI_VIEWS_BRAVE_JAVASCRIPT_TAB_MODAL_DIALOG_VIEW_VIEWS_H_ #define BRAVE_BROWSER_UI_VIEWS_BRAVE_JAVASCRIPT_TAB_MODAL_DIALOG_VIEW_VIEWS_H_ +#include "base/memory/raw_ref.h" #include "base/memory/weak_ptr.h" #include "chrome/browser/ui/views/javascript_tab_modal_dialog_view_views.h" #include "ui/base/metadata/metadata_header_macros.h" @@ -45,7 +46,7 @@ class BraveJavaScriptTabModalDialogViewViews // This returns point in dialog host's widget coordinate. gfx::Point GetDesiredPositionConsideringSplitView(); - raw_ref web_contents_; + raw_ref web_contents_; base::WeakPtrFactory weak_ptr_factory_{this}; From 499ad60b8861dc18288470073efd696ea5dd834d Mon Sep 17 00:00:00 2001 From: Aleksei Seren Date: Thu, 31 Oct 2024 18:34:28 -0500 Subject: [PATCH 4/5] [DanglingPtr] Fix dangling pointers in AdsServiceDelegate --- browser/brave_ads/ads_service_delegate.cc | 16 ++++++++++------ browser/brave_ads/ads_service_delegate.h | 5 ++--- browser/brave_ads/ads_service_factory.cc | 3 --- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/browser/brave_ads/ads_service_delegate.cc b/browser/brave_ads/ads_service_delegate.cc index 38a9c0b21064..04ee1392b193 100644 --- a/browser/brave_ads/ads_service_delegate.cc +++ b/browser/brave_ads/ads_service_delegate.cc @@ -20,6 +20,7 @@ #include "brave/components/skus/browser/pref_names.h" #include "build/build_config.h" #include "chrome/browser/notifications/notification_display_service.h" +#include "chrome/browser/notifications/notification_display_service_factory.h" #include "chrome/browser/profiles/profile.h" #include "chrome/common/channel_info.h" #include "components/search_engines/template_url_prepopulate_data.h" @@ -107,7 +108,6 @@ AdsServiceDelegate::AdsServiceDelegate( PrefService* local_state, brave_adaptive_captcha::BraveAdaptiveCaptchaService* adaptive_captcha_service, - NotificationDisplayService* notification_display_service, std::unique_ptr notification_ad_platform_bridge) : profile_(profile), @@ -117,7 +117,6 @@ AdsServiceDelegate::AdsServiceDelegate( local_state_, /*is_profile_eligible_for_dse_guest_propagation=*/false), adaptive_captcha_service_(adaptive_captcha_service), - notification_display_service_(notification_display_service), notification_ad_platform_bridge_( std::move(notification_ad_platform_bridge)) {} @@ -224,13 +223,13 @@ void AdsServiceDelegate::SnoozeScheduledCaptcha() { void AdsServiceDelegate::Display( const message_center::Notification& notification) { - notification_display_service_->Display(NotificationHandler::Type::BRAVE_ADS, - notification, nullptr); + GetNotificationDisplayService()->Display(NotificationHandler::Type::BRAVE_ADS, + notification, nullptr); } void AdsServiceDelegate::Close(const std::string& notification_id) { - notification_display_service_->Close(NotificationHandler::Type::BRAVE_ADS, - notification_id); + GetNotificationDisplayService()->Close(NotificationHandler::Type::BRAVE_ADS, + notification_id); } void AdsServiceDelegate::ShowNotificationAd(const std::string& id, @@ -279,4 +278,9 @@ base::Value::Dict AdsServiceDelegate::GetVirtualPrefs() { .Set("[virtual]:skus", GetSkus()); } +NotificationDisplayService* +AdsServiceDelegate::GetNotificationDisplayService() { + return NotificationDisplayServiceFactory::GetForProfile(profile_); +} + } // namespace brave_ads diff --git a/browser/brave_ads/ads_service_delegate.h b/browser/brave_ads/ads_service_delegate.h index c2240d2e5ae0..9a9663ca8c0d 100644 --- a/browser/brave_ads/ads_service_delegate.h +++ b/browser/brave_ads/ads_service_delegate.h @@ -35,7 +35,6 @@ class AdsServiceDelegate : public AdsService::Delegate { PrefService* local_state, brave_adaptive_captcha::BraveAdaptiveCaptchaService* adaptive_captcha_service, - NotificationDisplayService* notification_display_service, std::unique_ptr notification_ad_platform_bridge); @@ -78,13 +77,13 @@ class AdsServiceDelegate : public AdsService::Delegate { base::Value::Dict GetVirtualPrefs() override; private: + NotificationDisplayService* GetNotificationDisplayService(); + raw_ptr profile_ = nullptr; raw_ptr local_state_ = nullptr; search_engines::SearchEngineChoiceService search_engine_choice_service_; raw_ptr adaptive_captcha_service_ = nullptr; - raw_ptr - notification_display_service_ = nullptr; std::unique_ptr notification_ad_platform_bridge_; }; diff --git a/browser/brave_ads/ads_service_factory.cc b/browser/brave_ads/ads_service_factory.cc index be896f5d131b..3f966d5c747f 100644 --- a/browser/brave_ads/ads_service_factory.cc +++ b/browser/brave_ads/ads_service_factory.cc @@ -76,11 +76,8 @@ AdsServiceFactory::BuildServiceInstanceForBrowserContext( auto* brave_adaptive_captcha_service = brave_adaptive_captcha::BraveAdaptiveCaptchaServiceFactory::GetInstance() ->GetForProfile(profile); - auto* display_service = - NotificationDisplayServiceFactory::GetForProfile(profile); auto delegate = std::make_unique( profile, g_browser_process->local_state(), brave_adaptive_captcha_service, - display_service, std::make_unique(*profile)); auto* history_service = HistoryServiceFactory::GetInstance()->GetForProfile( From 2e949dec1a21a7381f76724925dc9d2c532e0e99 Mon Sep 17 00:00:00 2001 From: Aleksei Seren Date: Thu, 31 Oct 2024 18:21:27 -0500 Subject: [PATCH 5/5] [DanglingPtr] Fix dangling pointers in BraveAdsTabHelperTest --- browser/brave_ads/ads_service_delegate.cc | 6 + browser/brave_ads/ads_service_delegate.h | 4 +- .../tabs/ads_tab_helper_browsertest.cc | 114 +++++++++--------- 3 files changed, 67 insertions(+), 57 deletions(-) diff --git a/browser/brave_ads/ads_service_delegate.cc b/browser/brave_ads/ads_service_delegate.cc index 04ee1392b193..648e3eb3f6f4 100644 --- a/browser/brave_ads/ads_service_delegate.cc +++ b/browser/brave_ads/ads_service_delegate.cc @@ -223,11 +223,17 @@ void AdsServiceDelegate::SnoozeScheduledCaptcha() { void AdsServiceDelegate::Display( const message_center::Notification& notification) { + // We cannot store a raw_ptr to NotificationDisplayService due to upstream + // browser tests changes NotificationDisplayService instance during test run + // which leads to dangling pointer errors. GetNotificationDisplayService()->Display(NotificationHandler::Type::BRAVE_ADS, notification, nullptr); } void AdsServiceDelegate::Close(const std::string& notification_id) { + // We cannot store a raw_ptr to NotificationDisplayService due to upstream + // browser tests changes NotificationDisplayService instance during test run + // which leads to dangling pointer errors. GetNotificationDisplayService()->Close(NotificationHandler::Type::BRAVE_ADS, notification_id); } diff --git a/browser/brave_ads/ads_service_delegate.h b/browser/brave_ads/ads_service_delegate.h index 9a9663ca8c0d..b1d061dc1965 100644 --- a/browser/brave_ads/ads_service_delegate.h +++ b/browser/brave_ads/ads_service_delegate.h @@ -79,8 +79,8 @@ class AdsServiceDelegate : public AdsService::Delegate { private: NotificationDisplayService* GetNotificationDisplayService(); - raw_ptr profile_ = nullptr; - raw_ptr local_state_ = nullptr; + raw_ptr profile_ = nullptr; // NOT OWNED + raw_ptr local_state_ = nullptr; // NOT OWNED search_engines::SearchEngineChoiceService search_engine_choice_service_; raw_ptr adaptive_captcha_service_ = nullptr; diff --git a/browser/brave_ads/tabs/ads_tab_helper_browsertest.cc b/browser/brave_ads/tabs/ads_tab_helper_browsertest.cc index 47c11fdf150c..b79dbce75262 100644 --- a/browser/brave_ads/tabs/ads_tab_helper_browsertest.cc +++ b/browser/brave_ads/tabs/ads_tab_helper_browsertest.cc @@ -239,12 +239,14 @@ class BraveAdsTabHelperTest : public PlatformBrowserTest { // Since we are mocking the `AdsService`, a delegate is not required. Note // that we are not testing the `AdsService` itself, these tests are focused // on the `AdsTabHelper`. - auto ads_service = std::make_unique(/*delegate*/ nullptr); - ads_service_mock_ = ads_service.get(); - return ads_service; + return std::make_unique(/*delegate*/ nullptr); } - AdsServiceMock& ads_service_mock() { return *ads_service_mock_; } + AdsServiceMock& GetAdsServiceMock() { + AdsService* ads_service = AdsServiceFactory::GetForProfile(GetProfile()); + CHECK(ads_service); + return *static_cast(ads_service); + } Profile* GetProfile() { return chrome_test_utils::GetProfile(this); } @@ -448,8 +450,6 @@ class BraveAdsTabHelperTest : public PlatformBrowserTest { base::CallbackListSubscription callback_list_subscription_; - raw_ptr ads_service_mock_ = nullptr; - net::EmbeddedTestServer test_server_{ net::test_server::EmbeddedTestServer::TYPE_HTTPS}; net::test_server::EmbeddedTestServerHandle test_server_handle_; @@ -457,7 +457,7 @@ class BraveAdsTabHelperTest : public PlatformBrowserTest { IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, NotifyTabDidChange) { EXPECT_CALL( - ads_service_mock(), + GetAdsServiceMock(), NotifyTabDidChange(TabId(), RedirectChainExpectation(kMultiPageApplicationWebpage), /*is_new_navigation=*/true, /*is_restoring=*/false, @@ -468,11 +468,11 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, NotifyTabDidChange) { IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, NotifyTabDidChangeIfTabWasRestored) { - EXPECT_CALL(ads_service_mock(), NotifyTabDidChange) + EXPECT_CALL(GetAdsServiceMock(), NotifyTabDidChange) .Times(::testing::AnyNumber()); EXPECT_CALL( - ads_service_mock(), + GetAdsServiceMock(), NotifyTabDidChange(TabId(), RedirectChainExpectation(kMultiPageApplicationWebpage), /*is_new_navigation=*/true, /*is_restoring=*/false, @@ -482,6 +482,7 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, // Must occur before the browser is closed. Profile* const profile = GetProfile(); + AdsServiceMock& ads_service_mock = GetAdsServiceMock(); const ScopedKeepAlive scoped_keep_alive(KeepAliveOrigin::SESSION_RESTORE, KeepAliveRestartOption::DISABLED); @@ -491,7 +492,7 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, // We do not know the tab id until the tab is restored, so we match on // `::testing::_`. - EXPECT_CALL(ads_service_mock(), + EXPECT_CALL(ads_service_mock, NotifyTabDidChange( /*tab_id=*/::testing::_, RedirectChainExpectation(kMultiPageApplicationWebpage), @@ -503,34 +504,34 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, } IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, NotifyTabDidLoad) { - EXPECT_CALL(ads_service_mock(), NotifyTabDidLoad(TabId(), net::HTTP_OK)); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabDidLoad(TabId(), net::HTTP_OK)); NavigateToURL(kMultiPageApplicationWebpage, /*has_user_gesture=*/true); } IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, NotifyTabDidLoadForHttpServerErrorResponsePage) { - EXPECT_CALL(ads_service_mock(), + EXPECT_CALL(GetAdsServiceMock(), NotifyTabDidLoad(TabId(), net::HTTP_INTERNAL_SERVER_ERROR)); SimulateHttpStatusCodePage(net::HTTP_INTERNAL_SERVER_ERROR); } IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, NotifyTabDidLoadForHttpClientErrorResponsePage) { - EXPECT_CALL(ads_service_mock(), + EXPECT_CALL(GetAdsServiceMock(), NotifyTabDidLoad(TabId(), net::HTTP_NOT_FOUND)); SimulateHttpStatusCodePage(net::HTTP_NOT_FOUND); } IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, NotifyTabDidLoadForHttpRedirectionResponsePage) { - EXPECT_CALL(ads_service_mock(), + EXPECT_CALL(GetAdsServiceMock(), NotifyTabDidLoad(TabId(), net::HTTP_MOVED_PERMANENTLY)); SimulateHttpStatusCodePage(net::HTTP_MOVED_PERMANENTLY); } IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, NotifyTabDidLoadForHttpSuccessfulResponsePage) { - EXPECT_CALL(ads_service_mock(), NotifyTabDidLoad(TabId(), net::HTTP_OK)); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabDidLoad(TabId(), net::HTTP_OK)); SimulateHttpStatusCodePage(net::HTTP_OK); } @@ -540,7 +541,7 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, base::RunLoop run_loop; EXPECT_CALL( - ads_service_mock(), + GetAdsServiceMock(), NotifyTabHtmlContentDidChange( TabId(), RedirectChainExpectation(kMultiPageApplicationWebpage), kMultiPageApplicationWebpageHtmlContent)) @@ -556,7 +557,7 @@ IN_PROC_BROWSER_TEST_F( base::RunLoop run_loop; EXPECT_CALL( - ads_service_mock(), + GetAdsServiceMock(), NotifyTabHtmlContentDidChange( TabId(), RedirectChainExpectation(kMultiPageApplicationWebpage), /*html=*/::testing::IsEmpty())) @@ -570,14 +571,15 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, GetPrefs()->SetBoolean(brave_rewards::prefs::kEnabled, true); base::RunLoop run_loop; - EXPECT_CALL(ads_service_mock(), NotifyTabHtmlContentDidChange) + EXPECT_CALL(GetAdsServiceMock(), NotifyTabHtmlContentDidChange) .WillOnce(base::test::RunOnceClosure(run_loop.QuitClosure())); NavigateToURL(kMultiPageApplicationWebpage, /*has_user_gesture=*/true); run_loop.Run(); - ::testing::Mock::VerifyAndClearExpectations(&ads_service_mock()); + ::testing::Mock::VerifyAndClearExpectations(&GetAdsServiceMock()); // Must occur before the browser is closed. Profile* const profile = GetProfile(); + AdsServiceMock& ads_service_mock = GetAdsServiceMock(); const ScopedKeepAlive scoped_keep_alive(KeepAliveOrigin::SESSION_RESTORE, KeepAliveRestartOption::DISABLED); @@ -587,7 +589,7 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, // We should not notify about changes to the tab's HTML content, as the // session will be restored and the tab will reload. - EXPECT_CALL(ads_service_mock(), NotifyTabHtmlContentDidChange).Times(0); + EXPECT_CALL(ads_service_mock, NotifyTabHtmlContentDidChange).Times(0); RestoreBrowser(profile); EXPECT_TRUE(WaitForActiveWebContentsToLoad()); @@ -599,13 +601,13 @@ IN_PROC_BROWSER_TEST_F( GetPrefs()->SetBoolean(brave_rewards::prefs::kEnabled, true); base::RunLoop run_loop; - EXPECT_CALL(ads_service_mock(), NotifyTabHtmlContentDidChange) + EXPECT_CALL(GetAdsServiceMock(), NotifyTabHtmlContentDidChange) .WillOnce(base::test::RunOnceClosure(run_loop.QuitClosure())); NavigateToURL(kMultiPageApplicationWebpage, /*has_user_gesture=*/true); run_loop.Run(); - ::testing::Mock::VerifyAndClearExpectations(&ads_service_mock()); + ::testing::Mock::VerifyAndClearExpectations(&GetAdsServiceMock()); - EXPECT_CALL(ads_service_mock(), NotifyTabHtmlContentDidChange).Times(0); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabHtmlContentDidChange).Times(0); GoBack(); GoForward(); Reload(); @@ -618,7 +620,7 @@ IN_PROC_BROWSER_TEST_F( DoNotNotifyTabHtmlContentDidChangeForHttpClientErrorResponsePage) { GetPrefs()->SetBoolean(brave_rewards::prefs::kEnabled, true); - EXPECT_CALL(ads_service_mock(), NotifyTabHtmlContentDidChange).Times(0); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabHtmlContentDidChange).Times(0); SimulateHttpStatusCodePage(net::HTTP_NOT_FOUND); } @@ -627,7 +629,7 @@ IN_PROC_BROWSER_TEST_F( DoNotNotifyTabHtmlContentDidChangeForHttpServerErrorResponsePage) { GetPrefs()->SetBoolean(brave_rewards::prefs::kEnabled, true); - EXPECT_CALL(ads_service_mock(), NotifyTabHtmlContentDidChange).Times(0); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabHtmlContentDidChange).Times(0); SimulateHttpStatusCodePage(net::HTTP_INTERNAL_SERVER_ERROR); } @@ -637,16 +639,16 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, { base::RunLoop run_loop; - EXPECT_CALL(ads_service_mock(), NotifyTabHtmlContentDidChange) + EXPECT_CALL(GetAdsServiceMock(), NotifyTabHtmlContentDidChange) .WillOnce(base::test::RunOnceClosure(run_loop.QuitClosure())); NavigateToURL(kSinglePageApplicationWebpage, /*has_user_gesture=*/true); run_loop.Run(); - ::testing::Mock::VerifyAndClearExpectations(&ads_service_mock()); + ::testing::Mock::VerifyAndClearExpectations(&GetAdsServiceMock()); } { base::RunLoop run_loop; - EXPECT_CALL(ads_service_mock(), + EXPECT_CALL(GetAdsServiceMock(), NotifyTabHtmlContentDidChange( TabId(), ::testing::Contains(FileName("same_document")), kSinglePageApplicationWebpageHtmlContent)) @@ -665,7 +667,7 @@ IN_PROC_BROWSER_TEST_F( base::RunLoop run_loop; EXPECT_CALL( - ads_service_mock(), + GetAdsServiceMock(), NotifyTabTextContentDidChange( TabId(), RedirectChainExpectation(kMultiPageApplicationWebpage), kMultiPageApplicationWebpageTextContent)) @@ -678,7 +680,7 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, DoNotNotifyTabTextContentDidChangeForNonRewardsUser) { GetPrefs()->SetBoolean(brave_rewards::prefs::kEnabled, false); - EXPECT_CALL(ads_service_mock(), NotifyTabTextContentDidChange).Times(0); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabTextContentDidChange).Times(0); NavigateToURL(kMultiPageApplicationWebpage, /*has_user_gesture=*/true); } @@ -688,7 +690,7 @@ IN_PROC_BROWSER_TEST_F( GetPrefs()->SetBoolean(brave_rewards::prefs::kEnabled, false); GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, false); - EXPECT_CALL(ads_service_mock(), NotifyTabTextContentDidChange).Times(0); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabTextContentDidChange).Times(0); NavigateToURL(kMultiPageApplicationWebpage, /*has_user_gesture=*/true); } @@ -698,7 +700,7 @@ IN_PROC_BROWSER_TEST_F( GetPrefs()->SetBoolean(brave_rewards::prefs::kEnabled, true); GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, false); - EXPECT_CALL(ads_service_mock(), NotifyTabTextContentDidChange).Times(0); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabTextContentDidChange).Times(0); NavigateToURL(kMultiPageApplicationWebpage, /*has_user_gesture=*/true); } @@ -708,14 +710,15 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, true); base::RunLoop run_loop; - EXPECT_CALL(ads_service_mock(), NotifyTabTextContentDidChange) + EXPECT_CALL(GetAdsServiceMock(), NotifyTabTextContentDidChange) .WillOnce(base::test::RunOnceClosure(run_loop.QuitClosure())); NavigateToURL(kMultiPageApplicationWebpage, /*has_user_gesture=*/true); run_loop.Run(); - ::testing::Mock::VerifyAndClearExpectations(&ads_service_mock()); + ::testing::Mock::VerifyAndClearExpectations(&GetAdsServiceMock()); // Must occur before the browser is closed. Profile* const profile = GetProfile(); + AdsServiceMock& ads_service_mock = GetAdsServiceMock(); const ScopedKeepAlive scoped_keep_alive(KeepAliveOrigin::SESSION_RESTORE, KeepAliveRestartOption::DISABLED); @@ -725,7 +728,7 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, // We should not notify about changes to the tab's text content, as the // session will be restored and the tab will reload. - EXPECT_CALL(ads_service_mock(), NotifyTabTextContentDidChange).Times(0); + EXPECT_CALL(ads_service_mock, NotifyTabTextContentDidChange).Times(0); RestoreBrowser(profile); EXPECT_TRUE(WaitForActiveWebContentsToLoad()); @@ -738,13 +741,13 @@ IN_PROC_BROWSER_TEST_F( GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, true); base::RunLoop run_loop; - EXPECT_CALL(ads_service_mock(), NotifyTabTextContentDidChange) + EXPECT_CALL(GetAdsServiceMock(), NotifyTabTextContentDidChange) .WillOnce(base::test::RunOnceClosure(run_loop.QuitClosure())); NavigateToURL(kMultiPageApplicationWebpage, /*has_user_gesture=*/true); run_loop.Run(); - ::testing::Mock::VerifyAndClearExpectations(&ads_service_mock()); + ::testing::Mock::VerifyAndClearExpectations(&GetAdsServiceMock()); - EXPECT_CALL(ads_service_mock(), NotifyTabTextContentDidChange).Times(0); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabTextContentDidChange).Times(0); GoBack(); GoForward(); Reload(); @@ -758,7 +761,7 @@ IN_PROC_BROWSER_TEST_F( GetPrefs()->SetBoolean(brave_rewards::prefs::kEnabled, true); GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, true); - EXPECT_CALL(ads_service_mock(), NotifyTabTextContentDidChange).Times(0); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabTextContentDidChange).Times(0); SimulateHttpStatusCodePage(net::HTTP_NOT_FOUND); } @@ -768,7 +771,7 @@ IN_PROC_BROWSER_TEST_F( GetPrefs()->SetBoolean(brave_rewards::prefs::kEnabled, true); GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, true); - EXPECT_CALL(ads_service_mock(), NotifyTabTextContentDidChange).Times(0); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabTextContentDidChange).Times(0); SimulateHttpStatusCodePage(net::HTTP_INTERNAL_SERVER_ERROR); } @@ -779,13 +782,13 @@ IN_PROC_BROWSER_TEST_F( GetPrefs()->SetBoolean(prefs::kOptedInToNotificationAds, true); base::RunLoop run_loop; - EXPECT_CALL(ads_service_mock(), NotifyTabTextContentDidChange) + EXPECT_CALL(GetAdsServiceMock(), NotifyTabTextContentDidChange) .WillOnce(base::test::RunOnceClosure(run_loop.QuitClosure())); NavigateToURL(kSinglePageApplicationWebpage, /*has_user_gesture=*/true); run_loop.Run(); - ::testing::Mock::VerifyAndClearExpectations(&ads_service_mock()); + ::testing::Mock::VerifyAndClearExpectations(&GetAdsServiceMock()); - EXPECT_CALL(ads_service_mock(), NotifyTabTextContentDidChange).Times(0); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabTextContentDidChange).Times(0); SimulateClick(kSinglePageApplicationClickSelectors, /*has_user_gesture=*/true); @@ -799,7 +802,7 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, content::WebContents* const web_contents = GetActiveWebContents(); MediaWaiter waiter(web_contents); - EXPECT_CALL(ads_service_mock(), NotifyTabDidStartPlayingMedia); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabDidStartPlayingMedia); NavigateToURL(kAutoplayVideoWebpage, /*has_user_gesture=*/true); waiter.WaitForMediaStartedPlaying(); @@ -813,7 +816,7 @@ IN_PROC_BROWSER_TEST_F( content::WebContents* const web_contents = GetActiveWebContents(); MediaWaiter waiter(web_contents); - EXPECT_CALL(ads_service_mock(), NotifyTabDidStartPlayingMedia).Times(0); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabDidStartPlayingMedia).Times(0); NavigateToURL(kAutoplayVideoWebpage, /*has_user_gesture=*/true); waiter.WaitForMediaSessionCreated(); @@ -826,19 +829,19 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, content::WebContents* const web_contents = GetActiveWebContents(); MediaWaiter waiter(web_contents); - EXPECT_CALL(ads_service_mock(), NotifyTabDidStartPlayingMedia); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabDidStartPlayingMedia); NavigateToURL(kAutoplayVideoWebpage, /*has_user_gesture=*/true); waiter.WaitForMediaStartedPlaying(); - EXPECT_CALL(ads_service_mock(), NotifyTabDidStopPlayingMedia); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabDidStopPlayingMedia); PauseVideoPlayback(kVideoJavascriptDocumentQuerySelectors); } IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, NotifyTabDidStartPlayingMedia) { NavigateToURL(kVideoWebpage, /*has_user_gesture=*/true); - EXPECT_CALL(ads_service_mock(), NotifyTabDidStartPlayingMedia); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabDidStartPlayingMedia); StartVideoPlayback(kVideoJavascriptDocumentQuerySelectors); } @@ -847,36 +850,37 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, NotifyTabDidStopPlayingMedia) { StartVideoPlayback(kVideoJavascriptDocumentQuerySelectors); - EXPECT_CALL(ads_service_mock(), NotifyTabDidStopPlayingMedia); + EXPECT_CALL(GetAdsServiceMock(), NotifyTabDidStopPlayingMedia); PauseVideoPlayback(kVideoJavascriptDocumentQuerySelectors); } IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, NotifyDidCloseTab) { - EXPECT_CALL(ads_service_mock(), NotifyDidCloseTab); + EXPECT_CALL(GetAdsServiceMock(), NotifyDidCloseTab); CloseActiveWebContents(); } IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, NotifyUserGestureEventTriggered) { - EXPECT_CALL(ads_service_mock(), NotifyUserGestureEventTriggered) + EXPECT_CALL(GetAdsServiceMock(), NotifyUserGestureEventTriggered) .Times(::testing::AtLeast(1)); NavigateToURL(kMultiPageApplicationWebpage, /*has_user_gesture=*/true); } IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, DoNotNotifyUserGestureEventTriggered) { - EXPECT_CALL(ads_service_mock(), NotifyUserGestureEventTriggered).Times(0); + EXPECT_CALL(GetAdsServiceMock(), NotifyUserGestureEventTriggered).Times(0); NavigateToURL(kMultiPageApplicationWebpage, /*has_user_gesture=*/false); } IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, DoNotNotifyUserGestureEventTriggeredIfTabWasRestored) { - EXPECT_CALL(ads_service_mock(), NotifyUserGestureEventTriggered) + EXPECT_CALL(GetAdsServiceMock(), NotifyUserGestureEventTriggered) .Times(::testing::AtLeast(1)); NavigateToURL(kMultiPageApplicationWebpage, /*has_user_gesture=*/true); - ::testing::Mock::VerifyAndClearExpectations(&ads_service_mock()); + ::testing::Mock::VerifyAndClearExpectations(&GetAdsServiceMock()); // Must occur before the browser is closed. Profile* const profile = GetProfile(); + AdsServiceMock& ads_service_mock = GetAdsServiceMock(); const ScopedKeepAlive scoped_keep_alive(KeepAliveOrigin::SESSION_RESTORE, KeepAliveRestartOption::DISABLED); @@ -884,7 +888,7 @@ IN_PROC_BROWSER_TEST_F(BraveAdsTabHelperTest, profile, ProfileKeepAliveOrigin::kSessionRestore); CloseBrowserSynchronously(browser()); - EXPECT_CALL(ads_service_mock(), NotifyUserGestureEventTriggered).Times(0); + EXPECT_CALL(ads_service_mock, NotifyUserGestureEventTriggered).Times(0); RestoreBrowser(profile); EXPECT_TRUE(WaitForActiveWebContentsToLoad());