Skip to content

Commit

Permalink
fixed search extension crash and bug V2
Browse files Browse the repository at this point in the history
fix: brave/brave-browser#15224
fix: brave/brave-browser#10601
fix: brave/brave-browser#14218

Crash comes from absense of exension search provider data in private profile's
TemplateURLService because we are using different TemplateURLService for private profile.
When search provider extension is loaded/installed, browser doesn't notify about
this adding to private profile's TUS.
To fix this, each SearchEngineProviderService sets extension's template url data to
its service when normal profile's search provider comes from extension.
  • Loading branch information
simonhong committed May 7, 2021
1 parent fc638e6 commit f91c501
Show file tree
Hide file tree
Showing 14 changed files with 355 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,47 +5,63 @@

#include "brave/browser/search_engines/private_window_search_engine_provider_service.h"

#include "brave/components/search_engines/pref_names.h"
#include "chrome/browser/profiles/profile.h"
#include "components/search_engines/template_url_service.h"
#include "components/prefs/pref_service.h"
#include "components/search_engines/default_search_manager.h"
#include "components/search_engines/template_url.h"

PrivateWindowSearchEngineProviderService::
PrivateWindowSearchEngineProviderService(Profile* otr_profile)
: SearchEngineProviderService(otr_profile) {
DCHECK(otr_profile->IsIncognitoProfile());

const bool use_extension_provider = ShouldUseExtensionSearchProvider();
otr_profile->GetPrefs()->SetBoolean(kDefaultSearchProviderByExtension,
use_extension_provider);

if (use_extension_provider) {
UseExtensionSearchProvider();
} else {
ConfigureSearchEngineProvider();
}

// Monitor normal profile's search engine changing because private window
// should that search engine provider when alternative search engine isn't
// used.
original_template_url_service_->AddObserver(this);
ConfigureSearchEngineProvider();
observation_.Observe(original_template_url_service_);
}

PrivateWindowSearchEngineProviderService::
~PrivateWindowSearchEngineProviderService() {
original_template_url_service_->RemoveObserver(this);
}
~PrivateWindowSearchEngineProviderService() = default;

void PrivateWindowSearchEngineProviderService::
OnUseAlternativeSearchEngineProviderChanged() {
// If extension search provider is used, user can't change DSE by toggling.
if (ShouldUseExtensionSearchProvider())
return;

ConfigureSearchEngineProvider();
}

void PrivateWindowSearchEngineProviderService::
ConfigureSearchEngineProvider() {
DCHECK(!ShouldUseExtensionSearchProvider());

UseAlternativeSearchEngineProvider()
? ChangeToAlternativeSearchEngineProvider()
: ChangeToNormalWindowSearchEngineProvider();
}

void
PrivateWindowSearchEngineProviderService::OnTemplateURLServiceChanged() {
// If private window uses alternative, search provider changing of normal
// profile should not affect private window's provider.
if (UseAlternativeSearchEngineProvider())
void PrivateWindowSearchEngineProviderService::OnTemplateURLServiceChanged() {
const bool use_extension_provider = ShouldUseExtensionSearchProvider();
otr_profile_->GetPrefs()->SetBoolean(kDefaultSearchProviderByExtension,
use_extension_provider);

if (use_extension_provider) {
UseExtensionSearchProvider();
return;
}

// When normal profile's default search provider is changed, apply it to
// private window's provider.
ChangeToNormalWindowSearchEngineProvider();
ConfigureSearchEngineProvider();
}

Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
/* Copyright (c) 2019 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/. */

#ifndef BRAVE_BROWSER_SEARCH_ENGINES_PRIVATE_WINDOW_SEARCH_ENGINE_PROVIDER_SERVICE_H_
#define BRAVE_BROWSER_SEARCH_ENGINES_PRIVATE_WINDOW_SEARCH_ENGINE_PROVIDER_SERVICE_H_

#include "base/scoped_observation.h"
#include "brave/browser/search_engines/search_engine_provider_service.h"
#include "components/search_engines/template_url_service.h"
#include "components/search_engines/template_url_service_observer.h"

class PrivateWindowSearchEngineProviderService
Expand All @@ -14,6 +17,10 @@ class PrivateWindowSearchEngineProviderService
public:
explicit PrivateWindowSearchEngineProviderService(Profile* otr_profile);
~PrivateWindowSearchEngineProviderService() override;
PrivateWindowSearchEngineProviderService(
const PrivateWindowSearchEngineProviderService&) = delete;
PrivateWindowSearchEngineProviderService& operator=(
const PrivateWindowSearchEngineProviderService&) = delete;

private:
// Configure appropriate provider according to prefs.
Expand All @@ -25,7 +32,8 @@ class PrivateWindowSearchEngineProviderService
// SearchEngineProviderService overrides:
void OnUseAlternativeSearchEngineProviderChanged() override;

DISALLOW_COPY_AND_ASSIGN(PrivateWindowSearchEngineProviderService);
base::ScopedObservation<TemplateURLService, TemplateURLServiceObserver>
observation_{this};
};

#endif // BRAVE_BROWSER_SEARCH_ENGINES_PRIVATE_WINDOW_SEARCH_ENGINE_PROVIDER_SERVICE_H_
59 changes: 56 additions & 3 deletions browser/search_engines/search_engine_provider_service.cc
Original file line number Diff line number Diff line change
@@ -1,18 +1,28 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
/* Copyright (c) 2019 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 "brave/browser/search_engines/search_engine_provider_service.h"

#include <utility>
#include <vector>

#include "brave/browser/search_engines/search_engine_provider_util.h"
#include "brave/common/pref_names.h"
#include "brave/components/search_engines/brave_prepopulated_engines.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/search_engines/template_url_service_factory.h"
#include "components/prefs/pref_service.h"
#include "components/search_engines/prepopulated_engines.h"
#include "components/search_engines/template_url_data_util.h"
#include "components/search_engines/template_url_prepopulate_data.h"
#include "components/search_engines/template_url_service.h"
#include "extensions/buildflags/buildflags.h"

#if BUILDFLAG(ENABLE_EXTENSIONS)
#include "extensions/browser/extension_prefs.h"
#endif

SearchEngineProviderService::SearchEngineProviderService(
Profile* otr_profile)
Expand Down Expand Up @@ -49,8 +59,7 @@ SearchEngineProviderService::SearchEngineProviderService(
alternative_search_engine_url_.reset(new TemplateURL(*data));
}

SearchEngineProviderService::~SearchEngineProviderService() {
}
SearchEngineProviderService::~SearchEngineProviderService() = default;

void SearchEngineProviderService::OnPreferenceChanged(
const std::string& pref_name) {
Expand All @@ -76,3 +85,47 @@ void SearchEngineProviderService::ChangeToNormalWindowSearchEngineProvider() {
otr_template_url_service_->SetUserSelectedDefaultSearchProvider(
&normal_url);
}

void SearchEngineProviderService::UseExtensionSearchProvider() {
#if BUILDFLAG(ENABLE_EXTENSIONS)
DCHECK(ShouldUseExtensionSearchProvider());

const auto* extension_provider_url =
original_template_url_service_->GetDefaultSearchProvider();
auto data = extension_provider_url->data();
data.id = kInvalidTemplateURLID;

// Can't add same turl again to service.
if (CouldAddExtensionTemplateURL(extension_provider_url)) {
auto type = extension_provider_url->type();
auto extension_id = extension_provider_url->GetExtensionId();
extensions::ExtensionPrefs* prefs =
extensions::ExtensionPrefs::Get(otr_profile_->GetOriginalProfile());
auto time = prefs->GetInstallTime(extension_id);

auto turl =
std::make_unique<TemplateURL>(data, type, extension_id, time, true);

otr_template_url_service_->Add(std::move(turl));
}

otr_profile_->GetPrefs()->Set(
DefaultSearchManager::kDefaultSearchProviderDataPrefName,
*TemplateURLDataToDictionary(data));
#endif
}

bool SearchEngineProviderService::ShouldUseExtensionSearchProvider() {
return original_template_url_service_->IsExtensionControlledDefaultSearch();
}

bool SearchEngineProviderService::CouldAddExtensionTemplateURL(
const TemplateURL* url) {
DCHECK_NE(TemplateURL::NORMAL, url->type());
for (const auto* turl : otr_template_url_service_->GetTemplateURLs()) {
if (url->type() == turl->type() &&
url->GetExtensionId() == turl->GetExtensionId())
return false;
}
return true;
}
22 changes: 14 additions & 8 deletions browser/search_engines/search_engine_provider_service.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
/* Copyright (c) 2019 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/. */

Expand All @@ -21,6 +22,10 @@ class SearchEngineProviderService : public KeyedService {
explicit SearchEngineProviderService(Profile* otr_profile);
~SearchEngineProviderService() override;

SearchEngineProviderService(const SearchEngineProviderService&) = delete;
SearchEngineProviderService& operator=(const SearchEngineProviderService&) =
delete;

protected:
// If subclass want to know and configure according to prefs change, override
// this.
Expand All @@ -30,21 +35,22 @@ class SearchEngineProviderService : public KeyedService {
void ChangeToAlternativeSearchEngineProvider();
void ChangeToNormalWindowSearchEngineProvider();

bool ShouldUseExtensionSearchProvider();
void UseExtensionSearchProvider();

// Points off the record profile.
Profile* otr_profile_;
Profile* otr_profile_ = nullptr;
// Service for original profile of |otr_profile_|.
TemplateURLService* original_template_url_service_;
TemplateURLService* original_template_url_service_ = nullptr;
// Service for off the record profile.
TemplateURLService* otr_template_url_service_;

std::unique_ptr<TemplateURL> alternative_search_engine_url_;
TemplateURLService* otr_template_url_service_ = nullptr;

private:
void OnPreferenceChanged(const std::string& pref_name);
bool CouldAddExtensionTemplateURL(const TemplateURL* url);

std::unique_ptr<TemplateURL> alternative_search_engine_url_;
BooleanPrefMember use_alternative_search_engine_provider_;

DISALLOW_COPY_AND_ASSIGN(SearchEngineProviderService);
};

#endif // BRAVE_BROWSER_SEARCH_ENGINES_SEARCH_ENGINE_PROVIDER_SERVICE_H_
115 changes: 115 additions & 0 deletions browser/search_engines/search_engine_provider_service_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* 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 "base/path_service.h"
#include "base/strings/utf_string_conversions.h"
#include "brave/browser/profiles/brave_profile_manager.h"
#include "brave/browser/profiles/profile_util.h"
Expand All @@ -17,13 +18,23 @@
#include "chrome/browser/search_engines/template_url_service_factory.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_list.h"
#include "chrome/common/chrome_paths.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/search_test_utils.h"
#include "chrome/test/base/testing_browser_process.h"
#include "components/search_engines/search_engines_test_util.h"
#include "components/search_engines/template_url_prepopulate_data.h"
#include "components/search_engines/template_url_service.h"
#include "components/search_engines/template_url_service_observer.h"
#include "content/public/test/browser_test.h"
#include "content/public/test/test_utils.h"
#include "extensions/buildflags/buildflags.h"

#if BUILDFLAG(ENABLE_EXTENSIONS)
#include "chrome/browser/extensions/extension_browsertest.h"
#include "extensions/browser/extension_prefs.h"
#include "extensions/common/extension.h"
#endif

using SearchEngineProviderServiceTest = InProcessBrowserTest;

Expand Down Expand Up @@ -231,3 +242,107 @@ IN_PROC_BROWSER_TEST_F(SearchEngineProviderServiceTest,
search_engine_provider_service->OnTemplateURLServiceChanged();
EXPECT_TRUE(brave::UseAlternativeSearchEngineProviderEnabled(guest_profile));
}

#if BUILDFLAG(ENABLE_EXTENSIONS)

namespace extensions {

// Copied from settings_overrides_browsertest.cc
// On linux, search engine from extension is not set by default.
#if defined(OS_WIN) || defined(OS_MAC)
// Prepopulated id hardcoded in test_extension.
const int kTestExtensionPrepopulatedId = 3;
// TemplateURLData with search engines settings from test extension manifest.
// chrome/test/data/extensions/settings_override/manifest.json
std::unique_ptr<TemplateURLData> TestExtensionSearchEngine(PrefService* prefs) {
auto result = std::make_unique<TemplateURLData>();
result->SetShortName(base::ASCIIToUTF16("name.de"));
result->SetKeyword(base::ASCIIToUTF16("keyword.de"));
result->SetURL("http://www.foo.de/s?q={searchTerms}&id=10");
result->favicon_url = GURL("http://www.foo.de/favicon.ico?id=10");
result->suggestions_url = "http://www.foo.de/suggest?q={searchTerms}&id=10";
result->image_url = "http://www.foo.de/image?q={searchTerms}&id=10";
result->search_url_post_params = "search_lang=de";
result->suggestions_url_post_params = "suggest_lang=de";
result->image_url_post_params = "image_lang=de";
result->alternate_urls.push_back("http://www.moo.de/s?q={searchTerms}&id=10");
result->alternate_urls.push_back("http://www.noo.de/s?q={searchTerms}&id=10");
result->input_encodings.push_back("UTF-8");

std::unique_ptr<TemplateURLData> prepopulated =
TemplateURLPrepopulateData::GetPrepopulatedEngine(
prefs, kTestExtensionPrepopulatedId);
// Values below do not exist in extension manifest and are taken from
// prepopulated engine with prepopulated_id set in extension manifest.
result->contextual_search_url = prepopulated->contextual_search_url;
result->new_tab_url = prepopulated->new_tab_url;
return result;
}

testing::AssertionResult VerifyTemplateURLServiceLoad(
TemplateURLService* service) {
if (service->loaded())
return testing::AssertionSuccess();
search_test_utils::WaitForTemplateURLServiceToLoad(service);
if (service->loaded())
return testing::AssertionSuccess();
return testing::AssertionFailure() << "TemplateURLService isn't loaded";
}

IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest,
ExtensionSearchProviderWithPrivateWindow) {
PrefService* prefs = profile()->GetPrefs();
ASSERT_TRUE(prefs);
TemplateURLService* url_service =
TemplateURLServiceFactory::GetForProfile(profile());
ASSERT_TRUE(url_service);
EXPECT_TRUE(VerifyTemplateURLServiceLoad(url_service));
const TemplateURL* default_provider = url_service->GetDefaultSearchProvider();
ASSERT_TRUE(default_provider);
EXPECT_EQ(TemplateURL::NORMAL, default_provider->type());

const extensions::Extension* extension = LoadExtension(
test_data_dir_.AppendASCII("settings_override"), {.install_param = "10"});
ASSERT_TRUE(extension);
const TemplateURL* current_dse = url_service->GetDefaultSearchProvider();
EXPECT_EQ(TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION, current_dse->type());

std::unique_ptr<TemplateURLData> extension_dse =
TestExtensionSearchEngine(prefs);
ExpectSimilar(extension_dse.get(), &current_dse->data());

Profile* incognito_profile = profile()->GetPrimaryOTRProfile();

auto* incognito_url_service =
TemplateURLServiceFactory::GetForProfile(incognito_profile);
const TemplateURL* current_incognito_dse =
incognito_url_service->GetDefaultSearchProvider();
EXPECT_EQ(TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION,
current_incognito_dse->type());

if (!brave::IsRegionForQwant(profile())) {
// DDG toggle button is on and its preference is stored.
// but search provider is not changed. still extension search provider is
// used.
EXPECT_FALSE(brave::UseAlternativeSearchEngineProviderEnabled(profile()));
brave::ToggleUseAlternativeSearchEngineProvider(profile());
EXPECT_TRUE(brave::UseAlternativeSearchEngineProviderEnabled(profile()));
current_incognito_dse = incognito_url_service->GetDefaultSearchProvider();
EXPECT_EQ(TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION,
current_incognito_dse->type());
}

UnloadExtension(extension->id());
EXPECT_EQ(default_provider, url_service->GetDefaultSearchProvider());

// After unloading private window's search provider is ddg.
current_incognito_dse = incognito_url_service->GetDefaultSearchProvider();
EXPECT_EQ(current_incognito_dse->data().short_name(),
base::ASCIIToUTF16("DuckDuckGo"));
EXPECT_EQ(TemplateURL::NORMAL, current_incognito_dse->type());
}
#endif

} // namespace extensions

#endif // ENABLE_EXTENSIONS
Loading

0 comments on commit f91c501

Please sign in to comment.