Skip to content

Commit

Permalink
CCNS: Ignore the cookie changes made by the navigation request
Browse files Browse the repository at this point in the history 
In https://crrev.com/c/4235289, we move the cookie change listener
to NavigationRequest, which will observe all the cookie modification
since the start of the navigation until the end of the document's life
time. The cookie modification will be used as a signal indicating the
necessity of inactive document destruction, so we should exclude the
ones made by the navigation request itself.

In this CL, instead of just keeping two boolean values in
`CookieChangeListener`, we use two counters instead, which makes it
possible to decrement the number of cookie modification observed from
`CookieAccessObserver`.

Bug: 1399741
Change-Id: I567816a060b2548bee34a4a7741af553331d2974
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4328755
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Commit-Queue: Mingyu Lei <leimy@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1116265}
  • Loading branch information
@lozy219
lozy219 authored and Chromium LUCI CQ committed Mar 13, 2023
1 parent a225dbd commit 7728014
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 43 deletions.
67 changes: 33 additions & 34 deletions content/browser/back_forward_cache_no_store_browsertest.cc
View file
Expand Up @@ -1158,8 +1158,8 @@ IN_PROC_BROWSER_TEST_F(
BlockListedFeatures()));
}

// Test that a page with cache-control:no-store gets evicted if a cookie is set
// when the page is loaded.
// Test that a page with cache-control:no-store gets restored if the only cookie
// modification comes from the response of the `NavigationRequest`.
IN_PROC_BROWSER_TEST_F(
BackForwardCacheBrowserTestRestoreCacheControlNoStoreUnlessCookieChange,
PagesWithCacheControlNoStoreNotBFCachedWithCookieSetInResponse) {
Expand Down Expand Up @@ -1187,15 +1187,9 @@ IN_PROC_BROWSER_TEST_F(
EXPECT_TRUE(NavigateToURL(shell(), url_b));
EXPECT_TRUE(rfh_a->IsInBackForwardCache());

// 3) Go back. |rfh_a| should be evicted upon restoration.
// 3) Go back. |rfh_a| should be restored.
ASSERT_TRUE(HistoryGoBack(web_contents()));
ExpectNotRestored({NotRestoredReason::kCacheControlNoStoreCookieModified}, {},
{}, {}, {}, FROM_HERE);
EXPECT_THAT(GetTreeResult()->GetDocumentResult(),
MatchesDocumentResult(
NotRestoredReasons(
NotRestoredReason::kCacheControlNoStoreCookieModified),
BlockListedFeatures()));
ExpectRestored(FROM_HERE);
}

// Test that a page with `Cache-control: no-store` header gets evicted if some
Expand Down Expand Up @@ -1621,8 +1615,8 @@ IN_PROC_BROWSER_TEST_F(
BlockListedFeatures()));
}

// Test that a page with cache-control:no-store gets evicted if an HTTPOnly
// cookie is set when the page is loaded.
// Test that a page with cache-control:no-store gets restored if the HTTPOnly
// cookie modification comes from the response of the `NavigationRequest`.
IN_PROC_BROWSER_TEST_F(
BackForwardCacheBrowserTestRestoreUnlessHTTPOnlyCookieChange,
PagesWithCacheControlNoStoreNotBFCachedWithHTTPOnlyCookieSetInResponse) {
Expand Down Expand Up @@ -1650,17 +1644,9 @@ IN_PROC_BROWSER_TEST_F(
EXPECT_TRUE(NavigateToURL(shell(), url_b));
EXPECT_TRUE(rfh_a->IsInBackForwardCache());

// 3) Go back. |rfh_a| should be evicted upon restoration.
// 3) Go back. |rfh_a| should be restored.
ASSERT_TRUE(HistoryGoBack(web_contents()));
ExpectNotRestored(
{NotRestoredReason::kCacheControlNoStoreHTTPOnlyCookieModified}, {}, {},
{}, {}, FROM_HERE);
EXPECT_THAT(
GetTreeResult()->GetDocumentResult(),
MatchesDocumentResult(
NotRestoredReasons(
NotRestoredReason::kCacheControlNoStoreHTTPOnlyCookieModified),
BlockListedFeatures()));
ExpectRestored(FROM_HERE);
}

// Test that a page with cache-control:no-store gets evicted if some HTTPOnly
Expand Down Expand Up @@ -1726,38 +1712,51 @@ IN_PROC_BROWSER_TEST_F(
CreateHttpsServer();
net::test_server::ControllableHttpResponse response(https_server(),
"/title1.html");
net::test_server::ControllableHttpResponse response2(https_server(),
"/title1.html");

ASSERT_TRUE(https_server()->Start());

GURL url_a(https_server()->GetURL("a.com", "/title1.html"));
GURL url_a2(https_server()->GetURL("a.com", "/title1.html#foo"));
GURL url_b(https_server()->GetURL("b.com", "/title1.html"));

// 1) Load the document and specify no-store for the main resource, the
// response also sets a cookie.
TestNavigationObserver observer(web_contents());
shell()->LoadURL(url_a);
Shell* tab_to_be_bfcached = shell();
Shell* tab_to_modify_cookie = CreateBrowser();

// 1) Load the document and specify no-store for the main resource.
TestNavigationObserver observer(tab_to_be_bfcached->web_contents());
tab_to_be_bfcached->LoadURL(url_a);
RenderFrameHostImplWrapper rfh_a(current_frame_host());
response.WaitForRequest();
response.Send(kResponseWithNoCacheWithHTTPOnlyCookie);
response.Send(kResponseWithNoCache);
response.Done();
observer.Wait();
rfh_a->GetBackForwardCacheMetrics()->SetObserverForTesting(this);

// 2) Perform a same document navigation.
// 2) Modify the HTTPOnly cookie from another tab.
TestNavigationObserver observer2(tab_to_modify_cookie->web_contents());
tab_to_modify_cookie->LoadURL(url_a);
response2.WaitForRequest();
response2.Send(kResponseWithNoCacheWithHTTPOnlyCookie);
response2.Done();
observer2.Wait();

// 3) Perform a same document navigation.
EXPECT_TRUE(ExecJs(shell(), JsReplace("location = $1", url_a2.spec())));
EXPECT_TRUE(WaitForLoadStop(web_contents()));
EXPECT_EQ(url_a2,
web_contents()->GetPrimaryMainFrame()->GetLastCommittedURL());
EXPECT_TRUE(WaitForLoadStop(tab_to_be_bfcached->web_contents()));
EXPECT_EQ(url_a2, tab_to_be_bfcached->web_contents()
->GetPrimaryMainFrame()
->GetLastCommittedURL());
EXPECT_TRUE(rfh_a->IsActive());

// 3) Navigate away. |rfh_a| should enter the bfcache since we only evict
// 4) Navigate away. |rfh_a| should enter the bfcache since we only evict
// before restoration.
EXPECT_TRUE(NavigateToURL(shell(), url_b));
EXPECT_TRUE(rfh_a->IsInBackForwardCache());

// 4) Go back. |rfh_a| should be evicted upon restoration.
ASSERT_TRUE(HistoryGoBack(web_contents()));
// 5) Go back. |rfh_a| should be evicted upon restoration.
ASSERT_TRUE(HistoryGoBack(tab_to_be_bfcached->web_contents()));
ExpectNotRestored(
{NotRestoredReason::kCacheControlNoStoreHTTPOnlyCookieModified}, {}, {},
{}, {}, FROM_HERE);
Expand Down
6 changes: 4 additions & 2 deletions content/browser/renderer_host/back_forward_cache_impl.cc
View file
Expand Up @@ -609,10 +609,12 @@ void BackForwardCacheImpl::UpdateCanStoreToIncludeCacheControlNoStore(
// Note that kCacheControlNoStoreHTTPOnlyCookieModified,
// kCacheControlNoStoreCookieModified and kCacheControlNoStore are mutually
// exclusive.
if (render_frame_host->GetCookieChangeInfo().http_only_cookie_modified) {
if (render_frame_host->GetCookieChangeInfo()
.http_only_cookie_modification_count_ > 0) {
result.No(BackForwardCacheMetrics::NotRestoredReason::
kCacheControlNoStoreHTTPOnlyCookieModified);
} else if (render_frame_host->GetCookieChangeInfo().cookie_modified) {
} else if (render_frame_host->GetCookieChangeInfo()
.cookie_modification_count_ > 0) {
// JavaScript cookies are modified but not HTTP cookies. Only restore based
// on the experiment level.
if (GetCacheControlNoStoreLevel() <=
Expand Down
22 changes: 22 additions & 0 deletions content/browser/renderer_host/navigation_request.cc
View file
Expand Up @@ -36,6 +36,7 @@
#include "base/timer/elapsed_timer.h"
#include "base/trace_event/trace_conversion_helper.h"
#include "base/types/optional_util.h"
#include "base/types/pass_key.h"
#include "build/build_config.h"
#include "build/chromeos_buildflags.h"
#include "components/attribution_reporting/os_registration.h"
Expand Down Expand Up @@ -102,6 +103,7 @@
#include "content/public/browser/client_hints_controller_delegate.h"
#include "content/public/browser/commit_deferring_condition.h"
#include "content/public/browser/content_browser_client.h"
#include "content/public/browser/cookie_access_details.h"
#include "content/public/browser/global_request_id.h"
#include "content/public/browser/navigation_controller.h"
#include "content/public/browser/navigation_ui_data.h"
Expand All @@ -126,6 +128,7 @@
#include "net/base/net_errors.h"
#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
#include "net/base/url_util.h"
#include "net/cookies/canonical_cookie.h"
#include "net/http/http_request_headers.h"
#include "net/http/http_status_code.h"
#include "net/url_request/redirect_info.h"
Expand Down Expand Up @@ -8237,6 +8240,25 @@ void NavigationRequest::OnCookiesAccessed(
GetDelegate()->OnCookiesAccessed(this, allowed);
if (!blocked.cookie_list.empty())
GetDelegate()->OnCookiesAccessed(this, blocked);

// When determining the BFCache eligibility, we explicitly ignore the cookie
// changes from the navigation itself because we want the
// `CookieChangeListener` to only track the cookie changes that potentially
// make the document initially rendered by the navigation request outdated.
if (allowed.type == CookieAccessDetails::Type::kChange) {
uint64_t cookie_modification_count = allowed.cookie_list.size();
uint64_t http_only_cookie_modification_count = 0u;
for (net::CanonicalCookie& cookie : allowed.cookie_list) {
if (cookie.IsHttpOnly()) {
http_only_cookie_modification_count++;
}
}
if (cookie_change_listener_) {
cookie_change_listener_->RemoveNavigationCookieModificationCount(
base::PassKey<NavigationRequest>(), cookie_modification_count,
http_only_cookie_modification_count);
}
}
}

void NavigationRequest::Clone(
Expand Down
8 changes: 5 additions & 3 deletions content/browser/renderer_host/render_frame_host_impl.cc
View file
Expand Up @@ -14871,9 +14871,11 @@ void RenderFrameHostImpl::CookieChangeListener::OnCookieChange(
const net::CookieChangeInfo& change) {
// TODO (https://crbug.com/1399741): After adding the invalidation signals
// API, we could mark the page as ineligible for BFCache as soon as the cookie
// change event is received.
cookie_change_info_.http_only_cookie_modified |= change.cookie.IsHttpOnly();
cookie_change_info_.cookie_modified = true;
// change event is received after the navigation is committed.
cookie_change_info_.cookie_modification_count_++;
if (change.cookie.IsHttpOnly()) {
cookie_change_info_.http_only_cookie_modification_count_++;
}
}

RenderFrameHostImpl::CookieChangeListener::CookieChangeInfo
Expand Down
24 changes: 20 additions & 4 deletions content/browser/renderer_host/render_frame_host_impl.h
View file
Expand Up @@ -8,6 +8,7 @@
#include <stddef.h>
#include <stdint.h>

#include <cstdint>
#include <list>
#include <map>
#include <memory>
Expand Down Expand Up @@ -1382,10 +1383,10 @@ class CONTENT_EXPORT RenderFrameHostImpl
class CookieChangeListener : public network::mojom::CookieChangeListener {
public:
struct CookieChangeInfo {
// Indicates whether any cookie modification has been observed.
bool cookie_modified = false;
// Indicates whether any HTTPOnly cookie modification has been observed.
bool http_only_cookie_modified = false;
// The number of observed cookie modifications.
int64_t cookie_modification_count_ = 0;
// The number of observed HTTPOnly cookie modifications.
int64_t http_only_cookie_modification_count_ = 0;
};

CookieChangeListener(StoragePartition* storage_partition, GURL& url);
Expand All @@ -1396,6 +1397,21 @@ class CONTENT_EXPORT RenderFrameHostImpl
// Returns a copy of the `cookie_change_info_`.
CookieChangeInfo cookie_change_info() { return cookie_change_info_; }

// We don't want to count the cookie modification made by the
// `NavigationRequest` itself, so provide this function to allow the count
// adjustment.
// Passing the `base::PassKey` to restrict the caller of this method to
// `NavigationRequest` only.
void RemoveNavigationCookieModificationCount(
base::PassKey<content::NavigationRequest> navigation_request,
uint64_t cookie_modification_count_delta,
uint64_t http_only_cookie_modification_count_delta) {
cookie_change_info_.cookie_modification_count_ -=
cookie_modification_count_delta;
cookie_change_info_.http_only_cookie_modification_count_ -=
http_only_cookie_modification_count_delta;
}

private:
// network::mojom::CookieChangeListener
void OnCookieChange(const net::CookieChangeInfo& change) override;
Expand Down

0 comments on commit 7728014

Please sign in to comment.