Skip to content

Commit

Permalink
Modify CookieMonster's constructors to pass in a NetLog*
Browse files Browse the repository at this point in the history
This CL adds a NetLog* parameter to CookieMonster's constructors, so we can use
it to log NetLog events that are related to setting cookies.

This CL is split off from rdsmith@'s CL at
https://chromium-review.googlesource.com/c/chromium/src/+/925181

Left TODOs to hook up headless and iOS. I will upload followup CLs to address
those.

TBR=pfeldman@chromium.org
TBR=slan@chromium.org

Bug: 801910
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;luci.chromium.try:linux_mojo;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ibd76217b45326e1763263cdda391d9d584c458bf
Reviewed-on: https://chromium-review.googlesource.com/1171113
Commit-Queue: Helen Li <xunjieli@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Tao Bai <michaelbai@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582923}
  • Loading branch information
xunjieli authored and Commit Bot committed Aug 14, 2018
1 parent 1fb00d5 commit fb313a9
Show file tree
Hide file tree
Showing 25 changed files with 221 additions and 113 deletions.
2 changes: 1 addition & 1 deletion android_webview/browser/cookie_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ net::CookieStore* CookieManager::GetCookieStore() {
cookie_store_created_ = true;
}

cookie_store_ = content::CreateCookieStore(cookie_config);
cookie_store_ = content::CreateCookieStore(cookie_config, nullptr);
}

return cookie_store_.get();
Expand Down
7 changes: 4 additions & 3 deletions chrome/browser/profiles/off_the_record_profile_io_data.cc
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,8 @@ void OffTheRecordProfileIOData::
content::CookieStoreConfig cookie_config;
// Enable cookies for chrome-extension URLs.
cookie_config.cookieable_schemes.push_back(extensions::kExtensionScheme);
extensions_cookie_store_ = content::CreateCookieStore(cookie_config);
extensions_cookie_store_ = content::CreateCookieStore(
cookie_config, profile_params->io_thread->net_log());
extensions_context->set_cookie_store(extensions_cookie_store_.get());
}

Expand All @@ -256,8 +257,8 @@ net::URLRequestContext* OffTheRecordProfileIOData::InitializeAppRequestContext(
// Use a separate in-memory cookie store for the app.
// TODO(creis): We should have a cookie delegate for notifying the cookie
// extensions API, but we need to update it to understand isolated apps first.
std::unique_ptr<net::CookieStore> cookie_store =
content::CreateCookieStore(content::CookieStoreConfig());
std::unique_ptr<net::CookieStore> cookie_store = content::CreateCookieStore(
content::CookieStoreConfig(), main_context->net_log());
std::unique_ptr<net::ChannelIDService> channel_id_service(
new net::ChannelIDService(new net::DefaultChannelIDStore(nullptr)));
cookie_store->SetChannelIDServiceID(channel_id_service->GetUniqueID());
Expand Down
6 changes: 4 additions & 2 deletions chrome/browser/profiles/profile_impl_io_data.cc
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,8 @@ void ProfileImplIOData::
// Enable cookies for chrome-extension URLs.
cookie_config.cookieable_schemes.push_back(extensions::kExtensionScheme);
cookie_config.channel_id_service = extensions_context->channel_id_service();
extensions_cookie_store_ = content::CreateCookieStore(cookie_config);
extensions_cookie_store_ = content::CreateCookieStore(
cookie_config, profile_params->io_thread->net_log());
extensions_context->set_cookie_store(extensions_cookie_store_.get());
}

Expand Down Expand Up @@ -584,7 +585,8 @@ net::URLRequestContext* ProfileImplIOData::InitializeAppRequestContext(
new net::DefaultChannelIDStore(channel_id_db.get())));
cookie_config.channel_id_service = channel_id_service.get();
cookie_config.background_task_runner = cookie_background_task_runner;
cookie_store = content::CreateCookieStore(cookie_config);
cookie_store =
content::CreateCookieStore(cookie_config, main_context->net_log());
cookie_store->SetChannelIDServiceID(channel_id_service->GetUniqueID());

// Build a new HttpNetworkSession that uses the new ChannelIDService.
Expand Down
3 changes: 2 additions & 1 deletion chrome/test/base/testing_profile.cc
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,8 @@ class TestExtensionURLRequestContext : public net::URLRequestContext {
TestExtensionURLRequestContext() {
content::CookieStoreConfig cookie_config;
cookie_config.cookieable_schemes.push_back(extensions::kExtensionScheme);
cookie_store_ = content::CreateCookieStore(cookie_config);
cookie_store_ =
content::CreateCookieStore(cookie_config, nullptr /* netlog */);
set_cookie_store(cookie_store_.get());
}

Expand Down
4 changes: 2 additions & 2 deletions chromecast/browser/url_request_context_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ net::URLRequestContext* URLRequestContextFactory::CreateSystemRequestContext() {
PopulateNetworkSessionParams(IgnoreCertificateErrors(), &session_params);
system_job_factory_.reset(new net::URLRequestJobFactoryImpl());
system_cookie_store_ =
content::CreateCookieStore(content::CookieStoreConfig());
content::CreateCookieStore(content::CookieStoreConfig(), net_log_);

net::URLRequestContext* system_context = new net::URLRequestContext();
system_context->set_host_resolver(host_resolver_.get());
Expand Down Expand Up @@ -405,7 +405,7 @@ net::URLRequestContext* URLRequestContextFactory::CreateMainRequestContext(
protocol_handlers, std::move(request_interceptors));

content::CookieStoreConfig cookie_config(cookie_path, false, true, nullptr);
main_cookie_store_ = content::CreateCookieStore(cookie_config);
main_cookie_store_ = content::CreateCookieStore(cookie_config, net_log_);

net::URLRequestContext* main_context = new net::URLRequestContext();
main_context->set_host_resolver(host_resolver_.get());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ SafeBrowsingURLRequestContextGetter::GetURLRequestContext() {
nullptr);
cookie_config.channel_id_service = channel_id_service_.get();
cookie_config.background_task_runner = background_task_runner;
safe_browsing_cookie_store_ = content::CreateCookieStore(cookie_config);
safe_browsing_cookie_store_ =
content::CreateCookieStore(cookie_config, nullptr /* netlog */);
safe_browsing_request_context_->set_cookie_store(
safe_browsing_cookie_store_.get());

Expand Down
10 changes: 6 additions & 4 deletions content/browser/net/quota_policy_cookie_store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,14 @@ CookieStoreConfig::~CookieStoreConfig() {
}

std::unique_ptr<net::CookieStore> CreateCookieStore(
const CookieStoreConfig& config) {
const CookieStoreConfig& config,
net::NetLog* net_log) {
std::unique_ptr<net::CookieMonster> cookie_monster;

if (config.path.empty()) {
// Empty path means in-memory store.
cookie_monster.reset(new net::CookieMonster(nullptr));
cookie_monster = std::make_unique<net::CookieMonster>(
nullptr /* store */, nullptr /* channel_id_service */, net_log);
} else {
scoped_refptr<base::SequencedTaskRunner> client_task_runner =
config.client_task_runner;
Expand Down Expand Up @@ -100,8 +102,8 @@ std::unique_ptr<net::CookieStore> CreateCookieStore(
sqlite_store.get(),
config.storage_policy.get());

cookie_monster.reset(new net::CookieMonster(persistent_store,
config.channel_id_service));
cookie_monster = std::make_unique<net::CookieMonster>(
persistent_store, config.channel_id_service, net_log);
if (config.persist_session_cookies)
cookie_monster->SetPersistSessionCookies(true);
}
Expand Down
4 changes: 3 additions & 1 deletion content/public/browser/cookie_store_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ namespace net {
class ChannelIDService;
class CookieCryptoDelegate;
class CookieStore;
class NetLog;
}

namespace storage {
Expand Down Expand Up @@ -83,7 +84,8 @@ struct CONTENT_EXPORT CookieStoreConfig {
};

CONTENT_EXPORT std::unique_ptr<net::CookieStore> CreateCookieStore(
const CookieStoreConfig& config);
const CookieStoreConfig& config,
net::NetLog* net_log);

} // namespace content

Expand Down
2 changes: 1 addition & 1 deletion content/shell/browser/shell_url_request_context_getter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ net::URLRequestContext* ShellURLRequestContextGetter::GetURLRequestContext() {
builder.set_net_log(net_log_);
builder.set_network_delegate(CreateNetworkDelegate());
std::unique_ptr<net::CookieStore> cookie_store =
CreateCookieStore(CookieStoreConfig());
CreateCookieStore(CookieStoreConfig(), net_log_);
std::unique_ptr<net::ChannelIDService> channel_id_service =
std::make_unique<net::ChannelIDService>(
new net::DefaultChannelIDStore(nullptr));
Expand Down
3 changes: 2 additions & 1 deletion headless/lib/browser/headless_url_request_context_getter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,9 @@ HeadlessURLRequestContextGetter::GetURLRequestContext() {
user_data_path_.Append(FILE_PATH_LITERAL("Cookies")), false, true,
NULL);
cookie_config.crypto_delegate = cookie_config::GetCookieCryptoDelegate();
// TODO(crbug.com/801910): Hook up logging by passing in a non-null netlog.
std::unique_ptr<net::CookieStore> cookie_store =
CreateCookieStore(cookie_config);
CreateCookieStore(cookie_config, nullptr /* netlog*/);
std::unique_ptr<net::ChannelIDService> channel_id_service =
std::make_unique<net::ChannelIDService>(
new net::DefaultChannelIDStore(nullptr));
Expand Down
10 changes: 7 additions & 3 deletions ios/chrome/browser/net/cookie_util.mm
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,22 @@
// Creates a CookieMonster configured by |config|.
std::unique_ptr<net::CookieMonster> CreateCookieMonster(
const CookieStoreConfig& config) {
// TODO(crbug.com/801910): Hook up logging by passing in a non-null netlog.
if (config.path.empty()) {
// Empty path means in-memory store.
return std::make_unique<net::CookieMonster>(nullptr);
return std::make_unique<net::CookieMonster>(
nullptr /* store */, nullptr /* channel_id_service */,
nullptr /* netlog */);
}

const bool restore_old_session_cookies =
config.session_cookie_mode == CookieStoreConfig::RESTORED_SESSION_COOKIES;
scoped_refptr<net::SQLitePersistentCookieStore> persistent_store =
CreatePersistentCookieStore(config.path, restore_old_session_cookies,
config.crypto_delegate);
std::unique_ptr<net::CookieMonster> cookie_monster(
new net::CookieMonster(persistent_store.get()));
std::unique_ptr<net::CookieMonster> cookie_monster(new net::CookieMonster(
persistent_store.get(), nullptr /* channel_id_service */,
nullptr /* netlog */));
if (restore_old_session_cookies)
cookie_monster->SetPersistSessionCookies(true);
return cookie_monster;
Expand Down
5 changes: 4 additions & 1 deletion ios/components/io_thread/ios_io_thread.mm
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,10 @@
CreateDefaultAuthHandlerFactory();
globals_->http_server_properties.reset(new net::HttpServerPropertiesImpl());
// In-memory cookie store.
globals_->system_cookie_store.reset(new net::CookieMonster(nullptr));
// TODO(crbug.com/801910): Hook up logging by passing in a non-null netlog.
globals_->system_cookie_store.reset(new net::CookieMonster(
nullptr /* store */, nullptr /* channel_id_service */,
nullptr /* netlog */));
// In-memory channel ID store.
globals_->system_channel_id_service.reset(
new net::ChannelIDService(new net::DefaultChannelIDStore(nullptr)));
Expand Down
5 changes: 4 additions & 1 deletion ios/net/cookies/cookie_store_ios.mm
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,10 @@ bool HasExplicitDomain(const std::string& cookie_line) {
CookieStoreIOS::CookieStoreIOS(
net::CookieMonster::PersistentCookieStore* persistent_store,
std::unique_ptr<SystemCookieStore> system_store)
: cookie_monster_(new net::CookieMonster(persistent_store)),
: // TODO(crbug.com/801910): hook up logging by using a non-null netlog.
cookie_monster_(new net::CookieMonster(persistent_store,
nullptr /* channel_id_service */,
nullptr /* netlog */)),
system_store_(std::move(system_store)),
metrics_enabled_(false),
cookie_cache_(new CookieCache()),
Expand Down
25 changes: 11 additions & 14 deletions net/cookies/cookie_monster.cc
Original file line number Diff line number Diff line change
Expand Up @@ -350,33 +350,30 @@ size_t CountCookiesForPossibleDeletion(

} // namespace

CookieMonster::CookieMonster(scoped_refptr<PersistentCookieStore> store)
: CookieMonster(
std::move(store),
nullptr,
base::TimeDelta::FromSeconds(kDefaultAccessUpdateThresholdSeconds)) {}

CookieMonster::CookieMonster(scoped_refptr<PersistentCookieStore> store,
ChannelIDService* channel_id_service)
ChannelIDService* channel_id_service,
NetLog* net_log)
: CookieMonster(
std::move(store),
channel_id_service,
base::TimeDelta::FromSeconds(kDefaultAccessUpdateThresholdSeconds)) {}
base::TimeDelta::FromSeconds(kDefaultAccessUpdateThresholdSeconds),
net_log) {}

CookieMonster::CookieMonster(scoped_refptr<PersistentCookieStore> store,
base::TimeDelta last_access_threshold)
: CookieMonster(std::move(store), nullptr, last_access_threshold) {}
base::TimeDelta last_access_threshold,
NetLog* net_log)
: CookieMonster(std::move(store), nullptr, last_access_threshold, net_log) {
}

CookieMonster::CookieMonster(scoped_refptr<PersistentCookieStore> store,
ChannelIDService* channel_id_service,
base::TimeDelta last_access_threshold)
base::TimeDelta last_access_threshold,
NetLog* net_log)
: initialized_(false),
started_fetching_all_cookies_(false),
finished_fetching_all_cookies_(false),
seen_global_task_(false),
// TODO(https://crbug.com/801910): Hook up Cookies logging by using a
// non-null NetLog.
net_log_(NetLogWithSource::Make(nullptr, NetLogSourceType::COOKIE_STORE)),
net_log_(NetLogWithSource::Make(net_log, NetLogSourceType::COOKIE_STORE)),
store_(std::move(store)),
last_access_threshold_(last_access_threshold),
channel_id_service_(channel_id_service),
Expand Down
18 changes: 10 additions & 8 deletions net/cookies/cookie_monster.h
Original file line number Diff line number Diff line change
Expand Up @@ -131,18 +131,19 @@ class NET_EXPORT CookieMonster : public CookieStore {
// class will take care of initializing it. The backing store is NOT owned by
// this class, but it must remain valid for the duration of the cookie
// monster's existence. If |store| is NULL, then no backing store will be
// updated.
explicit CookieMonster(scoped_refptr<PersistentCookieStore> store);

// Like above, but includes a non-owning pointer |channel_id_service| for the
// updated. |channel_id_service| is a non-owninng pointer for the
// corresponding ChannelIDService used with this CookieStore. The
// |channel_id_service| must outlive the CookieMonster.
// |channel_id_service| must outlive the CookieMonster. |net_log| must outlive
// the CookieMonster. Both |channel_id_service| and |net_log| can be null.
CookieMonster(scoped_refptr<PersistentCookieStore> store,
ChannelIDService* channel_id_service);
ChannelIDService* channel_id_service,
NetLog* net_log);

// Only used during unit testing.
// |net_log| must outlive the CookieMonster.
CookieMonster(scoped_refptr<PersistentCookieStore> store,
base::TimeDelta last_access_threshold);
base::TimeDelta last_access_threshold,
NetLog* net_log);

~CookieMonster() override;

Expand Down Expand Up @@ -214,7 +215,8 @@ class NET_EXPORT CookieMonster : public CookieStore {
private:
CookieMonster(scoped_refptr<PersistentCookieStore> store,
ChannelIDService* channel_id_service,
base::TimeDelta last_access_threshold);
base::TimeDelta last_access_threshold,
NetLog* net_log);

// For garbage collection constants.
FRIEND_TEST_ALL_PREFIXES(CookieMonsterTest, TestHostGarbageCollection);
Expand Down
14 changes: 8 additions & 6 deletions net/cookies/cookie_monster_perftest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ TEST(ParsedCookieTest, TestParseBigCookies) {
}

TEST_F(CookieMonsterTest, TestAddCookiesOnSingleHost) {
std::unique_ptr<CookieMonster> cm(new CookieMonster(nullptr));
auto cm = std::make_unique<CookieMonster>(nullptr, nullptr, nullptr);
std::vector<std::string> cookies;
for (int i = 0; i < kNumCookies; i++) {
cookies.push_back(base::StringPrintf("a%03d=b", i));
Expand Down Expand Up @@ -168,7 +168,7 @@ TEST_F(CookieMonsterTest, TestAddCookiesOnSingleHost) {
}

TEST_F(CookieMonsterTest, TestAddCookieOnManyHosts) {
std::unique_ptr<CookieMonster> cm(new CookieMonster(nullptr));
auto cm = std::make_unique<CookieMonster>(nullptr, nullptr, nullptr);
std::string cookie(kCookieLine);
std::vector<GURL> gurls; // just wanna have ffffuunnn
for (int i = 0; i < kNumCookies; ++i) {
Expand Down Expand Up @@ -201,7 +201,7 @@ TEST_F(CookieMonsterTest, TestAddCookieOnManyHosts) {
}

TEST_F(CookieMonsterTest, TestDomainTree) {
std::unique_ptr<CookieMonster> cm(new CookieMonster(nullptr));
auto cm = std::make_unique<CookieMonster>(nullptr, nullptr, nullptr);
GetCookieListCallback getCookieListCallback;
SetCookieCallback setCookieCallback;
const char domain_cookie_format_tree[] = "a=b; domain=%s";
Expand Down Expand Up @@ -257,7 +257,7 @@ TEST_F(CookieMonsterTest, TestDomainTree) {
}

TEST_F(CookieMonsterTest, TestDomainLine) {
std::unique_ptr<CookieMonster> cm(new CookieMonster(nullptr));
auto cm = std::make_unique<CookieMonster>(nullptr, nullptr, nullptr);
SetCookieCallback setCookieCallback;
GetCookieListCallback getCookieListCallback;
std::vector<std::string> domain_list;
Expand Down Expand Up @@ -318,7 +318,8 @@ TEST_F(CookieMonsterTest, TestImport) {

store->SetLoadExpectation(true, std::move(initial_cookies));

std::unique_ptr<CookieMonster> cm(new CookieMonster(store.get()));
std::unique_ptr<CookieMonster> cm(
new CookieMonster(store.get(), nullptr, nullptr));

// Import will happen on first access.
GURL gurl("www.foo.com");
Expand All @@ -332,7 +333,8 @@ TEST_F(CookieMonsterTest, TestImport) {
}

TEST_F(CookieMonsterTest, TestGetKey) {
std::unique_ptr<CookieMonster> cm(new CookieMonster(nullptr));
std::unique_ptr<CookieMonster> cm(
new CookieMonster(nullptr, nullptr, nullptr));
base::PerfTimeLogger timer("Cookie_monster_get_key");
for (int i = 0; i < kNumCookies; i++)
cm->GetKey("www.foo.com");
Expand Down
2 changes: 1 addition & 1 deletion net/cookies/cookie_monster_store_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ std::unique_ptr<CookieMonster> CreateMonsterFromStoreForGC(
store->AddCookie(*cc);
}

return std::make_unique<CookieMonster>(store.get());
return std::make_unique<CookieMonster>(store.get(), nullptr, nullptr);
}

MockSimplePersistentCookieStore::~MockSimplePersistentCookieStore() = default;
Expand Down
Loading

0 comments on commit fb313a9

Please sign in to comment.