Skip to content

Commit

Permalink
Merge pull request #26326 from brave/issues/41989
Browse files Browse the repository at this point in the history 
[ads] Fix `DanglingUntriaged` usage
  • Loading branch information
@aseren
aseren authored Nov 1, 2024
2 parents 6d0ffe2 + 2e949de commit 57a44aa
Show file tree
Hide file tree
Showing 9 changed files with 134 additions and 132 deletions.
22 changes: 16 additions & 6 deletions browser/brave_ads/ads_service_delegate.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -107,7 +108,6 @@ AdsServiceDelegate::AdsServiceDelegate(
PrefService* local_state,
brave_adaptive_captcha::BraveAdaptiveCaptchaService*
adaptive_captcha_service,
NotificationDisplayService* notification_display_service,
std::unique_ptr<NotificationAdPlatformBridge>
notification_ad_platform_bridge)
: profile_(profile),
Expand All @@ -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)) {}

Expand Down Expand Up @@ -224,13 +223,19 @@ void AdsServiceDelegate::SnoozeScheduledCaptcha() {

void AdsServiceDelegate::Display(
const message_center::Notification& notification) {
notification_display_service_->Display(NotificationHandler::Type::BRAVE_ADS,
notification, nullptr);
// 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) {
notification_display_service_->Close(NotificationHandler::Type::BRAVE_ADS,
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);
}

void AdsServiceDelegate::ShowNotificationAd(const std::string& id,
Expand Down Expand Up @@ -279,4 +284,9 @@ base::Value::Dict AdsServiceDelegate::GetVirtualPrefs() {
.Set("[virtual]:skus", GetSkus());
}

NotificationDisplayService*
AdsServiceDelegate::GetNotificationDisplayService() {
return NotificationDisplayServiceFactory::GetForProfile(profile_);
}

} // namespace brave_ads
9 changes: 4 additions & 5 deletions browser/brave_ads/ads_service_delegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<NotificationAdPlatformBridge>
notification_ad_platform_bridge);

Expand Down Expand Up @@ -78,13 +77,13 @@ class AdsServiceDelegate : public AdsService::Delegate {
base::Value::Dict GetVirtualPrefs() override;

private:
raw_ptr<Profile> profile_ = nullptr;
raw_ptr<PrefService> local_state_ = nullptr;
NotificationDisplayService* GetNotificationDisplayService();

raw_ptr<Profile> profile_ = nullptr; // NOT OWNED
raw_ptr<PrefService> local_state_ = nullptr; // NOT OWNED
search_engines::SearchEngineChoiceService search_engine_choice_service_;
raw_ptr<brave_adaptive_captcha::BraveAdaptiveCaptchaService>
adaptive_captcha_service_ = nullptr;
raw_ptr<NotificationDisplayService, DanglingUntriaged>
notification_display_service_ = nullptr;
std::unique_ptr<NotificationAdPlatformBridge>
notification_ad_platform_bridge_;
};
Expand Down
3 changes: 0 additions & 3 deletions browser/brave_ads/ads_service_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<AdsServiceDelegate>(
profile, g_browser_process->local_state(), brave_adaptive_captcha_service,
display_service,
std::make_unique<NotificationAdPlatformBridge>(*profile));

auto* history_service = HistoryServiceFactory::GetInstance()->GetForProfile(
Expand Down
7 changes: 3 additions & 4 deletions browser/brave_ads/analytics/p3a/brave_stats_helper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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(
Expand Down
6 changes: 3 additions & 3 deletions browser/brave_ads/analytics/p3a/brave_stats_helper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -54,8 +54,8 @@ class BraveStatsHelper : public ProfileManagerObserver, public ProfileObserver {
base::ScopedObservation<ProfileManager, ProfileManagerObserver>
profile_manager_observer_{this};

raw_ptr<PrefService> local_state_;
raw_ptr<ProfileManager, DanglingUntriaged> profile_manager_;
raw_ptr<PrefService> local_state_ = nullptr; // NOT OWNED
raw_ptr<ProfileManager> profile_manager_ = nullptr; // NOT OWNED

base::Time testing_first_run_time_;
};
Expand Down
98 changes: 44 additions & 54 deletions browser/brave_ads/analytics/p3a/brave_stats_helper_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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, DanglingUntriaged> profile_one_;

base::FilePath profile_two_path_;
raw_ptr<Profile, DanglingUntriaged> profile_two_;
PrefService* local_state() const { return g_browser_process->local_state(); }

raw_ptr<ProfileManager, DanglingUntriaged> profile_manager_;
raw_ptr<PrefService, DanglingUntriaged> local_state_;
raw_ptr<BraveStatsHelper, DanglingUntriaged> 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);

Expand All @@ -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);
Expand Down
Loading

0 comments on commit 57a44aa

Please sign in to comment.