0

Revert "Match prefetches against render requests"

This reverts commit 74fb3b3d2c.

Reason for revert: New unit tests are failing in the following builder:
https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Tests%20(dbg)(1)/123028/overview

Original change's description:
> Match prefetches against render requests
>
> Call the matching code from PrefetchMatchingURLLoaderFactory. If the
> newly-added "NetworkContextPrefetchUseMatches" feature is enabled, also
> allow successful matches to be consumed by the renderer. Otherwise, only
> the histograms indicating matching success or failure will be logged.
> The prefetch is abandoned after a delay but the renderer may still
> be able to use the disk cache entry it created.
>
> Add check that the `request_initiator` is valid so that the render
> process cannot bypass access checks when a prefetch is matched.
>
> Rename the SetClient() method of network::PrefetchURLLoaderClient to
> Consume() and make it take a PendingReceiver<URLLoader> argument. The
> CorsURLLoader that the client is bound to is rebound to the passed-in
> argument, permitted it to be called from the render process directly.
> Add a Matches() method which calls PrefetchMatches() against the stored
> ResourceRequest.
>
> Also refactor the PrefetchURLLoaderClient tests to provide a more
> realistic environment.
>
> Currently tests that call Consume() leak memory and so are disabled on
> sanitizer builds. See https://crbug.com/375425822. This memory leak only
> happens if the "NetworkContextPrefetchUseMatches" feature is enabled and
> will be fixed before that is used in production.
>
> BUG=342445996
>
> Low-Coverage-Reason: TESTS_IN_SEPARATE_CL Uncovered code has no callers yet.
> Change-Id: I607db0dbbf8017730b56de3baa4d66d2b15268ae
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5715614
> Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
> Commit-Queue: Adam Rice <ricea@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1374542}

Bug: 342445996, 375425822
Change-Id: I7293583afc1f1d0291b693b2e1f47f3d492adcdb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5971793
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Felipe Andrade <fsandrade@chromium.org>
Owners-Override: Felipe Andrade <fsandrade@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1374591}
This commit is contained in:
Felipe Andrade
2024-10-28 13:59:08 +00:00
committed by Chromium LUCI CQ
parent 5e7e8ba0eb
commit 00f7b4fc7a
12 changed files with 152 additions and 521 deletions

@ -102,12 +102,6 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CorsURLLoaderFactory final
// network context.
void ClearBindings();
// Exposed for use by PrefetchMatchingURLLoaderFactory.
int32_t process_id() const { return process_id_; }
const std::optional<url::Origin>& request_initiator_origin_lock() const {
return request_initiator_origin_lock_;
}
mojom::CrossOriginEmbedderPolicyReporter* coep_reporter() {
return coep_reporter_ ? coep_reporter_.get() : nullptr;
}
@ -117,8 +111,6 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CorsURLLoaderFactory final
return url_loaders_;
}
const net::IsolationInfo& isolation_info() const { return isolation_info_; }
mojom::SharedDictionaryAccessObserver* GetSharedDictionaryAccessObserver()
const;

@ -8,9 +8,7 @@
#include "base/check.h"
#include "base/check_op.h"
#include "base/location.h"
#include "base/logging.h"
#include "base/task/sequenced_task_runner.h"
#include "base/types/pass_key.h"
#include "net/base/network_isolation_key.h"
#include "services/network/prefetch_url_loader_client.h"
@ -21,20 +19,13 @@ namespace network {
namespace {
size_t GetMaxSize() {
const int supplied_size = features::kNetworkContextPrefetchMaxLoaders.Get();
int supplied_size = features::kNetworkContextPrefetchMaxLoaders.Get();
return static_cast<size_t>(std::max(supplied_size, 1));
}
base::TimeDelta GetEraseGraceTime() {
const base::TimeDelta supplied_delta =
features::kNetworkContextPrefetchEraseGraceTime.Get();
return std::max(base::Seconds(0), supplied_delta);
}
} // namespace
PrefetchCache::PrefetchCache()
: max_size_(GetMaxSize()), erase_grace_time_(GetEraseGraceTime()) {}
PrefetchCache::PrefetchCache() : max_size_(GetMaxSize()) {}
PrefetchCache::~PrefetchCache() = default;
@ -93,7 +84,7 @@ PrefetchURLLoaderClient* PrefetchCache::Emplace(
PrefetchURLLoaderClient* PrefetchCache::Lookup(
const net::NetworkIsolationKey& nik,
const GURL& url) {
const auto it = FindInMap(nik, url);
const auto it = map_.find(KeyType(nik, url));
return it == map_.end() ? nullptr : it->second;
}
@ -111,7 +102,8 @@ void PrefetchCache::Consume(PrefetchURLLoaderClient* client) {
}
void PrefetchCache::Erase(PrefetchURLLoaderClient* client) {
const auto it = FindInMap(client->network_isolation_key(), client->url());
const auto it =
map_.find(KeyType(client->network_isolation_key(), client->url()));
if (it != map_.end()) {
CHECK_EQ(it->second, client);
map_.erase(it);
@ -120,15 +112,6 @@ void PrefetchCache::Erase(PrefetchURLLoaderClient* client) {
EraseFromStorage(client);
}
void PrefetchCache::DelayedErase(PrefetchURLLoaderClient* client) {
const auto now = base::TimeTicks::Now();
PendingErasure pending_erasure = {client->network_isolation_key(),
client->url(), now + erase_grace_time_};
CHECK(Lookup(pending_erasure.nik, pending_erasure.url));
delayed_erase_queue_.push(std::move(pending_erasure));
SchedulePendingErases(now);
}
void PrefetchCache::OnTimer() {
auto now = base::TimeTicks::Now();
while (!list_.empty() &&
@ -140,23 +123,6 @@ void PrefetchCache::OnTimer() {
}
}
void PrefetchCache::DoDelayedErases() {
const auto now = base::TimeTicks::Now();
while (!delayed_erase_queue_.empty() &&
delayed_erase_queue_.front().erase_time <= now) {
PendingErasure pending = std::move(delayed_erase_queue_.front());
delayed_erase_queue_.pop();
PrefetchURLLoaderClient* to_erase = Lookup(pending.nik, pending.url);
if (to_erase) {
RemoveFromCache(to_erase);
EraseFromStorage(to_erase);
}
}
if (!delayed_erase_queue_.empty()) {
SchedulePendingErases(now);
}
}
void PrefetchCache::EraseOldest() {
CHECK(!list_.empty());
PrefetchURLLoaderClient* oldest = list_.head()->value();
@ -167,7 +133,8 @@ void PrefetchCache::EraseOldest() {
}
void PrefetchCache::RemoveFromCache(PrefetchURLLoaderClient* client) {
const auto it = FindInMap(client->network_isolation_key(), client->url());
const auto it =
map_.find(KeyType(client->network_isolation_key(), client->url()));
CHECK(it != map_.end());
CHECK_EQ(it->second, client);
map_.erase(it);
@ -183,7 +150,6 @@ void PrefetchCache::EraseFromStorage(PrefetchURLLoaderClient* client) {
}
void PrefetchCache::StartTimer(base::TimeTicks now) {
CHECK(!list_.empty());
auto next_expiry = list_.head()->value()->expiry_time();
// It's safe to use base::Unretained() as destroying `this` will destroy
// `expiry_timer_`, preventing the callback being called.
@ -192,21 +158,4 @@ void PrefetchCache::StartTimer(base::TimeTicks now) {
base::BindOnce(&PrefetchCache::OnTimer, base::Unretained(this)));
}
void PrefetchCache::SchedulePendingErases(base::TimeTicks now) {
CHECK(!delayed_erase_queue_.empty());
const auto next_expiry = delayed_erase_queue_.front().erase_time;
const auto delay = std::max(base::Seconds(0), next_expiry - now);
base::SequencedTaskRunner::GetCurrentDefault()->PostDelayedTask(
FROM_HERE,
base::BindOnce(&PrefetchCache::DoDelayedErases,
weak_factory_.GetWeakPtr()),
delay);
}
PrefetchCache::MapType::iterator PrefetchCache::FindInMap(
const net::NetworkIsolationKey& nik,
const GURL& url) {
return map_.find(KeyType(nik, url));
}
} // namespace network

@ -11,12 +11,9 @@
#include "base/component_export.h"
#include "base/containers/linked_list.h"
#include "base/containers/queue.h"
#include "base/containers/unique_ptr_adapters.h"
#include "base/memory/raw_ptr.h"
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "net/base/network_isolation_key.h"
#include "url/gurl.h"
namespace net {
@ -75,19 +72,6 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) PrefetchCache final {
// created by Emplace() and not already erased.
void Erase(PrefetchURLLoaderClient* client);
// Erases `client` after `kEraseGraceTime` has expired. The purpose is to
// permit a new transaction from a renderer to reach the HttpCache code before
// this client is erased, so that it can take over the cache lock if possible,
// avoiding the entry being truncated.
//
// This is a temporary feature to maximise the chances of reusing the disk
// cache entry when the feature kNetworkContextPrefetchUseMatches is not
// enabled.
//
// TODO(crbug.com/342445996): Remove this method and associated code after
// kNetworkContextPrefetchUseMatches has been permanently enabled.
void DelayedErase(PrefetchURLLoaderClient* client);
private:
// This is not a std::list because we want to be able to remove an item from
// the cache by pointer.
@ -104,23 +88,9 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) PrefetchCache final {
using ClientStorage = std::set<std::unique_ptr<PrefetchURLLoaderClient>,
base::UniquePtrComparator>;
struct PendingErasure {
// The client is referred to by its key rather than a pointer, so that there
// is no dangling reference if something else erases the client first.
net::NetworkIsolationKey nik;
GURL url;
base::TimeTicks erase_time;
};
using EraseQueue = base::queue<PendingErasure>;
// Deletes any expired cache entries and then restarts the timer if needed.
void OnTimer();
// Deletes anything in `delayed_erase_queue_` that is due for deletion, and
// then schedules another call if the queue is still non-empty.
void DoDelayedErases();
// Removes and deletes the oldest entry from the cache.
void EraseOldest();
@ -136,16 +106,6 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) PrefetchCache final {
// handy and some don't.
void StartTimer(base::TimeTicks now = base::TimeTicks::Now());
// Schedules a task to call DoDelayedErases() the next time something in
// `delayed_erase_queue_` needs to be erased. `now` should be the return value
// from a recent call to `base::TimeTicks::Now()`.
void SchedulePendingErases(base::TimeTicks now);
// Performing a find() on `map_` is sufficiently messy that it's worth
// encapsulating in a separate method.
MapType::iterator FindInMap(const net::NetworkIsolationKey& nik,
const GURL& url);
// Storage for all the PrefetchURLLoaderClients created by this object,
// regardless if Consume() has been called for them or not. `list_` and `map_`
// contain references into these objects, so must be destroyed first.
@ -164,18 +124,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) PrefetchCache final {
// `list_` will expire.
base::OneShotTimer expiry_timer_;
// Queue for DelayedErase(). Entries will be deleted when their `erase_time`
// is reached.
EraseQueue delayed_erase_queue_;
// Initialized from kNetworkContextPrefetchMaxLoaders feature flag.
const size_t max_size_;
// Initialized from "erase_grace_time" parameter to "NetworkContextPrefetch"
// feature.
const base::TimeDelta erase_grace_time_;
base::WeakPtrFactory<PrefetchCache> weak_factory_{this};
};
} // namespace network

@ -63,7 +63,6 @@ class PrefetchCacheTest : public ::testing::Test {
feature_list_.InitAndEnableFeatureWithParameters(
features::kNetworkContextPrefetch,
{{"max_loaders", base::NumberToString(kMaxSize)}});
erase_grace_time_ = features::kNetworkContextPrefetchEraseGraceTime.Get();
}
PrefetchCache& cache() { return cache_; }
@ -72,12 +71,7 @@ class PrefetchCacheTest : public ::testing::Test {
task_environment_.FastForwardBy(delta);
}
base::TimeDelta erase_grace_time() const { return erase_grace_time_; }
base::TimeDelta half_grace_time() const { return erase_grace_time_ / 2; }
private:
base::TimeDelta erase_grace_time_;
base::test::ScopedFeatureList feature_list_;
base::test::TaskEnvironment task_environment_{
base::test::TaskEnvironment::TimeSource::MOCK_TIME};
@ -264,57 +258,6 @@ TEST_F(PrefetchCacheTest, ConsumedPrefetchesAreNotExpired) {
cache().Erase(client);
}
TEST_F(PrefetchCacheTest, DelayedEraseErasesAfterADelay) {
auto* client =
cache().Emplace(MakeResourceRequest(TestURL(0), TestIsolationInfo()));
ASSERT_TRUE(client);
cache().DelayedErase(client);
// Should be still there.
FastForwardBy(half_grace_time());
// Still there.
EXPECT_TRUE(cache().Lookup(TestNIK(), TestURL(0)));
FastForwardBy(erase_grace_time());
// Gone.
EXPECT_FALSE(cache().Lookup(TestNIK(), TestURL(0)));
}
TEST_F(PrefetchCacheTest, MultipleDelayedErases) {
// This test requires that erase_grace_time() be evenly divisible by two.
ASSERT_EQ(half_grace_time() + half_grace_time(), erase_grace_time())
<< "This test requires that kEraseGraceTime be evenly divisible by two";
const auto still_there = [&](int client_index) {
return cache().Lookup(TestNIK(), TestURL(client_index));
};
for (int i = 0; i < 3; ++i) {
auto* client =
cache().Emplace(MakeResourceRequest(TestURL(i), TestIsolationInfo()));
ASSERT_TRUE(client);
cache().DelayedErase(client);
FastForwardBy(half_grace_time());
EXPECT_TRUE(still_there(i));
if (i >= 1) {
EXPECT_FALSE(still_there(i - 1));
}
}
EXPECT_TRUE(still_there(2));
FastForwardBy(half_grace_time());
EXPECT_FALSE(still_there(2));
}
TEST_F(PrefetchCacheTest, DelayedEraseRacesWithExpiry) {
auto* client =
cache().Emplace(MakeResourceRequest(TestURL(0), TestIsolationInfo()));
ASSERT_TRUE(client);
FastForwardBy(PrefetchCache::kMaxAge - half_grace_time());
cache().DelayedErase(client);
FastForwardBy(erase_grace_time());
// Should be gone now.
EXPECT_FALSE(cache().Lookup(TestNIK(), TestURL(0)));
}
} // namespace
} // namespace network

@ -9,10 +9,6 @@
#include "base/check_op.h"
#include "base/functional/bind.h"
#include "services/network/cors/cors_url_loader_factory.h"
#include "services/network/prefetch_cache.h"
#include "services/network/prefetch_url_loader_client.h"
#include "services/network/public/cpp/features.h"
#include "services/network/public/cpp/resource_request.h"
#include "services/network/resource_scheduler/resource_scheduler_client.h"
namespace network {
@ -31,10 +27,9 @@ PrefetchMatchingURLLoaderFactory::PrefetchMatchingURLLoaderFactory(
mojo::PendingReceiver<mojom::URLLoaderFactory>(),
origin_access_list,
this)),
origin_access_list_(origin_access_list),
context_(context),
cache_(cache),
use_matches_(base::FeatureList::IsEnabled(
features::kNetworkContextPrefetchUseMatches)) {
cache_(cache) {
receivers_.Add(this, std::move(receiver));
// This use of base::Unretained() is safe because `receivers_` won't call the
// disconnect handler after it has been destroyed.
@ -66,41 +61,7 @@ void PrefetchMatchingURLLoaderFactory::CreateLoaderAndStart(
ResourceRequest& request,
mojo::PendingRemote<mojom::URLLoaderClient> client,
const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
// If we don't think the request should be permitted from a render process, we
// don't try to match it and instead let CorsURLLoaderFactory deal with the
// issue.
if (cache_ && IsRequestSafeForMatching(request)) {
PrefetchURLLoaderClient* prefetch_client = cache_->Lookup(
next_->isolation_info().network_isolation_key(), request.url);
if (prefetch_client) {
// A prefetch exists with the same NIK and URL.
if (prefetch_client->Matches(request)) {
if (use_matches_) {
// The match has succeeded, and we are going to hand-off the
// in-progress prefetch to the renderer.
// TODO(crbug.com/342445996): Check that `options` is compatible with
// the prefetch and whether anything needs to be done to the ongoing
// request to match it.
prefetch_client->Consume(std::move(loader), std::move(client));
return;
}
// The match has succeeded, but the kNetworkContextPrefetchUseMatches
// feature is not enabled. Try to make it possible for the render
// process to reuse the cache entry.
// Give the real URLLoader time to reach the cache, then cancel the
// prefetch.
cache_->DelayedErase(prefetch_client);
} else {
// There was an entry with the same NIK and URL, but it failed to match
// on some other field. It is unlikely the renderer will subsequently
// issue a matching request for the same URL, so erase the cache entry
// to save resources.
cache_->Erase(prefetch_client);
}
}
}
// TODO(ricea): Actually match prefetches here.
next_->CreateLoaderAndStart(std::move(loader), request_id, options, request,
std::move(client), traffic_annotation);
@ -150,56 +111,4 @@ void PrefetchMatchingURLLoaderFactory::OnDisconnect() {
}
}
bool PrefetchMatchingURLLoaderFactory::IsRequestSafeForMatching(
const ResourceRequest& request) {
// We never match requests from the browser process.
if (next_->process_id() == mojom::kBrowserProcessId) {
return false;
}
// `trusted_params` should never be set on a request from a render process.
if (request.trusted_params) {
return false;
}
// Ensure that the value of `request_initiator` is permitted for this render
// process.
InitiatorLockCompatibility compatibility =
VerifyRequestInitiatorLock(next_->request_initiator_origin_lock(),
request.mode == mojom::RequestMode::kNavigate
? url::Origin::Create(request.url)
: request.request_initiator);
// TODO(crbug.com/342445996): Share code with CorsURLLoaderFactory.
switch (compatibility) {
case InitiatorLockCompatibility::kCompatibleLock:
return true;
case InitiatorLockCompatibility::kBrowserProcess:
NOTREACHED();
case InitiatorLockCompatibility::kNoLock:
// `request_initiator_origin_lock` should always be set in a
// URLLoaderFactory vended to a renderer process. See also
// https://crbug.com/1114906.
NOTREACHED_IN_MIGRATION();
mojo::ReportBadMessage(
"CorsURLLoaderFactory: no initiator lock in a renderer request");
return false;
case InitiatorLockCompatibility::kNoInitiator:
// Requests from the renderer need to always specify an initiator.
mojo::ReportBadMessage(
"CorsURLLoaderFactory: no initiator in a renderer request");
return false;
case InitiatorLockCompatibility::kIncorrectLock:
// Requests from the renderer need to always specify a correct initiator.
mojo::ReportBadMessage(
"CorsURLLoaderFactory: lock VS initiator mismatch");
return false;
}
NOTREACHED();
}
} // namespace network

@ -106,13 +106,12 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) PrefetchMatchingURLLoaderFactory final
private:
void OnDisconnect();
bool IsRequestSafeForMatching(const ResourceRequest& request);
const std::unique_ptr<cors::CorsURLLoaderFactory> next_;
const raw_ptr<const cors::OriginAccessList> origin_access_list_;
const raw_ptr<NetworkContext> context_;
const raw_ptr<PrefetchCache> cache_;
mojo::ReceiverSet<mojom::URLLoaderFactory> receivers_;
const bool use_matches_;
};
} // namespace network

@ -11,7 +11,6 @@
#include "mojo/public/cpp/base/big_buffer.h"
#include "mojo/public/cpp/system/data_pipe.h"
#include "services/network/prefetch_cache.h"
#include "services/network/prefetch_matches.h"
#include "services/network/public/cpp/resource_request.h"
#include "services/network/public/mojom/early_hints.mojom.h"
#include "services/network/public/mojom/url_loader.mojom.h"
@ -79,24 +78,11 @@ PrefetchURLLoaderClient::BindNewPipeAndPassRemote() {
return pending_remote;
}
bool PrefetchURLLoaderClient::Matches(
const ResourceRequest& real_request) const {
return PrefetchMatches(request_, real_request);
}
void PrefetchURLLoaderClient::Consume(
mojo::PendingReceiver<mojom::URLLoader> loader,
void PrefetchURLLoaderClient::SetClient(
mojo::PendingRemote<mojom::URLLoaderClient> client) {
// Remove ourselves from the PrefetchCache.
prefetch_cache_->Consume(this);
// This is safe because we never call any methods on the URLLoader.
mojo::PendingRemote<mojom::URLLoader> unbound_loader = url_loader_.Unbind();
bool fuse_success =
mojo::FusePipes(std::move(loader), std::move(unbound_loader));
// TODO(ricea): Can this fail?
CHECK(fuse_success);
real_client_.Bind(std::move(client));
// This use of base::Unretained is safe because the disconnect handler will
// not be called after `real_client_` is destroyed.

@ -60,17 +60,11 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) PrefetchURLLoaderClient final
// only be called once.
mojo::PendingRemote<mojom::URLLoaderClient> BindNewPipeAndPassRemote();
// Returns true if `request_` is similar enough to `real_request_` that it can
// be used to serve it.
bool Matches(const ResourceRequest& real_request) const;
// Binds this response to a request from a client. Sets `real_client_` and
// replays any callbacks that have been received up until this point. Once
// this has been called, the object's lifetime is tied to the remotes and it
// will instruct PrefetchCache to delete it if it loses connection to
// `real_client_` or the URLLoader. Calling Consume() may delete `this`.
void Consume(mojo::PendingReceiver<mojom::URLLoader> loader,
mojo::PendingRemote<mojom::URLLoaderClient> client);
// Sets `real_client_` and replays any callbacks that have been received up
// until this point. Once this has been called, the object becomes
// self-owning and will delete itself if it loses connection to `real_client_`
// or the URLLoader. May delete `this`.
void SetClient(mojo::PendingRemote<mojom::URLLoaderClient> client);
// Implementation of mojom::URLLoaderClient. Each of these methods forwards
// the arguments in `real_client_` if it has been set, otherwise caches the
@ -116,8 +110,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) PrefetchURLLoaderClient final
// This is initialized at construction.
mojo::Receiver<mojom::URLLoaderClient> receiver_{this};
// This is initialized when the request is started. It is unbound again when
// Consume() is called.
// This is initialized when the request is started.
mojo::Remote<mojom::URLLoader> url_loader_;
// A pointer to the PrefetchCache object that created and owns us.

@ -10,7 +10,6 @@
#include <utility>
#include "base/functional/bind.h"
#include "base/functional/callback_helpers.h"
#include "base/ranges/algorithm.h"
#include "base/task/thread_pool.h"
#include "base/test/task_environment.h"
@ -214,210 +213,124 @@ MATCHER(URLLoaderCompletionStatusIsOk, "URLLoaderCompletionStatus is ok") {
equals(&S::should_collapse_initiator);
}
// An implementation of URLLoader that does nothing.
class StubURLLoader final : public network::mojom::URLLoader {
public:
explicit StubURLLoader(
mojo::PendingReceiver<network::mojom::URLLoader> url_loader_receiver)
: receiver_(this, std::move(url_loader_receiver)) {}
// Calls all the mojo methods on `client` in order with verifiable parameters.
// This doesn't in any way correspond to the real behaviour of a URLLoader.
void CallAllMojoMethods(mojom::URLLoaderClient* client) {
client->OnReceiveEarlyHints(TestEarlyHints());
client->OnReceiveResponse(TestURLResponseHead(), TestDataPipeConsumer(),
TestBigBuffer());
client->OnReceiveRedirect(TestRedirectInfo(), TestURLResponseHead());
client->OnUploadProgress(kTestCurrentPosition, kTestTotalSize,
base::DoNothing());
client->OnTransferSizeUpdated(kTransferSizeDiff);
client->OnComplete(TestURLLoaderCompletionStatus());
}
// network::mojom::URLLoader overrides.
void FollowRedirect(
const std::vector<std::string>& removed_headers,
const net::HttpRequestHeaders& modified_headers,
const net::HttpRequestHeaders& modified_cors_exempt_headers,
const std::optional<GURL>& new_url) override {}
void SetPriority(net::RequestPriority priority,
int32_t intra_priority_value) override {}
void PauseReadingBodyFromNet() override {}
void ResumeReadingBodyFromNet() override {}
// This adds expectations that all the methods on `client` will be called with
// arguments matching those in `CallAllMojoMethods()`.
void ExpectCallMojoMethods(StrictMock<MockURLLoaderClient>& mock_client) {
EXPECT_CALL(mock_client, OnReceiveEarlyHints(IsTrue()));
EXPECT_CALL(mock_client,
OnReceiveResponse(URLResponseHeadIsOk(), _,
Optional(BigBufferHasExpectedContents())))
.WillOnce(WithArg<1>(Invoke(CheckDataPipeContents)));
EXPECT_CALL(mock_client, OnReceiveRedirect(EqualsTestRedirectInfo(),
URLResponseHeadIsOk()));
EXPECT_CALL(mock_client,
OnUploadProgress(Eq(kTestCurrentPosition), Eq(kTestTotalSize), _))
.WillOnce(WithArg<2>([](auto&& callback) { std::move(callback).Run(); }));
EXPECT_CALL(mock_client, OnTransferSizeUpdated(Eq(kTransferSizeDiff)));
EXPECT_CALL(mock_client, OnComplete(URLLoaderCompletionStatusIsOk()));
}
// A wrapper for a mojo::Receiver that calls Call() on `disconnect` when the
// pending remote is disconnected.
class DisconnectDetectingReceiver final {
public:
DisconnectDetectingReceiver(mojom::URLLoaderClient* client,
MockFunction<void()>* disconnect)
: receiver_(client), disconnect_(disconnect) {}
mojo::PendingRemote<mojom::URLLoaderClient> GetPendingRemote() {
auto pending_remote = receiver_.BindNewPipeAndPassRemote();
// After `receiver_` is destroyed, the callback will not be called, so this
// use of base::Unretained() is safe.
receiver_.set_disconnect_handler(base::BindOnce(
&MockFunction<void()>::Call, base::Unretained(disconnect_)));
return pending_remote;
}
private:
mojo::Receiver<network::mojom::URLLoader> receiver_;
mojo::Receiver<mojom::URLLoaderClient> receiver_;
raw_ptr<MockFunction<void()>> disconnect_;
};
class PrefetchURLLoaderClientTest : public ::testing::Test {
protected:
PrefetchURLLoaderClientTest()
: client_(cache()->Emplace(TestRequest())),
stub_url_loader_(client_->GetURLLoaderPendingReceiver()),
client_pending_remote_(client_->BindNewPipeAndPassRemote()),
mock_client_receiver_(&mock_client_) {}
~PrefetchURLLoaderClientTest() override {
// Avoid calls to a disconnect handler on the stack.
mock_client_receiver_.reset();
}
// The cache that owns the object under test.
PrefetchCache* cache() { return &cache_; }
// The object under test.
PrefetchURLLoaderClient* client() { return client_; }
// A mock URLLoaderClient that will stand-in for the URLLoaderClient inside
// the render process.
StrictMock<MockURLLoaderClient>& mock_client() { return mock_client_; }
// Arranges for `disconnect_handler` to be called when `mock_client_receiver_`
// detects a disconnection. Must be called after `Consume()`.
void set_disconnect_handler(MockFunction<void()>* disconnect_handler) {
// This use of base::Unretained is potentially unsafe as
// `disconnect_handler` is on the stack. It is mitigated by resetting
// `mock_client_receiver_` in the destructor.
mock_client_receiver_.set_disconnect_handler(base::BindOnce(
&MockFunction<void()>::Call, base::Unretained(disconnect_handler)));
}
// Pretend the datapipe that passed mojo messages from the "real" URLLoader
// disconnected.
void ResetClientPendingRemote() { client_pending_remote_.reset(); }
// Remove the reference to the client. This needs to be called by tests which
// arrange for the client to be deleted before the test ends, in order to
// prevent triggering the dangling pointer detection.
void ClearClientPointer() { client_ = nullptr; }
// Sets `client_` to nullptr and returns the old value. This is needed for
// tests that need a call a method on `client_` that will delete it.
PrefetchURLLoaderClient* TakeClientPointer() {
return std::exchange(client_, nullptr);
}
// Calls Consume() on `client_`, hooking it up to the mock client.
void Consume() {
client_->Consume(loader_.BindNewPipeAndPassReceiver(),
mock_client_receiver_.BindNewPipeAndPassRemote());
EXPECT_TRUE(loader_.is_bound());
}
// Calls all the mojo methods on `client_` in order with verifiable
// parameters. This doesn't in any way correspond to the real behaviour of a
// URLLoader.
void CallAllMojoMethods() {
client_->OnReceiveEarlyHints(TestEarlyHints());
client_->OnReceiveResponse(TestURLResponseHead(), TestDataPipeConsumer(),
TestBigBuffer());
client_->OnReceiveRedirect(TestRedirectInfo(), TestURLResponseHead());
client_->OnUploadProgress(kTestCurrentPosition, kTestTotalSize,
base::DoNothing());
client_->OnTransferSizeUpdated(kTransferSizeDiff);
client_->OnComplete(TestURLLoaderCompletionStatus());
}
// This adds expectations that all the methods on `client` will be called with
// arguments matching those in `CallAllMojoMethods()`.
void ExpectCallMojoMethods() {
EXPECT_CALL(mock_client_, OnReceiveEarlyHints(IsTrue()));
EXPECT_CALL(mock_client_,
OnReceiveResponse(URLResponseHeadIsOk(), _,
Optional(BigBufferHasExpectedContents())))
.WillOnce(WithArg<1>(Invoke(CheckDataPipeContents)));
EXPECT_CALL(mock_client_, OnReceiveRedirect(EqualsTestRedirectInfo(),
URLResponseHeadIsOk()));
EXPECT_CALL(mock_client_, OnUploadProgress(Eq(kTestCurrentPosition),
Eq(kTestTotalSize), _))
.WillOnce(
WithArg<2>([](auto&& callback) { std::move(callback).Run(); }));
EXPECT_CALL(mock_client_, OnTransferSizeUpdated(Eq(kTransferSizeDiff)));
EXPECT_CALL(mock_client_, OnComplete(URLLoaderCompletionStatusIsOk()));
}
// Construct a PrefetchURLLoaderClient with TestRequest() that is initially
// owned by `cache_`. The return value is always destroyed automatically.
PrefetchURLLoaderClient* Emplace() { return cache()->Emplace(TestRequest()); }
void RunUntilIdle() { task_environment_.RunUntilIdle(); }
private:
base::test::TaskEnvironment task_environment_;
// A dummy URLLoader that adopts `client_`'s url_loader_ when Consume() is
// called.
mojo::Remote<mojom::URLLoader> loader_;
// Owner for `client_`.
PrefetchCache cache_;
// The client under test. Owned by `cache_`.
raw_ptr<PrefetchURLLoaderClient> client_;
// The "real" URLLoader that `client_` connects to.
StubURLLoader stub_url_loader_;
// Connected to `client_`. Used to detect when `client_` is destroyed. In
// normal operation this would be passed to a real URLLoader.
mojo::PendingRemote<mojom::URLLoaderClient> client_pending_remote_;
// Represents the renderer-side client that will eventually consume this
// object.
StrictMock<MockURLLoaderClient> mock_client_;
// Forwards mojo calls to `mock_client_`.
mojo::Receiver<mojom::URLLoaderClient> mock_client_receiver_;
};
// Tests that call Consume() leak memory and hit an assert() on iOS, so they are
// disabled on address sanitizer and iOS builds.
// TODO(crbug.com/375425822): Fix the leak before running this code in
// production.
#if BUILDFLAG(IS_IOS) || defined(ADDRESS_SANITIZER)
#define MAYBE_RecordAndReplay DISABLED_RecordAndReplay
#define MAYBE_DelegateDirectly DISABLED_DelegateDirectly
#define MAYBE_ReplayAfterResponse DISABLED_ReplayAfterResponse
#define MAYBE_ConsumeRemovesFromCache DISABLED_ConsumeRemovesFromCache
#else
#define MAYBE_RecordAndReplay RecordAndReplay
#define MAYBE_DelegateDirectly DelegateDirectly
#define MAYBE_ReplayAfterResponse ReplayAfterResponse
#define MAYBE_ConsumeRemovesFromCache ConsumeRemovesFromCache
#endif
TEST_F(PrefetchURLLoaderClientTest, Construct) {
EXPECT_EQ(client()->url(), TestURL());
EXPECT_EQ(client()->network_isolation_key(), TestNIK());
EXPECT_GT(client()->expiry_time(), base::TimeTicks::Now());
EXPECT_LT(client()->expiry_time(), base::TimeTicks::Now() + base::Days(1));
auto* client = Emplace();
EXPECT_EQ(client->url(), TestURL());
EXPECT_EQ(client->network_isolation_key(), TestNIK());
EXPECT_GT(client->expiry_time(), base::TimeTicks::Now());
EXPECT_LT(client->expiry_time(), base::TimeTicks::Now() + base::Days(1));
}
TEST_F(PrefetchURLLoaderClientTest, MAYBE_RecordAndReplay) {
TEST_F(PrefetchURLLoaderClientTest, RecordAndReplay) {
StrictMock<MockURLLoaderClient> mock_client;
MockFunction<void()> checkpoint;
MockFunction<void()> disconnect;
DisconnectDetectingReceiver receiver(&mock_client, &disconnect);
{
InSequence s;
EXPECT_CALL(checkpoint, Call());
ExpectCallMojoMethods();
ExpectCallMojoMethods(mock_client);
EXPECT_CALL(disconnect, Call());
}
CallAllMojoMethods();
ResetClientPendingRemote();
auto* client = Emplace();
auto pending_remote = client->BindNewPipeAndPassRemote();
CallAllMojoMethods(client);
pending_remote.reset();
checkpoint.Call();
Consume();
set_disconnect_handler(&disconnect);
ClearClientPointer();
client->SetClient(receiver.GetPendingRemote());
RunUntilIdle();
}
// The difference from the previous test is that now Consume() is called
// The difference from the previous test is that now SetClient() is called
// before any of the delegating methods, so there's no need to record them.
TEST_F(PrefetchURLLoaderClientTest, MAYBE_DelegateDirectly) {
TEST_F(PrefetchURLLoaderClientTest, DelegateDirectly) {
StrictMock<MockURLLoaderClient> mock_client;
MockFunction<void()> checkpoint;
MockFunction<void()> disconnect;
DisconnectDetectingReceiver receiver(&mock_client, &disconnect);
{
InSequence s;
ExpectCallMojoMethods();
ExpectCallMojoMethods(mock_client);
EXPECT_CALL(disconnect, Call());
EXPECT_CALL(checkpoint, Call());
}
Consume();
set_disconnect_handler(&disconnect);
CallAllMojoMethods();
ClearClientPointer();
ResetClientPendingRemote();
auto* client = Emplace();
auto pending_remote = client->BindNewPipeAndPassRemote();
client->SetClient(receiver.GetPendingRemote());
CallAllMojoMethods(client);
pending_remote.reset();
RunUntilIdle();
checkpoint.Call();
}
@ -425,82 +338,117 @@ TEST_F(PrefetchURLLoaderClientTest, MAYBE_DelegateDirectly) {
// This test just verifies that all the recorded callbacks can be destroyed
// without leaks.
TEST_F(PrefetchURLLoaderClientTest, RecordAndDiscard) {
CallAllMojoMethods();
auto* client = Emplace();
CallAllMojoMethods(client);
RunUntilIdle();
}
// Verifies that setting the client after the response comes but before it
// completes works.
TEST_F(PrefetchURLLoaderClientTest, MAYBE_ReplayAfterResponse) {
TEST_F(PrefetchURLLoaderClientTest, ReplayAfterResponse) {
StrictMock<MockURLLoaderClient> mock_client;
MockFunction<void(int)> checkpoint;
MockFunction<void()> disconnect;
DisconnectDetectingReceiver receiver(&mock_client, &disconnect);
{
InSequence s;
EXPECT_CALL(checkpoint, Call(0));
EXPECT_CALL(mock_client(),
EXPECT_CALL(mock_client,
OnReceiveResponse(URLResponseHeadIsOk(), _,
Optional(BigBufferHasExpectedContents())))
.WillOnce(WithArg<1>(Invoke(CheckDataPipeContents)));
EXPECT_CALL(checkpoint, Call(1));
EXPECT_CALL(mock_client(), OnComplete(URLLoaderCompletionStatusIsOk()));
EXPECT_CALL(mock_client, OnComplete(URLLoaderCompletionStatusIsOk()));
EXPECT_CALL(checkpoint, Call(2));
EXPECT_CALL(disconnect, Call());
EXPECT_CALL(checkpoint, Call(3));
}
client()->OnReceiveResponse(TestURLResponseHead(), TestDataPipeConsumer(),
TestBigBuffer());
auto* client = Emplace();
auto pending_remote = client->BindNewPipeAndPassRemote();
client->OnReceiveResponse(TestURLResponseHead(), TestDataPipeConsumer(),
TestBigBuffer());
RunUntilIdle();
checkpoint.Call(0);
Consume();
set_disconnect_handler(&disconnect);
client->SetClient(receiver.GetPendingRemote());
RunUntilIdle();
checkpoint.Call(1);
client()->OnComplete(TestURLLoaderCompletionStatus());
client->OnComplete(TestURLLoaderCompletionStatus());
RunUntilIdle();
checkpoint.Call(2);
ClearClientPointer();
ResetClientPendingRemote();
pending_remote.reset();
RunUntilIdle();
checkpoint.Call(3);
}
TEST_F(PrefetchURLLoaderClientTest, MAYBE_ConsumeRemovesFromCache) {
Consume();
TEST_F(PrefetchURLLoaderClientTest, GetURLLoaderPendingReceiver) {
StrictMock<MockURLLoaderClient> mock_client;
MockFunction<void()> checkpoint;
MockFunction<void()> disconnect;
DisconnectDetectingReceiver receiver(&mock_client, &disconnect);
EXPECT_FALSE(
cache()->Lookup(client()->network_isolation_key(), client()->url()));
{
InSequence s;
EXPECT_CALL(checkpoint, Call());
EXPECT_CALL(disconnect, Call());
}
auto* client = Emplace();
auto pending_remote = client->BindNewPipeAndPassRemote();
auto pending_receiver = client->GetURLLoaderPendingReceiver();
EXPECT_TRUE(pending_receiver.is_valid());
checkpoint.Call();
client->SetClient(receiver.GetPendingRemote());
pending_receiver.reset();
RunUntilIdle();
}
TEST_F(PrefetchURLLoaderClientTest, SetClientRemovesFromCache) {
StrictMock<MockURLLoaderClient> mock_client;
mojo::Receiver<mojom::URLLoaderClient> receiver(&mock_client);
auto* client = Emplace();
auto pending_remote = client->BindNewPipeAndPassRemote();
client->SetClient(receiver.BindNewPipeAndPassRemote());
EXPECT_FALSE(cache()->Lookup(client->network_isolation_key(), client->url()));
}
TEST_F(PrefetchURLLoaderClientTest, BadResponseCode) {
constexpr auto kBadResponseCode = net::HTTP_NOT_FOUND;
TakeClientPointer()->OnReceiveResponse(
CreateURLResponseHead(kBadResponseCode), TestDataPipeConsumer(),
TestBigBuffer());
auto* client = Emplace();
client->OnReceiveResponse(CreateURLResponseHead(kBadResponseCode),
TestDataPipeConsumer(), TestBigBuffer());
// It should have been deleted from the cache.
EXPECT_FALSE(cache()->Lookup(TestNIK(), TestURL()));
}
TEST_F(PrefetchURLLoaderClientTest, BadHeader) {
auto* client = Emplace();
auto url_response_head = TestURLResponseHead();
url_response_head->headers->AddHeader("Vary", "Sec-Purpose, Set-Cookie");
TakeClientPointer()->OnReceiveResponse(
std::move(url_response_head), TestDataPipeConsumer(), TestBigBuffer());
client->OnReceiveResponse(std::move(url_response_head),
TestDataPipeConsumer(), TestBigBuffer());
// It should have been deleted from the cache.
EXPECT_FALSE(cache()->Lookup(TestNIK(), TestURL()));
}
TEST_F(PrefetchURLLoaderClientTest, NoStore) {
auto* client = Emplace();
auto url_response_head = TestURLResponseHead();
url_response_head->headers->AddHeader("Cache-Control", "no-cache, no-store");
TakeClientPointer()->OnReceiveResponse(
std::move(url_response_head), TestDataPipeConsumer(), TestBigBuffer());
client->OnReceiveResponse(std::move(url_response_head),
TestDataPipeConsumer(), TestBigBuffer());
// It should have been deleted from the cache.
EXPECT_FALSE(cache()->Lookup(TestNIK(), TestURL()));

@ -457,43 +457,17 @@ BASE_FEATURE(kDocumentIsolationPolicy,
"DocumentIsolationPolicy",
base::FEATURE_DISABLED_BY_DEFAULT);
// This feature enables the Prefetch() method on the NetworkContext, and makes
// the PrefetchMatchingURLLoaderFactory check the match quality.
// This feature enables the Prefetch() method on the NetworkContext, and the
// PrefetchMatchingURLLoaderFactory.
BASE_FEATURE(kNetworkContextPrefetch,
"NetworkContextPrefetch",
base::FEATURE_DISABLED_BY_DEFAULT);
// How many prefetches should be cached before old ones are evicted. This
// provides rough control over the overall memory used by prefetches.
const base::FeatureParam<int> kNetworkContextPrefetchMaxLoaders{
&kNetworkContextPrefetch,
/*name=*/"max_loaders", /*default_value=*/10};
// When "NetworkContextPrefetchUseMatches" is disabled, how long to leave a
// matched cache entry alive before deleting it. This corresponds to the
// expected maximum time it will take for a request to reach the HttpCache once
// it has been initiated. Since it may be delayed by the ResourceScheduler, give
// the delay is quite large.
//
// Why not shorter: the request from the render process may be delayed by the
// ResourceScheduler.
//
// Why not longer: If the prefetch has not yet received response headers, it
// has an exclusive cache lock. The real request from the render process
// cannot proceed until the cache lock is released. If the response turns out
// to be uncacheable, then this time is pure waste.
const base::FeatureParam<base::TimeDelta> kNetworkContextPrefetchEraseGraceTime{
&kNetworkContextPrefetch, /*name=*/"erase_grace_time",
/*default_value=*/base::Seconds(1)};
// This feature makes the matching fetches performed by the Prefetch() actually
// be consumed directly by renderers. When this is disabled, the disk cache
// entry may be reused but the original URLLoader is cancelled. Does nothing
// unless "NetworkContextPrefetch" is also enabled.
BASE_FEATURE(kNetworkContextPrefetchUseMatches,
"NetworkContextPrefetchUseMatches",
base::FEATURE_DISABLED_BY_DEFAULT);
// This feature enables treating 0.0.0.0/8 as the public address space instead
// of private or local. This is a killswitch for a tightening of a loophole in
// Private Network Access. See https://crbug.com/40058874.

@ -177,21 +177,12 @@ BASE_DECLARE_FEATURE(kAvoidResourceRequestCopies);
COMPONENT_EXPORT(NETWORK_CPP) BASE_DECLARE_FEATURE(kDocumentIsolationPolicy);
// To actually use the prefetch results, it's also necessary to enable
// kNetworkContextPrefetchUseCache, below.
COMPONENT_EXPORT(NETWORK_CPP)
BASE_DECLARE_FEATURE(kNetworkContextPrefetch);
COMPONENT_EXPORT(NETWORK_CPP)
extern const base::FeatureParam<int> kNetworkContextPrefetchMaxLoaders;
COMPONENT_EXPORT(NETWORK_CPP)
extern const base::FeatureParam<base::TimeDelta>
kNetworkContextPrefetchEraseGraceTime;
COMPONENT_EXPORT(NETWORK_CPP)
BASE_DECLARE_FEATURE(kNetworkContextPrefetchUseMatches);
COMPONENT_EXPORT(NETWORK_CPP)
BASE_DECLARE_FEATURE(kTreatNullIPAsPublicAddressSpace);

@ -49,8 +49,6 @@ namespace network {
//
// Note: Please revise EqualsForTesting accordingly on any updates to this
// struct.
//
// LINT.IfChange(ResourceRequest)
struct COMPONENT_EXPORT(NETWORK_CPP_BASE) ResourceRequest {
// Typemapped to network.mojom.TrustedUrlRequestParams, see comments there
// for details of each field.
@ -226,7 +224,6 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) ResourceRequest {
bool is_ad_tagged = false;
std::optional<base::UnguessableToken> prefetch_token;
};
// LINT.ThenChange(//services/network/prefetch_matches.cc)
// This does not accept |kDefault| referrer policy.
COMPONENT_EXPORT(NETWORK_CPP_BASE)