Skip to content

Commit

Permalink
Fix 6804: Avoid the OnPreferenceChanged callback when preferences are…
Browse files Browse the repository at this point in the history 
… changed by CookiePrefService
  • Loading branch information
@jumde
jumde committed Nov 7, 2019
1 parent 16a085b commit d4d5ec8
Show file tree
Hide file tree
Showing 4 changed files with 217 additions and 3 deletions.
170 changes: 170 additions & 0 deletions browser/net/brave_network_delegate_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,14 @@
#include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/ui_test_utils.h"
#include "components/content_settings/core/browser/host_content_settings_map.h"
#include "components/content_settings/core/common/pref_names.h"
#include "components/prefs/pref_service.h"
#include "content/public/test/browser_test_utils.h"
#include "net/dns/mock_host_resolver.h"
#include "url/gurl.h"

using net::test_server::EmbeddedTestServer;

class BraveNetworkDelegateBrowserTest : public InProcessBrowserTest {
public:
void SetUpOnMainThread() override {
Expand Down Expand Up @@ -53,6 +57,18 @@ class BraveNetworkDelegateBrowserTest : public InProcessBrowserTest {
GURL());
}

void BlockThirdPartyCookies() {
brave_shields::SetCookieControlType(browser()->profile(),
brave_shields::ControlType::BLOCK_THIRD_PARTY,
GURL());
}

void AllowAllCookies() {
brave_shields::SetCookieControlType(browser()->profile(),
brave_shields::ControlType::ALLOW,
GURL());
}

void AllowCookies() {
brave_shields::SetCookieControlType(browser()->profile(),
brave_shields::ControlType::ALLOW,
Expand All @@ -71,6 +87,28 @@ class BraveNetworkDelegateBrowserTest : public InProcessBrowserTest {
top_level_page_url_);
}

void NavigateToPageWithFrame(const EmbeddedTestServer *server,
const std::string& host) {
ui_test_utils::NavigateToURL(browser(),
server->GetURL(host, "/cookie_iframe.html"));
}

void ExpectCookiesOnHost(const EmbeddedTestServer *server,
const std::string& host,
const std::string& expected) {
EXPECT_EQ(expected, content::GetCookies(browser()->profile(),
server->GetURL(host, "/")));
}

void NavigateFrameTo(const EmbeddedTestServer *server,
const std::string& host,
const std::string& path) {
GURL page = server->GetURL(host, path);
content::WebContents* web_contents =
browser()->tab_strip_model()->GetActiveWebContents();
EXPECT_TRUE(NavigateIframeToURL(web_contents, "test", page));
}

protected:
GURL url_;
GURL nested_iframe_script_url_;
Expand Down Expand Up @@ -152,3 +190,135 @@ IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest,
content::GetCookies(browser()->profile(), GURL("http://a.com/"));
EXPECT_TRUE(cookie.empty()) << "Actual cookie: " << cookie;
}

IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest,
PrefToggleBlockAllToBlockThirdParty) {
DefaultBlockAllCookies();
BlockThirdPartyCookies();

EXPECT_TRUE(browser()->profile()->GetPrefs()->
GetBoolean(prefs::kBlockThirdPartyCookies));
EXPECT_EQ(browser()->profile()->GetPrefs()->
GetInteger("profile.default_content_setting_values.cookies"),
ContentSetting::CONTENT_SETTING_ALLOW);

NavigateToPageWithFrame(embedded_test_server(),
"a.com");

NavigateFrameTo(embedded_test_server(),
"b.com",
"/set-cookie?name=Good");

ExpectCookiesOnHost(embedded_test_server(), "a.com", "name=Good");
ExpectCookiesOnHost(embedded_test_server(), "b.com", "");
}

IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest,
PrefToggleBlockAllToAllowAll) {
DefaultBlockAllCookies();
AllowAllCookies();

EXPECT_FALSE(browser()->profile()->GetPrefs()->
GetBoolean(prefs::kBlockThirdPartyCookies));
EXPECT_EQ(browser()->profile()->GetPrefs()->
GetInteger("profile.default_content_setting_values.cookies"),
ContentSetting::CONTENT_SETTING_ALLOW);

NavigateToPageWithFrame(embedded_test_server(),
"a.com");

NavigateFrameTo(embedded_test_server(),
"b.com",
"/set-cookie?name=Good");

ExpectCookiesOnHost(embedded_test_server(), "a.com", "name=Good");
ExpectCookiesOnHost(embedded_test_server(), "b.com", "name=Good");
}

IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest,
PrefToggleBlockThirdPartyToAllowAll) {
BlockThirdPartyCookies();
AllowAllCookies();

EXPECT_FALSE(browser()->profile()->GetPrefs()->
GetBoolean(prefs::kBlockThirdPartyCookies));
EXPECT_EQ(browser()->profile()->GetPrefs()->
GetInteger("profile.default_content_setting_values.cookies"),
ContentSetting::CONTENT_SETTING_ALLOW);

NavigateToPageWithFrame(embedded_test_server(),
"a.com");

NavigateFrameTo(embedded_test_server(),
"b.com",
"/set-cookie?name=Good");

ExpectCookiesOnHost(embedded_test_server(), "a.com", "name=Good");
ExpectCookiesOnHost(embedded_test_server(), "b.com", "name=Good");
}

IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest,
PrefToggleBlockThirdPartyToBlockAll) {
BlockThirdPartyCookies();
DefaultBlockAllCookies();

EXPECT_FALSE(browser()->profile()->GetPrefs()->
GetBoolean(prefs::kBlockThirdPartyCookies));
EXPECT_EQ(browser()->profile()->GetPrefs()->
GetInteger("profile.default_content_setting_values.cookies"),
ContentSetting::CONTENT_SETTING_BLOCK);

NavigateToPageWithFrame(embedded_test_server(),
"a.com");

NavigateFrameTo(embedded_test_server(),
"b.com",
"/set-cookie?name=Good");

ExpectCookiesOnHost(embedded_test_server(), "a.com", "");
ExpectCookiesOnHost(embedded_test_server(), "b.com", "");
}

IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest,
PrefToggleAllowAllToBlockThirdParty) {
AllowAllCookies();
BlockThirdPartyCookies();

EXPECT_TRUE(browser()->profile()->GetPrefs()->
GetBoolean(prefs::kBlockThirdPartyCookies));
EXPECT_EQ(browser()->profile()->GetPrefs()->
GetInteger("profile.default_content_setting_values.cookies"),
ContentSetting::CONTENT_SETTING_ALLOW);

NavigateToPageWithFrame(embedded_test_server(),
"a.com");

NavigateFrameTo(embedded_test_server(),
"b.com",
"/set-cookie?name=Good");

ExpectCookiesOnHost(embedded_test_server(), "a.com", "name=Good");
ExpectCookiesOnHost(embedded_test_server(), "b.com", "");
}

IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest,
PrefToggleAllowAllToBlockAll) {
AllowAllCookies();
DefaultBlockAllCookies();

EXPECT_FALSE(browser()->profile()->GetPrefs()->
GetBoolean(prefs::kBlockThirdPartyCookies));
EXPECT_EQ(browser()->profile()->GetPrefs()->
GetInteger("profile.default_content_setting_values.cookies"),
ContentSetting::CONTENT_SETTING_BLOCK);

NavigateToPageWithFrame(embedded_test_server(),
"a.com");

NavigateFrameTo(embedded_test_server(),
"b.com",
"/set-cookie?name=Good");

ExpectCookiesOnHost(embedded_test_server(), "a.com", "");
ExpectCookiesOnHost(embedded_test_server(), "b.com", "");
}
30 changes: 27 additions & 3 deletions components/brave_shields/browser/cookie_pref_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,28 @@ void SetCookiePrefDefaults(HostContentSettingsMap* map, PrefService* prefs) {

} // namespace

CookiePrefService::Lock::Lock() : locked_(false) {}

CookiePrefService::Lock::~Lock() {}

bool CookiePrefService::Lock::Try() {
if (locked_)
return false;

locked_ = true;
return locked_;
}

void CookiePrefService::Lock::Release() {
DCHECK(locked_);
locked_ = false;
}

CookiePrefService::CookiePrefService(
HostContentSettingsMap* host_content_settings_map,
PrefService* prefs)
: host_content_settings_map_(host_content_settings_map), prefs_(prefs) {
: host_content_settings_map_(host_content_settings_map),
prefs_(prefs) {
SetCookiePrefDefaults(host_content_settings_map, prefs);
host_content_settings_map_->AddObserver(this);
pref_change_registrar_.Init(prefs_);
Expand All @@ -79,7 +97,10 @@ CookiePrefService::~CookiePrefService() {
}

void CookiePrefService::OnPreferenceChanged() {
SetCookieControlTypeFromPrefs(host_content_settings_map_, prefs_);
if (lock_.Try()) {
SetCookieControlTypeFromPrefs(host_content_settings_map_, prefs_);
lock_.Release();
}
}

void CookiePrefService::OnContentSettingChanged(
Expand All @@ -91,7 +112,10 @@ void CookiePrefService::OnContentSettingChanged(
secondary_pattern == ContentSettingsPattern::Wildcard() &&
content_type == CONTENT_SETTINGS_TYPE_PLUGINS &&
resource_identifier == brave_shields::kCookies) {
SetCookiePrefDefaults(host_content_settings_map_, prefs_);
if (lock_.Try()) {
SetCookiePrefDefaults(host_content_settings_map_, prefs_);
lock_.Release();
}
}
}

Expand Down
15 changes: 15 additions & 0 deletions components/brave_shields/browser/cookie_pref_service.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include <string>

#include "base/macros.h"
#include "components/keyed_service/core/keyed_service.h"
#include "components/content_settings/core/browser/content_settings_observer.h"
#include "components/prefs/pref_change_registrar.h"
Expand All @@ -27,6 +28,17 @@ class CookiePrefService : public KeyedService,
~CookiePrefService() override;

private:
class Lock {
public:
Lock();
~Lock();
bool Try();
void Release();
private:
bool locked_;
DISALLOW_COPY_AND_ASSIGN(Lock);
};

void OnPreferenceChanged();

// content_settings::Observer overrides:
Expand All @@ -36,9 +48,12 @@ class CookiePrefService : public KeyedService,
ContentSettingsType content_type,
const std::string& resource_identifier) override;

Lock lock_;
HostContentSettingsMap* host_content_settings_map_;
PrefService* prefs_;
PrefChangeRegistrar pref_change_registrar_;

DISALLOW_COPY_AND_ASSIGN(CookiePrefService);
};

} // namespace brave_shields
Expand Down
5 changes: 5 additions & 0 deletions test/data/cookie_iframe.html
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<html><head><title>iframe test</title></head>
<body>
<script src="/cross-site/a.com/script_cookies.js"></script>
<iframe src="simple.html" id="test"></iframe>
</body></html>

0 comments on commit d4d5ec8

Please sign in to comment.