Skip to content

Commit

Permalink
Merge pull request #4442 from brave/ntp-sbw-canada-crash
Browse files Browse the repository at this point in the history
NTP Sponsored Images: Fix crash when using component with no data
  • Loading branch information
petemill committed Jan 29, 2020
1 parent 36c9ec0 commit d1e898b
Show file tree
Hide file tree
Showing 15 changed files with 211 additions and 23 deletions.
1 change: 1 addition & 0 deletions browser/ntp_sponsored_images/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ source_set("ntp_sponsored_images") {
deps = [
"//base",
"//brave/components/ntp_sponsored_images/browser",
"//brave/components/ntp_sponsored_images/common",
"//brave/common",
"//brave/components/brave_ads/browser",
"//chrome/common",
Expand Down
8 changes: 3 additions & 5 deletions browser/ntp_sponsored_images/view_counter_service_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "brave/components/ntp_sponsored_images/browser/features.h"
#include "brave/components/ntp_sponsored_images/browser/ntp_sponsored_images_service.h"
#include "brave/components/ntp_sponsored_images/browser/view_counter_service.h"
#include "brave/components/ntp_sponsored_images/common/pref_names.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/common/chrome_features.h"
#include "content/public/browser/browser_context.h"
Expand Down Expand Up @@ -68,11 +69,8 @@ KeyedService* ViewCounterServiceFactory::BuildServiceInstanceFor(
}

void ViewCounterServiceFactory::RegisterProfilePrefs(
user_prefs::PrefRegistrySyncable* registry) {
registry->RegisterBooleanPref(
kBrandedWallpaperNotificationDismissed, false);
registry->RegisterBooleanPref(
kNewTabPageShowBrandedBackgroundImage, true);
user_prefs::PrefRegistrySyncable* registry) {
ViewCounterService::RegisterProfilePrefs(registry);
}

bool ViewCounterServiceFactory::ServiceIsCreatedWithBrowserContext() const {
Expand Down
19 changes: 13 additions & 6 deletions browser/ui/webui/brave_new_tab_message_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "brave/components/ntp_sponsored_images/browser/features.h"
#include "brave/components/ntp_sponsored_images/browser/ntp_sponsored_images_data.h"
#include "brave/components/ntp_sponsored_images/browser/view_counter_service.h"
#include "brave/components/ntp_sponsored_images/common/pref_names.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/common/chrome_features.h"
#include "content/public/browser/web_ui_data_source.h"
Expand Down Expand Up @@ -78,7 +79,8 @@ base::DictionaryValue GetPreferencesDictionary(PrefService* prefs) {
prefs->GetBoolean(kNewTabPageShowBackgroundImage));
pref_data.SetBoolean(
"brandedWallpaperOptIn",
prefs->GetBoolean(kNewTabPageShowBrandedBackgroundImage));
prefs->GetBoolean(
ntp_sponsored_images::prefs::kNewTabPageShowBrandedBackgroundImage));
pref_data.SetBoolean(
"showClock",
prefs->GetBoolean(kNewTabPageShowClock));
Expand All @@ -93,7 +95,8 @@ base::DictionaryValue GetPreferencesDictionary(PrefService* prefs) {
prefs->GetBoolean(kNewTabPageShowRewards));
pref_data.SetBoolean(
"isBrandedWallpaperNotificationDismissed",
prefs->GetBoolean(kBrandedWallpaperNotificationDismissed));
prefs->GetBoolean(
ntp_sponsored_images::prefs::kBrandedWallpaperNotificationDismissed));
return pref_data;
}

Expand Down Expand Up @@ -222,7 +225,8 @@ void BraveNewTabMessageHandler::OnJavascriptAllowed() {
pref_change_registrar_.Add(kNewTabPageShowBackgroundImage,
base::Bind(&BraveNewTabMessageHandler::OnPreferencesChanged,
base::Unretained(this)));
pref_change_registrar_.Add(kNewTabPageShowBrandedBackgroundImage,
pref_change_registrar_.Add(
ntp_sponsored_images::prefs::kNewTabPageShowBrandedBackgroundImage,
base::Bind(&BraveNewTabMessageHandler::OnPreferencesChanged,
base::Unretained(this)));
pref_change_registrar_.Add(kNewTabPageShowClock,
Expand All @@ -237,7 +241,8 @@ void BraveNewTabMessageHandler::OnJavascriptAllowed() {
pref_change_registrar_.Add(kNewTabPageShowRewards,
base::Bind(&BraveNewTabMessageHandler::OnPreferencesChanged,
base::Unretained(this)));
pref_change_registrar_.Add(kBrandedWallpaperNotificationDismissed,
pref_change_registrar_.Add(
ntp_sponsored_images::prefs::kBrandedWallpaperNotificationDismissed,
base::Bind(&BraveNewTabMessageHandler::OnPreferencesChanged,
base::Unretained(this)));
}
Expand Down Expand Up @@ -296,7 +301,8 @@ void BraveNewTabMessageHandler::HandleSaveNewTabPagePref(
if (settingsKeyInput == "showBackgroundImage") {
settingsKey = kNewTabPageShowBackgroundImage;
} else if (settingsKeyInput == "brandedWallpaperOptIn") {
settingsKey = kNewTabPageShowBrandedBackgroundImage;
settingsKey =
ntp_sponsored_images::prefs::kNewTabPageShowBrandedBackgroundImage;
} else if (settingsKeyInput == "showClock") {
settingsKey = kNewTabPageShowClock;
} else if (settingsKeyInput == "showTopSites") {
Expand All @@ -306,7 +312,8 @@ void BraveNewTabMessageHandler::HandleSaveNewTabPagePref(
} else if (settingsKeyInput == "showRewards") {
settingsKey = kNewTabPageShowRewards;
} else if (settingsKeyInput == "isBrandedWallpaperNotificationDismissed") {
settingsKey = kBrandedWallpaperNotificationDismissed;
settingsKey =
ntp_sponsored_images::prefs::kBrandedWallpaperNotificationDismissed;
} else {
LOG(ERROR) << "Invalid setting key";
return;
Expand Down
4 changes: 0 additions & 4 deletions common/pref_names.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,10 @@ const char kHideBraveRewardsButton[] = "brave.hide_brave_rewards_button";
const char kIPFSCompanionEnabled[] = "brave.ipfs_companion_enabled";
const char kNewTabPageShowBackgroundImage[] =
"brave.new_tab_page.show_background_image";
const char kNewTabPageShowBrandedBackgroundImage[] =
"brave.new_tab_page.show_branded_background_image";
const char kNewTabPageShowClock[] = "brave.new_tab_page.show_clock";
const char kNewTabPageShowTopSites[] = "brave.new_tab_page.show_top_sites";
const char kNewTabPageShowStats[] = "brave.new_tab_page.show_stats";
const char kNewTabPageShowRewards[] = "brave.new_tab_page.show_rewards";
const char kBrandedWallpaperNotificationDismissed[] =
"brave.branded_wallpaper_notification_dismissed";
const char kBraveEnabledMediaRouter[] = "brave.enable_media_router";
const char kBraveWalletAES256GCMSivNonce[] =
"brave.wallet.aes_256_gcm_siv_nonce";
Expand Down
2 changes: 0 additions & 2 deletions common/pref_names.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,10 @@ extern const char kHangoutsEnabled[];
extern const char kHideBraveRewardsButton[];
extern const char kIPFSCompanionEnabled[];
extern const char kNewTabPageShowBackgroundImage[];
extern const char kNewTabPageShowBrandedBackgroundImage[];
extern const char kNewTabPageShowClock[];
extern const char kNewTabPageShowTopSites[];
extern const char kNewTabPageShowStats[];
extern const char kNewTabPageShowRewards[];
extern const char kBrandedWallpaperNotificationDismissed[];
extern const char kBraveEnabledMediaRouter[];
extern const char kBraveWalletAES256GCMSivNonce[];
extern const char kBraveWalletEncryptedSeed[];
Expand Down
1 change: 1 addition & 0 deletions components/ntp_sponsored_images/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ static_library("browser") {
"//brave/components/brave_ads/common",
"//brave/components/brave_rewards/common",
"//brave/components/brave_component_updater/browser",
"//brave/components/ntp_sponsored_images/common",
"//components/component_updater",
"//components/keyed_service/core",
"//components/prefs",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ NTPSponsoredImagesData::NTPSponsoredImagesData(
NTPSponsoredImagesData::~NTPSponsoredImagesData() = default;

bool NTPSponsoredImagesData::IsValid() const {
return wallpaper_image_files.size() > 0;
return (wallpaper_image_files.size() > 0 && !logo_destination_url.empty());
}

std::string NTPSponsoredImagesData::logo_image_url() const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,16 @@
#include "base/observer_list.h"

FORWARD_DECLARE_TEST(NTPSponsoredImagesServiceTest, InternalDataTest);
FORWARD_DECLARE_TEST(NTPSponsoredImagesViewCounterTest,
NotActiveInitially);
FORWARD_DECLARE_TEST(NTPSponsoredImagesViewCounterTest,
NotActiveWithBadData);
FORWARD_DECLARE_TEST(NTPSponsoredImagesViewCounterTest,
NotActiveOptedOut);
FORWARD_DECLARE_TEST(NTPSponsoredImagesViewCounterTest,
IsActiveOptedIn);
FORWARD_DECLARE_TEST(NTPSponsoredImagesViewCounterTest,
ActiveInitiallyOptedIn);

namespace component_updater {
class ComponentUpdateService;
Expand Down Expand Up @@ -59,6 +69,17 @@ class NTPSponsoredImagesService {

private:
FRIEND_TEST_ALL_PREFIXES(::NTPSponsoredImagesServiceTest, InternalDataTest);
FRIEND_TEST_ALL_PREFIXES(::NTPSponsoredImagesViewCounterTest,
NotActiveInitially);
FRIEND_TEST_ALL_PREFIXES(::NTPSponsoredImagesViewCounterTest,
NotActiveWithBadData);
FRIEND_TEST_ALL_PREFIXES(::NTPSponsoredImagesViewCounterTest,
NotActiveOptedOut);
FRIEND_TEST_ALL_PREFIXES(::NTPSponsoredImagesViewCounterTest,
IsActiveOptedIn);
FRIEND_TEST_ALL_PREFIXES(::NTPSponsoredImagesViewCounterTest,
ActiveInitiallyOptedIn);


void OnComponentReady(const base::FilePath& installed_dir);
void OnGetPhotoJsonData(const std::string& photo_json);
Expand Down
22 changes: 17 additions & 5 deletions components/ntp_sponsored_images/browser/view_counter_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
#include "brave/components/brave_rewards/common/pref_names.h"
#include "brave/components/ntp_sponsored_images/browser/features.h"
#include "brave/components/ntp_sponsored_images/browser/ntp_sponsored_images_data.h"
#include "brave/components/ntp_sponsored_images/common/pref_names.h"
#include "components/pref_registry/pref_registry_syncable.h"
#include "components/prefs/pref_change_registrar.h"
#include "components/prefs/pref_service.h"

Expand All @@ -39,6 +41,15 @@ NTPSponsoredImagesData* GetDemoWallpaper() {

} // namespace

// static
void ViewCounterService::RegisterProfilePrefs(
user_prefs::PrefRegistrySyncable* registry) {
registry->RegisterBooleanPref(
prefs::kBrandedWallpaperNotificationDismissed, false);
registry->RegisterBooleanPref(
prefs::kNewTabPageShowBrandedBackgroundImage, true);
}

ViewCounterService::ViewCounterService(
NTPSponsoredImagesService* service,
PrefService* prefs,
Expand All @@ -49,6 +60,7 @@ ViewCounterService::ViewCounterService(
// If we have a wallpaper, store it as private var.
// Set demo wallpaper if a flag is set.
if (!base::FeatureList::IsEnabled(features::kBraveNTPBrandedWallpaperDemo)) {
DCHECK(service_);
service_->AddObserver(this);
}

Expand All @@ -71,9 +83,8 @@ ViewCounterService::~ViewCounterService() = default;
NTPSponsoredImagesData* ViewCounterService::current_wallpaper() {
if (base::FeatureList::IsEnabled(features::kBraveNTPBrandedWallpaperDemo)) {
return GetDemoWallpaper();
} else {
return service_->GetSponsoredImagesData();
}
return service_->GetSponsoredImagesData();
}

void ViewCounterService::Shutdown() {
Expand All @@ -100,7 +111,7 @@ void ViewCounterService::OnPreferenceChanged(
}

void ViewCounterService::ResetNotificationState() {
prefs_->SetBoolean(kBrandedWallpaperNotificationDismissed, false);
prefs_->SetBoolean(prefs::kBrandedWallpaperNotificationDismissed, false);
}

void ViewCounterService::RegisterPageView() {
Expand All @@ -113,7 +124,8 @@ void ViewCounterService::RegisterPageView() {
}

bool ViewCounterService::IsBrandedWallpaperActive() {
return (is_supported_locale_ && IsOptedIn() && current_wallpaper());
return (is_supported_locale_ && IsOptedIn() && current_wallpaper() &&
current_wallpaper()->IsValid());
}

bool ViewCounterService::ShouldShowBrandedWallpaper() {
Expand All @@ -125,7 +137,7 @@ size_t ViewCounterService::GetWallpaperImageIndexToDisplay() {
}

bool ViewCounterService::IsOptedIn() {
return prefs_->GetBoolean(kNewTabPageShowBrandedBackgroundImage) &&
return prefs_->GetBoolean(prefs::kNewTabPageShowBrandedBackgroundImage) &&
prefs_->GetBoolean(kNewTabPageShowBackgroundImage);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

class PrefService;

namespace user_prefs {
class PrefRegistrySyncable;
} // namespace user_prefs

namespace ntp_sponsored_images {

struct NTPSponsoredImagesData;
Expand All @@ -34,6 +38,8 @@ class ViewCounterService
ViewCounterService& operator=(
const ViewCounterService&) = delete;

static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);

// Lets the counter know that a New Tab Page view has occured.
// This should always be called as it will evaluate whether the user has
// opted-in or data is available.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/* Copyright (c) 2020 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 http://mozilla.org/MPL/2.0/. */

#include <string>

#include "base/files/file_path.h"
#include "brave/common/pref_names.h"
#include "brave/components/ntp_sponsored_images/browser/ntp_sponsored_images_data.h"
#include "brave/components/ntp_sponsored_images/browser/ntp_sponsored_images_service.h"
#include "brave/components/ntp_sponsored_images/browser/view_counter_service.h"
#include "brave/components/ntp_sponsored_images/common/pref_names.h"
#include "components/prefs/testing_pref_service.h"
#include "components/pref_registry/pref_registry_syncable.h"
#include "components/sync_preferences/testing_pref_service_syncable.h"
#include "testing/gtest/include/gtest/gtest.h"

using ntp_sponsored_images::ViewCounterService;
using ntp_sponsored_images::NTPSponsoredImagesData;
using ntp_sponsored_images::NTPSponsoredImagesService;


class NTPSponsoredImagesViewCounterTest : public testing::Test {
public:
NTPSponsoredImagesViewCounterTest() = default;
~NTPSponsoredImagesViewCounterTest() override = default;

void SetUp() override {
// Need ntp_sponsored_images prefs
auto* registry = prefs()->registry();
ViewCounterService::RegisterProfilePrefs(registry);
// Need general prefs for "ShowBackgroundImage"
registry->RegisterBooleanPref(kNewTabPageShowBackgroundImage, true);

service_ = std::make_unique<NTPSponsoredImagesService>(nullptr);
view_counter_ = std::make_unique<ViewCounterService>(
service_.get(), prefs(), true);
}

void OptOut() {
prefs()->SetBoolean(
ntp_sponsored_images::prefs::kNewTabPageShowBrandedBackgroundImage,
false);
}

void OptIn() {
prefs()->SetBoolean(
ntp_sponsored_images::prefs::kNewTabPageShowBrandedBackgroundImage,
true);
}

std::unique_ptr<NTPSponsoredImagesData> CreateGoodData() {
auto data = std::make_unique<NTPSponsoredImagesData>();
data->url_prefix = "not://real/data/";
data->wallpaper_image_files = {
base::FilePath(FILE_PATH_LITERAL("fake1.jpg")),
base::FilePath(FILE_PATH_LITERAL("fake2.jpg")),
};
data->logo_alt_text = "Test alt text.";
data->logo_company_name = "Test";
data->logo_destination_url = "not://real.site";
return data;
}

sync_preferences::TestingPrefServiceSyncable* prefs() { return &prefs_; }

protected:
sync_preferences::TestingPrefServiceSyncable prefs_;
std::unique_ptr<ViewCounterService> view_counter_;
std::unique_ptr<NTPSponsoredImagesService> service_;
};

TEST_F(NTPSponsoredImagesViewCounterTest, NotActiveInitially) {
// By default, data is bad and wallpaper is not active.
EXPECT_FALSE(view_counter_->IsBrandedWallpaperActive());
}

TEST_F(NTPSponsoredImagesViewCounterTest, NotActiveWithBadData) {
// Set some bad data explicitly.
NTPSponsoredImagesData* badData = new NTPSponsoredImagesData;
service_->images_data_.reset(badData);
EXPECT_FALSE(view_counter_->IsBrandedWallpaperActive());
}

TEST_F(NTPSponsoredImagesViewCounterTest, NotActiveOptedOut) {
// Even with good data, wallpaper should not be active if user pref is off.
service_->images_data_ = CreateGoodData();
OptOut();
EXPECT_FALSE(view_counter_->IsBrandedWallpaperActive());
}

TEST_F(NTPSponsoredImagesViewCounterTest, IsActiveOptedIn) {
service_->images_data_ = CreateGoodData();
OptIn();
EXPECT_TRUE(view_counter_->IsBrandedWallpaperActive());
}

TEST_F(NTPSponsoredImagesViewCounterTest, ActiveInitiallyOptedIn) {
// Sanity check that the default is still to be opted-in.
// If this gets manually changed, then this test should be manually changed
// too.
service_->images_data_ = CreateGoodData();
EXPECT_TRUE(view_counter_->IsBrandedWallpaperActive());
}

6 changes: 6 additions & 0 deletions components/ntp_sponsored_images/common/BUILD.gn
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
source_set("common") {
sources = [
"pref_names.cc",
"pref_names.h"
]
}
Loading

0 comments on commit d1e898b

Please sign in to comment.