Skip to content

Commit

Permalink
Split cache per request's includeCredentials
Browse files Browse the repository at this point in the history 
whatwg/fetch issue:
whatwg/fetch#1253

Design doc:
https://docs.google.com/document/d/1lvbiy4n-GM5I56Ncw304sgvY5Td32R6KHitjRXvkZ6U/edit#

Add the feature "SplitCacheByIncludeCredentials".
When enabled, it makes the HTTP cache to differentiate the requests sent
anonymously from the ones sent with credentials.

This avoids anonymous requests from getting responses requested with
credentials.

This aligns Chrome toward Firefox. See Firefox implementations:
https://github.com/mozilla/gecko-dev/blob/b31b78eea683b0eb341c676adb422cd129909fe9/netwerk/protocol/http/nsHttpChannel.cpp#L4117

Goal after this patch is to start a finch experiment to check for
potential performance regressions.

VIRTUAL_OWNERS: This adds 13 tests from http-cache WPT directory to be
run with the feature enabled. Tests are often "any.js" tests, causing
46 run.

Bug: 1221529
Change-Id: I0f1c969eb2c3401e9548c5d1e6ec63570166d7e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3010379
Auto-Submit: Arthur Sonzogni <[email protected]>
Reviewed-by: Mason Freed <[email protected]>
Reviewed-by: Mike West <[email protected]>
Reviewed-by: Camille Lamy <[email protected]>
Reviewed-by: Maksim Orlovich <[email protected]>
Commit-Queue: Arthur Sonzogni <[email protected]>
Cr-Commit-Position: refs/heads/main@{#918101}
  • Loading branch information
@ArthurSonzogni
ArthurSonzogni authored and Chromium LUCI CQ committed Sep 3, 2021
1 parent 9ed0f65 commit 54013f2
Show file tree
Hide file tree
Showing 31 changed files with 575 additions and 132 deletions.
7 changes: 6 additions & 1 deletion chrome/browser/net/profile_network_context_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,12 @@ bool GetHttpCacheBackendResetParam(PrefService* local_state) {
// This used to be for keying on scheme + eTLD+1 vs origin, but the trial was
// removed, and now it's always keyed on eTLD+1. Still keeping a third "None"
// to avoid resetting the disk cache.
current_field_trial_status += " None";
current_field_trial_status += " None ";

field_trial = base::FeatureList::GetFieldTrial(
net::features::kSplitCacheByIncludeCredentials);
current_field_trial_status +=
(field_trial ? field_trial->group_name() : "None");

std::string previous_field_trial_status =
local_state->GetString(kHttpCacheFinchExperimentGroups);
Expand Down
64 changes: 57 additions & 7 deletions chrome/browser/net/profile_network_context_service_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ IN_PROC_BROWSER_TEST_F(ProfileNetworkContextServiceCacheSameBrowsertest,
DCHECK_EQ(
local_state->GetString(
"profile_network_context_service.http_cache_finch_experiment_groups"),
"None None None");
"None None None None");
}

IN_PROC_BROWSER_TEST_F(ProfileNetworkContextServiceCacheSameBrowsertest,
Expand All @@ -288,7 +288,7 @@ IN_PROC_BROWSER_TEST_F(ProfileNetworkContextServiceCacheSameBrowsertest,
DCHECK_EQ(
local_state->GetString(
"profile_network_context_service.http_cache_finch_experiment_groups"),
"None None None");
"None None None None");
}

class ProfileNetworkContextServiceCacheChangeBrowsertest
Expand Down Expand Up @@ -319,15 +319,15 @@ IN_PROC_BROWSER_TEST_F(ProfileNetworkContextServiceCacheChangeBrowsertest,
DCHECK_EQ(
local_state->GetString(
"profile_network_context_service.http_cache_finch_experiment_groups"),
"scoped_feature_list_trial_group None None");
"scoped_feature_list_trial_group None None None");
// Set the local state for the next test.
local_state->SetString(
"profile_network_context_service.http_cache_finch_experiment_groups",
"None None None");
"None None None None");
}

// The second time we load we know the state, which was "None None None" for the
// previous test, so we should see a reset being in an experiment.
// The second time we load we know the state, which was "None None None None"
// for the previous test, so we should see a reset being in an experiment.
IN_PROC_BROWSER_TEST_F(ProfileNetworkContextServiceCacheChangeBrowsertest,
TestCacheResetParameter) {
NavigateToCreateHttpCache();
Expand All @@ -339,7 +339,57 @@ IN_PROC_BROWSER_TEST_F(ProfileNetworkContextServiceCacheChangeBrowsertest,
DCHECK_EQ(
local_state->GetString(
"profile_network_context_service.http_cache_finch_experiment_groups"),
"scoped_feature_list_trial_group None None");
"scoped_feature_list_trial_group None None None");
}

// This subclass adds the "SplitCacheByIncludeCredentials" feature.
class ProfileNetworkContextServiceCacheCredentialsBrowserTest
: public ProfileNetworkContextServiceBrowsertest {
public:
ProfileNetworkContextServiceCacheCredentialsBrowserTest() {
scoped_feature_list_.InitAndEnableFeatureWithParameters(
net::features::kSplitCacheByIncludeCredentials, {});
}
~ProfileNetworkContextServiceCacheCredentialsBrowserTest() override = default;

base::HistogramTester histograms_;

private:
base::test::ScopedFeatureList scoped_feature_list_;
};

IN_PROC_BROWSER_TEST_F(ProfileNetworkContextServiceCacheCredentialsBrowserTest,
PRE_TestCacheResetParameter) {
NavigateToCreateHttpCache();
CheckCacheResetStatus(&histograms_, false);

// At this point, we have already called the initialization.
// Verify that we have the correct values in the local_state.
PrefService* local_state = g_browser_process->local_state();
DCHECK_EQ(
local_state->GetString(
"profile_network_context_service.http_cache_finch_experiment_groups"),
"None None None scoped_feature_list_trial_group");
// Set the local state for the next test.
local_state->SetString(
"profile_network_context_service.http_cache_finch_experiment_groups",
"None None None None");
}

// The second time we load we know the state, which was "None None None None"
// for the previous test, so we should see a reset being in an experiment.
IN_PROC_BROWSER_TEST_F(ProfileNetworkContextServiceCacheCredentialsBrowserTest,
TestCacheResetParameter) {
NavigateToCreateHttpCache();
CheckCacheResetStatus(&histograms_, true);

// At this point, we have already called the initialization once.
// Verify that we have the correct values in the local_state.
PrefService* local_state = g_browser_process->local_state();
DCHECK_EQ(
local_state->GetString(
"profile_network_context_service.http_cache_finch_experiment_groups"),
"None None None scoped_feature_list_trial_group");
}

class AmbientAuthenticationTestWithPolicy : public policy::PolicyTest {
Expand Down
122 changes: 104 additions & 18 deletions content/browser/loader/prefetch_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,14 @@ namespace content {

class PrefetchBrowserTest
: public PrefetchBrowserTestBase,
public testing::WithParamInterface<std::tuple<bool, bool>> {
public testing::WithParamInterface<std::tuple<bool, bool, bool>> {
public:
PrefetchBrowserTest()
: cross_origin_server_(std::make_unique<net::EmbeddedTestServer>()),
signed_exchange_enabled_(std::get<0>(GetParam())),
split_cache_enabled_(std::get<1>(GetParam())) {}
~PrefetchBrowserTest() = default;
split_cache_enabled_(std::get<1>(GetParam())),
split_cache_by_credentials_enabled_(std::get<2>(GetParam())) {}
~PrefetchBrowserTest() override = default;

void SetUpOnMainThread() override {
PrefetchBrowserTestBase::SetUpOnMainThread();
Expand All @@ -52,19 +53,13 @@ class PrefetchBrowserTest
void SetUp() override {
std::vector<base::Feature> enable_features;
std::vector<base::Feature> disabled_features;
if (signed_exchange_enabled_) {
enable_features.push_back(features::kSignedHTTPExchange);
} else {
disabled_features.push_back(features::kSignedHTTPExchange);
}

if (split_cache_enabled_) {
enable_features.push_back(
net::features::kSplitCacheByNetworkIsolationKey);
} else {
disabled_features.push_back(
net::features::kSplitCacheByNetworkIsolationKey);
}
(signed_exchange_enabled_ ? enable_features : disabled_features)
.push_back(features::kSignedHTTPExchange);
(split_cache_enabled_ ? enable_features : disabled_features)
.push_back(net::features::kSplitCacheByNetworkIsolationKey);
(split_cache_by_credentials_enabled_ ? enable_features : disabled_features)
.push_back(net::features::kSplitCacheByIncludeCredentials);

feature_list_.InitWithFeatures(enable_features, disabled_features);
PrefetchBrowserTestBase::SetUp();
Expand All @@ -74,6 +69,7 @@ class PrefetchBrowserTest
std::unique_ptr<net::EmbeddedTestServer> cross_origin_server_;
const bool signed_exchange_enabled_;
const bool split_cache_enabled_;
const bool split_cache_by_credentials_enabled_;

private:
base::test::ScopedFeatureList feature_list_;
Expand Down Expand Up @@ -712,7 +708,10 @@ IN_PROC_BROWSER_TEST_P(PrefetchBrowserTest,
.IsEqualForTesting(request->trusted_params->isolation_info));
}

IN_PROC_BROWSER_TEST_P(PrefetchBrowserTest, CrossOriginWithPreload) {
// Variants of this test:
// - PrefetchBrowserTest.CrossOriginWithPreloadAnonymous
// - PrefetchBrowserTest.CrossOriginWithPreloadCredentialled
IN_PROC_BROWSER_TEST_P(PrefetchBrowserTest, CrossOriginWithPreloadAnonymous) {
const char* target_path = "/target.html";
const char* preload_path = "/preload.js";
RegisterResponse(
Expand Down Expand Up @@ -802,6 +801,13 @@ IN_PROC_BROWSER_TEST_P(PrefetchBrowserTest, CrossOriginWithPreload) {

// We won't need this server again.
EXPECT_TRUE(other_cross_origin_server->ShutdownAndWaitUntilComplete());
} else if (split_cache_by_credentials_enabled_) {
// The navigation is requested with credentials, but the preload is
// requested anonymously. As a result of "SplitCacheByIncludeCredentials",
// those aren't considered the same for the HTTP cache. Early return.
// See the variant of this test in:
// PrefetchBrowserTest.CrossOriginWithPreloadAnonymous
return;
}

// Shutdown the servers.
Expand All @@ -813,6 +819,84 @@ IN_PROC_BROWSER_TEST_P(PrefetchBrowserTest, CrossOriginWithPreload) {
NavigateToURLAndWaitTitle(cross_origin_target_url, "done");
}

// Variants of this test:
// - PrefetchBrowserTest.CrossOriginWithPreloadAnonymous
// - PrefetchBrowserTest.CrossOriginWithPreloadCredentialled
IN_PROC_BROWSER_TEST_P(PrefetchBrowserTest,
CrossOriginWithPreloadCredentialled) {
const char target_path[] = "/target.html";
const char preload_path[] = "/preload.js";
RegisterResponse(
target_path,
ResponseEntry("<head><title>Prefetch Target</title><script "
"src=\"./preload.js\"></script></head>",
"text/html",
{
{
"link",
"</preload.js>;rel=\"preload\";as=\"script\"",
},
{
"Access-Control-Allow-Origin",
"http://prefetch.com:22072",
},
{
"Access-Control-Allow-Credentials",
"true",
},
}));
RegisterResponse(preload_path,
ResponseEntry("document.title=\"done\";", "text/javascript",
{{"cache-control", "public, max-age=600"}}));

base::RunLoop preload_waiter;
auto target_request_counter =
RequestCounter::CreateAndMonitor(cross_origin_server_.get(), target_path);
auto preload_request_counter = RequestCounter::CreateAndMonitor(
cross_origin_server_.get(), preload_path, &preload_waiter);
RegisterRequestHandler(cross_origin_server_.get());
base::RunLoop preload_waiter_second_request;
auto preload_request_counter_second_request =
RequestCounter::CreateAndMonitor(cross_origin_server_.get(), preload_path,
&preload_waiter_second_request);

ASSERT_TRUE(cross_origin_server_->Start());

const GURL cross_origin_target_url =
cross_origin_server_->GetURL("3p.example", target_path);

const char* prefetch_path = "/prefetch.html";
RegisterResponse(prefetch_path,
ResponseEntry(base::StringPrintf(
"<body><link rel='prefetch' href='%s' as='document' "
"crossorigin='use-credentials'></body>",
cross_origin_target_url.spec().c_str())));
RegisterRequestHandler(embedded_test_server());
ASSERT_TRUE(embedded_test_server()->Start(22072));
EXPECT_EQ(0, GetPrefetchURLLoaderCallCount());

// Loading a page that prefetches the target URL would increment both
// |target_request_counter| and |preload_request_counter|.
EXPECT_TRUE(NavigateToURL(
shell(), embedded_test_server()->GetURL("prefetch.com", prefetch_path)));
preload_waiter.Run();
EXPECT_EQ(1, target_request_counter->GetRequestCount());
EXPECT_EQ(1, preload_request_counter->GetRequestCount());
EXPECT_EQ(2, GetPrefetchURLLoaderCallCount());

GURL cross_origin_preload_url =
cross_origin_server_->GetURL("3p.example", preload_path);
WaitUntilLoaded(cross_origin_preload_url);

// Shutdown the servers.
EXPECT_TRUE(embedded_test_server()->ShutdownAndWaitUntilComplete());
EXPECT_TRUE(cross_origin_server_->ShutdownAndWaitUntilComplete());

// Subsequent navigation to the target URL wouldn't hit the network for
// the target URL. The target content should still be read correctly.
NavigateToURLAndWaitTitle(cross_origin_target_url, "done");
}

IN_PROC_BROWSER_TEST_P(PrefetchBrowserTest, SignedExchangeWithPreload) {
const char* prefetch_path = "/prefetch.html";
const char* target_sxg_path = "/target.sxg";
Expand Down Expand Up @@ -1006,10 +1090,12 @@ IN_PROC_BROWSER_TEST_P(PrefetchBrowserTest, FileToHttp) {

INSTANTIATE_TEST_SUITE_P(PrefetchBrowserTest,
PrefetchBrowserTest,
testing::Combine(testing::Bool(), testing::Bool()));
testing::Combine(testing::Bool(),
testing::Bool(),
testing::Bool()));

INSTANTIATE_TEST_SUITE_P(PrefetchBrowserTestPrivacyChanges,
PrefetchBrowserTestPrivacyChanges,
testing::Values(false, true));
testing::Bool());

} // namespace content
Loading

0 comments on commit 54013f2

Please sign in to comment.