Skip to content

Commit

Permalink
[FLEDGE] Preconnect for trusted signals requests.
Browse files Browse the repository at this point in the history
This CL adds preconnect calls when creating an
AuctionUrlLoaderFactoryProxy for bidders or sellers in a FLEDGE auciton.
These proxies are created at the time worklets are created - so we
either have, or are creating, a process to load the worklets in. This
means that in auctions with lots of buyer origins, the buyer preconnects
won't time out before we need them (Though could try being more
aggressive about timing).

The seller process is created before creating any bidder worklets, so
it's possible the seller connection will time out before any bids have
been generated (e.g., other auctions could be using all the bidder
worklet process quota), but that seems unlikely.

Bug: 1380999
Change-Id: I7a27b87c2cd335cbdd1094cc1bcc4975e74de30e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4001043
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1067395}
  • Loading branch information
Matt Menke authored and Chromium LUCI CQ committed Nov 4, 2022
1 parent f4eb426 commit 3068f8b
Show file tree
Hide file tree
Showing 9 changed files with 125 additions and 15 deletions.
10 changes: 10 additions & 0 deletions content/browser/interest_group/ad_auction_service_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,16 @@ AdAuctionServiceImpl::GetTrustedURLLoaderFactory() {
return trusted_url_loader_factory_.get();
}

void AdAuctionServiceImpl::PreconnectSocket(
const GURL& url,
const net::NetworkAnonymizationKey& network_anonymization_key) {
render_frame_host()
.GetStoragePartition()
->GetNetworkContext()
->PreconnectSockets(/*num_streams=*/1, url, /*allow_credentials=*/false,
network_anonymization_key);
}

scoped_refptr<network::WrapperSharedURLLoaderFactory>
AdAuctionServiceImpl::GetRefCountedTrustedURLLoaderFactory() {
GetTrustedURLLoaderFactory();
Expand Down
3 changes: 3 additions & 0 deletions content/browser/interest_group/ad_auction_service_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ class CONTENT_EXPORT AdAuctionServiceImpl final
// AuctionWorkletManager::Delegate implementation:
network::mojom::URLLoaderFactory* GetFrameURLLoaderFactory() override;
network::mojom::URLLoaderFactory* GetTrustedURLLoaderFactory() override;
void PreconnectSocket(
const GURL& url,
const net::NetworkAnonymizationKey& network_anonymization_key) override;
RenderFrameHostImpl* GetFrame() override;
scoped_refptr<SiteInstance> GetFrameSiteInstance() override;
network::mojom::ClientSecurityStatePtr GetClientSecurityState() override;
Expand Down
3 changes: 3 additions & 0 deletions content/browser/interest_group/auction_runner_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1941,6 +1941,9 @@ class AuctionRunnerTest : public testing::Test,
network::mojom::URLLoaderFactory* GetTrustedURLLoaderFactory() override {
return &url_loader_factory_;
}
void PreconnectSocket(
const GURL& url,
const net::NetworkAnonymizationKey& network_anonymization_key) override {}
RenderFrameHostImpl* GetFrame() override { return nullptr; }
scoped_refptr<SiteInstance> GetFrameSiteInstance() override {
return scoped_refptr<SiteInstance>();
Expand Down
35 changes: 24 additions & 11 deletions content/browser/interest_group/auction_url_loader_factory_proxy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "mojo/public/cpp/bindings/pending_remote.h"
#include "mojo/public/cpp/bindings/receiver.h"
#include "net/base/isolation_info.h"
#include "net/base/network_anonymization_key.h"
#include "net/cookies/site_for_cookies.h"
#include "net/http/http_request_headers.h"
#include "net/traffic_annotation/network_traffic_annotation.h"
Expand All @@ -32,10 +33,23 @@

namespace content {

namespace {

// Helper to create the IsolationInfo used for all bidder requests. A helper
// method is used to avoid having to construct two copies of `bidder_origin`.
net::IsolationInfo CreateBidderIsolationInfo(const url::Origin& bidder_origin) {
return net::IsolationInfo::Create(net::IsolationInfo::RequestType::kOther,
bidder_origin, bidder_origin,
net::SiteForCookies());
}

} // namespace

AuctionURLLoaderFactoryProxy::AuctionURLLoaderFactoryProxy(
mojo::PendingReceiver<network::mojom::URLLoaderFactory> pending_receiver,
GetUrlLoaderFactoryCallback get_frame_url_loader_factory,
GetUrlLoaderFactoryCallback get_trusted_url_loader_factory,
PreconnectSocketCallback preconnect_socket_callback,
const url::Origin& top_frame_origin,
const url::Origin& frame_origin,
bool is_for_seller,
Expand All @@ -51,10 +65,18 @@ AuctionURLLoaderFactoryProxy::AuctionURLLoaderFactoryProxy(
frame_origin_(frame_origin),
is_for_seller_(is_for_seller),
client_security_state_(std::move(client_security_state)),
isolation_info_(is_for_seller ? net::IsolationInfo::CreateTransient()
: CreateBidderIsolationInfo(
url::Origin::Create(script_url))),
script_url_(script_url),
wasm_url_(wasm_url),
trusted_signals_base_url_(trusted_signals_base_url) {
DCHECK(client_security_state_);
if (trusted_signals_base_url_) {
std::move(preconnect_socket_callback)
.Run(*trusted_signals_base_url_,
isolation_info_.network_anonymization_key());
}
}

AuctionURLLoaderFactoryProxy::~AuctionURLLoaderFactoryProxy() = default;
Expand Down Expand Up @@ -132,14 +154,8 @@ void AuctionURLLoaderFactoryProxy::CreateLoaderAndStart(
// Other URLs combine a base URL from the frame's Javascript and data from
// bidding interest groups, so use a (single) transient IsolationInfo for
// them, to prevent exposing data across sites.
if (isolation_info_for_seller_signals_.IsEmpty()) {
isolation_info_for_seller_signals_ =
net::IsolationInfo::CreateTransient();
}

new_request.trusted_params = network::ResourceRequest::TrustedParams();
new_request.trusted_params->isolation_info =
isolation_info_for_seller_signals_;
new_request.trusted_params->isolation_info = isolation_info_;
new_request.trusted_params->client_security_state =
client_security_state_.Clone();
}
Expand All @@ -152,10 +168,7 @@ void AuctionURLLoaderFactoryProxy::CreateLoaderAndStart(
// NIK leaks information). These leaks need to be fixed.
new_request.mode = network::mojom::RequestMode::kNoCors;
new_request.trusted_params = network::ResourceRequest::TrustedParams();
url::Origin origin = url::Origin::Create(url_request.url);
new_request.trusted_params->isolation_info =
net::IsolationInfo::Create(net::IsolationInfo::RequestType::kOther,
origin, origin, net::SiteForCookies());
new_request.trusted_params->isolation_info = isolation_info_;
new_request.trusted_params->client_security_state =
client_security_state_.Clone();
}
Expand Down
23 changes: 20 additions & 3 deletions content/browser/interest_group/auction_url_loader_factory_proxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@
#include "url/gurl.h"
#include "url/origin.h"

namespace net {
class NetworkAnonymizationKey;
}

namespace content {

// Proxy URLLoaderFactoryFactory, to limit the requests that an auction worklet
Expand All @@ -31,6 +35,11 @@ class CONTENT_EXPORT AuctionURLLoaderFactoryProxy
using GetUrlLoaderFactoryCallback =
base::RepeatingCallback<network::mojom::URLLoaderFactory*()>;

// Callback to preconnect a single socket with the given info.
using PreconnectSocketCallback = base::OnceCallback<void(
const GURL& url,
const net::NetworkAnonymizationKey& network_anonymization_key)>;

// Passed in callbacks must be safe to call at any time during the lifetime of
// the AuctionURLLoaderFactoryProxy.
//
Expand All @@ -41,6 +50,12 @@ class CONTENT_EXPORT AuctionURLLoaderFactoryProxy
// `get_trusted_url_loader_factory` returns a trusted URLLoaderFactory. Used
// for bidder worklet script and trusted selling signals fetches.
//
// `preconnect_socket_callback` is used to issue a preconnect if there's a
// non-empty trusted signals URL. No preconnect is made for the JS and web
// assembly, since they should be cacheable, and currently erring on the side
// of not making unnecessary connections. Invoked immediately, if it's going
// to be invoked at all.
//
// `frame_origin` is the origin of the frame running the auction. Used as the
// initiator.
//
Expand All @@ -62,6 +77,7 @@ class CONTENT_EXPORT AuctionURLLoaderFactoryProxy
mojo::PendingReceiver<network::mojom::URLLoaderFactory> pending_receiver,
GetUrlLoaderFactoryCallback get_frame_url_loader_factory,
GetUrlLoaderFactoryCallback get_trusted_url_loader_factory,
PreconnectSocketCallback preconnect_socket_callback,
const url::Origin& top_frame_origin,
const url::Origin& frame_origin,
bool is_for_seller,
Expand Down Expand Up @@ -111,9 +127,10 @@ class CONTENT_EXPORT AuctionURLLoaderFactoryProxy
const bool is_for_seller_;
const network::mojom::ClientSecurityStatePtr client_security_state_;

// Transient IsolationInfo used for all seller requests to trusted bidding
// signals URLs. Populated on first fetch it applies to.
net::IsolationInfo isolation_info_for_seller_signals_;
// IsolationInfo used for requests using the trusted URLLoaderFactory. A
// Transient IsolationInfo for sellers, the bidder's IsolationInfo for
// bidders.
const net::IsolationInfo isolation_info_;

const GURL script_url_;
const absl::optional<GURL> wasm_url_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
#include "mojo/public/cpp/bindings/pending_receiver.h"
#include "mojo/public/cpp/bindings/pending_remote.h"
#include "net/base/isolation_info.h"
#include "net/base/network_anonymization_key.h"
#include "net/base/schemeful_site.h"
#include "net/cookies/site_for_cookies.h"
#include "net/traffic_annotation/network_traffic_annotation.h"
#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
Expand Down Expand Up @@ -60,14 +62,25 @@ class AuctionUrlLoaderFactoryProxyTest : public testing::Test {
client_security_state_->is_web_secure_context = true;
}

~AuctionUrlLoaderFactoryProxyTest() override = default;
~AuctionUrlLoaderFactoryProxyTest() override {
// This should be both set and cleared during CreateUrlLoaderFactoryProxy().
// Check that the callback passed to AuctionURLLoaderFactoryProxy's wasn't
// invoked asynchronously unexpectedly.
EXPECT_FALSE(preconnect_url_);
}

void CreateUrlLoaderFactoryProxy() {
// The AuctionURLLoaderFactoryProxy should only be created if there is no
// old one, or the old one's pipe was closed.
DCHECK(!remote_url_loader_factory_ ||
!remote_url_loader_factory_.is_connected());

// `preconnect_url_` should only be set by callback (sometimes) invoked by
// AuctionURLLoaderFactoryProxy's constructor, and cleared at the end of
// CreateUrlLoaderFactoryProxy(). Check that it's nullopt here to catch it
// being set unexpectedly.
EXPECT_TRUE(!preconnect_url_);

remote_url_loader_factory_.reset();
url_loader_factory_proxy_ = std::make_unique<AuctionURLLoaderFactoryProxy>(
remote_url_loader_factory_.BindNewPipeAndPassReceiver(),
Expand All @@ -77,9 +90,37 @@ class AuctionUrlLoaderFactoryProxyTest : public testing::Test {
base::BindRepeating(
[](network::mojom::URLLoaderFactory* factory) { return factory; },
&trusted_url_loader_factory_),
base::BindOnce(&AuctionUrlLoaderFactoryProxyTest::PreconnectSocket,
base::Unretained(this)),
top_frame_origin_, frame_origin_, is_for_seller_,
client_security_state_.Clone(), GURL(kScriptUrl), wasm_url_,
trusted_signals_base_url_);

EXPECT_EQ(preconnect_url_, trusted_signals_base_url_);
if (trusted_signals_base_url_) {
if (is_for_seller_) {
EXPECT_TRUE(preconnect_network_anonymization_key_->IsTransient());
} else {
net::SchemefulSite buyer_site{GURL(kScriptUrl)};
EXPECT_EQ(preconnect_network_anonymization_key_,
net::NetworkAnonymizationKey(buyer_site, buyer_site));
}
} else {
// This check is not strictly needed, since `preconnect_url_` being equal
// to `trusted_signals_base_url_`, as checked above, implies this must
// also be nullopt if `trusted_signals_base_url_` is nullopt.
EXPECT_FALSE(preconnect_network_anonymization_key_);
}
preconnect_url_ = absl::nullopt;
}

void PreconnectSocket(
const GURL& url,
const net::NetworkAnonymizationKey& network_anonymization_key) {
EXPECT_TRUE(!preconnect_url_);

preconnect_url_ = url;
preconnect_network_anonymization_key_ = network_anonymization_key;
}

// Attempts to make a request for `request`.
Expand Down Expand Up @@ -222,6 +263,12 @@ class AuctionUrlLoaderFactoryProxyTest : public testing::Test {
EXPECT_TRUE(observed_request.trusted_params->isolation_info
.network_isolation_key()
.IsTransient());
// There should have been a preconnect in this case, with a
// NetworkAnonymizationKey that's consistent with the request's
// IsolationInfo.
EXPECT_EQ(preconnect_network_anonymization_key_,
observed_request.trusted_params->isolation_info
.network_anonymization_key());
EXPECT_EQ(*client_security_state_,
*observed_request.trusted_params->client_security_state);
}
Expand Down Expand Up @@ -276,6 +323,10 @@ class AuctionUrlLoaderFactoryProxyTest : public testing::Test {
url::Origin::Create(GURL("https://top.test/"));
url::Origin frame_origin_ = url::Origin::Create(GURL("https://foo.test/"));

absl::optional<GURL> preconnect_url_;
absl::optional<net::NetworkAnonymizationKey>
preconnect_network_anonymization_key_;

network::TestURLLoaderFactory frame_url_loader_factory_;
network::TestURLLoaderFactory trusted_url_loader_factory_;
std::unique_ptr<AuctionURLLoaderFactoryProxy> url_loader_factory_proxy_;
Expand Down
1 change: 1 addition & 0 deletions content/browser/interest_group/auction_worklet_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ void AuctionWorkletManager::WorkletOwner::OnProcessAssigned() {
base::Unretained(delegate)),
base::BindRepeating(&Delegate::GetTrustedURLLoaderFactory,
base::Unretained(delegate)),
base::BindOnce(&Delegate::PreconnectSocket, base::Unretained(delegate)),
worklet_manager_->top_window_origin(), worklet_manager_->frame_origin(),
/*is_for_seller_=*/worklet_info_.type == WorkletType::kSeller,
delegate->GetClientSecurityState(), worklet_info_.script_url,
Expand Down
9 changes: 9 additions & 0 deletions content/browser/interest_group/auction_worklet_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@
#include "url/gurl.h"
#include "url/origin.h"

namespace net {
class NetworkAnonymizationKey;
}

namespace content {

class RenderFrameHostImpl;
Expand Down Expand Up @@ -82,6 +86,11 @@ class CONTENT_EXPORT AuctionWorkletManager {
// signals.
virtual network::mojom::URLLoaderFactory* GetTrustedURLLoaderFactory() = 0;

// Preconnects a single uncredentialed socket with the provided parameters.
virtual void PreconnectSocket(
const GURL& url,
const net::NetworkAnonymizationKey& network_anonymization_key) = 0;

// Get containing frame. (Passed to debugging hooks).
virtual RenderFrameHostImpl* GetFrame() = 0;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,9 @@ class AuctionWorkletManagerTest : public testing::Test,
network::mojom::URLLoaderFactory* GetTrustedURLLoaderFactory() override {
return &url_loader_factory_;
}
void PreconnectSocket(
const GURL& url,
const net::NetworkAnonymizationKey& network_anonymization_key) override {}
RenderFrameHostImpl* GetFrame() override { return nullptr; }
scoped_refptr<SiteInstance> GetFrameSiteInstance() override {
return scoped_refptr<SiteInstance>();
Expand Down

0 comments on commit 3068f8b

Please sign in to comment.