Skip to content

GitLab

T
tangled
Commit ae812c0c authored by Alexey Baskakov's avatar Alexey Baskakov Committed by Chromium LUCI CQ
Browse files
Options

Revert "Adding more navigation-like fields to search prefetch requests" 

This reverts commit f8a44173.

Reason for revert: Breaks browser tests here
https://ci.chromium.org/ui/p/chromium/builders/ci/Mac10.15%20Tests/6369/overview

Original change's description:
> Adding more navigation-like fields to search prefetch requests
>
> This CL adds a few fields to the search prefetch resource request to
> make it more similar to a navigation request. Additionally, this adds
> capability for URLLoader throttles to defer (in this case cancel) or
> change headers on the prefetch request allowing things like variation
> headers to be added more similarly to how navigation requests have them
> added.
>
> Client hints CL:
> https://chromium-review.googlesource.com/c/chromium/src/+/2551671
>
> Bug: 1138648
> Change-Id: Ia868aa624b2f9f573a54432eb0fa62834eacd2f5
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2552723
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Reviewed-by: Robert Ogden <robertogden@chromium.org>
> Commit-Queue: Ryan Sturm <ryansturm@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#833987}

TBR=kinuko@chromium.org,robertogden@chromium.org,ryansturm@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1138648
Change-Id: I7077c0f73dbc60ddd5541caed7c1a91adf04b208
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2574640Reviewed-by: default avatarAlexey Baskakov <loyso@chromium.org>
Reviewed-by: default avatarRyan Sturm <ryansturm@chromium.org>
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834106}
parent 7aedee51
Hide whitespace changes
Inline Side-by-side
Showing with 7 additions and 289 deletions
chrome/browser/prefetch/search_prefetch/base_search_prefetch_request.cc
View file @ ae812c0c
......@@ -4,80 +4,18 @@
#include "chrome/browser/prefetch/search_prefetch/base_search_prefetch_request.h"
#include <vector>
#include "chrome/browser/chrome_content_browser_client.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/common/pref_names.h"
#include "components/prefs/pref_service.h"
#include "components/variations/net/variations_http_headers.h"
#include "content/public/browser/client_hints.h"
#include "content/public/browser/frame_accept_header.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/url_loader_throttles.h"
#include "content/public/browser/web_contents.h"
#include "content/public/common/content_constants.h"
#include "net/base/load_flags.h"
#include "net/cookies/site_for_cookies.h"
#include "net/http/http_response_headers.h"
#include "net/http/http_status_code.h"
#include "net/traffic_annotation/network_traffic_annotation.h"
#include "services/network/public/cpp/resource_request.h"
#include "services/network/public/mojom/fetch_api.mojom.h"
#include "third_party/blink/public/common/loader/url_loader_throttle.h"
#include "third_party/blink/public/mojom/loader/resource_load_info.mojom-shared.h"
#include "url/origin.h"
namespace {
// A custom URLLoaderThrottle delegate that is very sensitive. Anything that
// would delay or cancel the request is treated the same, which would prevent
// the prefetch request.
class CheckForCancelledOrPausedDelegate
: public blink::URLLoaderThrottle::Delegate {
public:
CheckForCancelledOrPausedDelegate() = default;
~CheckForCancelledOrPausedDelegate() override = default;
CheckForCancelledOrPausedDelegate(const CheckForCancelledOrPausedDelegate&) =
delete;
CheckForCancelledOrPausedDelegate& operator=(
const CheckForCancelledOrPausedDelegate&) = delete;
// URLLoaderThrottle::Delegate:
void CancelWithError(int error_code,
base::StringPiece custom_reason) override {
cancelled_or_paused_ = true;
}
void Resume() override {}
void PauseReadingBodyFromNet() override { cancelled_or_paused_ = true; }
void RestartWithFlags(int additional_load_flags) override {
cancelled_or_paused_ = true;
}
void RestartWithURLResetAndFlags(int additional_load_flags) override {
cancelled_or_paused_ = true;
}
void RestartWithURLResetAndFlagsNow(int additional_load_flags) override {
cancelled_or_paused_ = true;
}
void RestartWithModifiedHeadersNow(
const net::HttpRequestHeaders& modified_headers) override {
cancelled_or_paused_ = true;
}
bool cancelled_or_paused() const { return cancelled_or_paused_; }
private:
bool cancelled_or_paused_ = false;
};
} // namespace
BaseSearchPrefetchRequest::BaseSearchPrefetchRequest(
const GURL& prefetch_url,
......@@ -87,7 +25,7 @@ BaseSearchPrefetchRequest::BaseSearchPrefetchRequest(
BaseSearchPrefetchRequest::~BaseSearchPrefetchRequest() = default;
bool BaseSearchPrefetchRequest::StartPrefetchRequest(Profile* profile) {
void BaseSearchPrefetchRequest::StartPrefetchRequest(Profile* profile) {
net::NetworkTrafficAnnotationTag network_traffic_annotation =
net::DefineNetworkTrafficAnnotation("search_prefetch_service", R"(
semantics {
......@@ -123,8 +61,6 @@ bool BaseSearchPrefetchRequest::StartPrefetchRequest(Profile* profile) {
}
})");
url::Origin prefetch_origin = url::Origin::Create(prefetch_url_);
auto resource_request = std::make_unique<network::ResourceRequest>();
resource_request->load_flags |= net::LOAD_PREFETCH;
resource_request->url = prefetch_url_;
......@@ -134,26 +70,8 @@ bool BaseSearchPrefetchRequest::StartPrefetchRequest(Profile* profile) {
resource_request->report_raw_headers = true;
resource_request->credentials_mode =
network::mojom::CredentialsMode::kInclude;
resource_request->method = "GET";
resource_request->mode = network::mojom::RequestMode::kNavigate;
resource_request->site_for_cookies =
net::SiteForCookies::FromUrl(prefetch_url_);
resource_request->destination = network::mojom::RequestDestination::kDocument;
resource_request->resource_type =
static_cast<int>(blink::mojom::ResourceType::kMainFrame);
resource_request->trusted_params = network::ResourceRequest::TrustedParams();
// We don't handle redirects, so |kOther| makes sense here.
resource_request->trusted_params->isolation_info = net::IsolationInfo::Create(
net::IsolationInfo::RequestType::kOther, prefetch_origin, prefetch_origin,
resource_request->site_for_cookies);
resource_request->referrer_policy = net::ReferrerPolicy::NO_REFERRER;
// Tack an 'Upgrade-Insecure-Requests' header to outgoing navigational
// requests, as described in
// https://w3c.github.io/webappsec/specs/upgrade/#feature-detect
resource_request->headers.SetHeader("Upgrade-Insecure-Requests", "1");
resource_request->headers.SetHeader(net::HttpRequestHeaders::kUserAgent,
GetUserAgent());
variations::AppendVariationsHeaderUnknownSignedIn(
prefetch_url_, variations::InIncognito::kNo, resource_request.get());
resource_request->headers.SetHeader(content::kCorsExemptPurposeHeaderName,
"prefetch");
resource_request->headers.SetHeader(
......@@ -168,37 +86,13 @@ bool BaseSearchPrefetchRequest::StartPrefetchRequest(Profile* profile) {
profile->GetClientHintsControllerDelegate(),
/*is_ua_override_on=*/false, js_enabled);
// Before sending out the request, allow throttles to modify the request (not
// the URL). The rest of the URL Loader throttle calls are captured in the
// navigation stack. Headers can be added by throttles at this point, which we
// want to capture.
auto wc_getter =
base::BindRepeating([]() -> content::WebContents* { return nullptr; });
std::vector<std::unique_ptr<blink::URLLoaderThrottle>> throttles =
content::CreateContentBrowserURLLoaderThrottles(
*resource_request, profile, std::move(wc_getter),
/*navigation_ui_data=*/nullptr,
content::RenderFrameHost::kNoFrameTreeNodeId);
bool should_defer = false;
for (auto& throttle : throttles) {
CheckForCancelledOrPausedDelegate cancel_or_pause_delegate;
throttle->set_delegate(&cancel_or_pause_delegate);
throttle->WillStartRequest(resource_request.get(), &should_defer);
// Make sure throttles are deleted before |cancel_or_pause_delegate| in case
// they call into the delegate in the destructor.
throttle.reset();
if (should_defer || resource_request->url != prefetch_url_ ||
cancel_or_pause_delegate.cancelled_or_paused()) {
return false;
}
}
// TODO(ryansturm): Find other headers that may need to be set.
// https://crbug.com/1138648
current_status_ = SearchPrefetchStatus::kInFlight;
StartPrefetchRequestInternal(profile, std::move(resource_request),
network_traffic_annotation);
return true;
}
void BaseSearchPrefetchRequest::CancelPrefetch() {
......
chrome/browser/prefetch/search_prefetch/base_search_prefetch_request.h
View file @ ae812c0c
......@@ -49,10 +49,8 @@ class BaseSearchPrefetchRequest {
delete;
// Starts the network request to prefetch |prefetch_url_|. Sets various fields
// on a resource request and calls |StartPrefetchRequestInternal()|. Returns
// |false| if the request is not started (i.e., it would be deferred by
// throttles).
bool StartPrefetchRequest(Profile* profile);
// on a resource request and calls |StartPrefetchRequestInternal()|.
void StartPrefetchRequest(Profile* profile);
// Marks a prefetch as canceled and stops any ongoing fetch.
void CancelPrefetch();
......
chrome/browser/prefetch/search_prefetch/search_prefetch_service.cc
View file @ ae812c0c
......@@ -127,9 +127,6 @@ bool SearchPrefetchService::MaybePrefetchURL(const GURL& url) {
}
DCHECK(prefetch_request);
if (!prefetch_request->StartPrefetchRequest(profile_)) {
return false;
}
prefetches_.emplace(search_terms, std::move(prefetch_request));
prefetches_[search_terms]->StartPrefetchRequest(profile_);
......
chrome/browser/prefetch/search_prefetch/search_prefetch_service_browsertest.cc
View file @ ae812c0c
......@@ -59,8 +59,6 @@ constexpr char kOmniboxSuggestPrefetchSecondItemQuery[] = "porgsandwich";
constexpr char kOmniboxSuggestNonPrefetchQuery[] = "puffins";
constexpr char kLoadInSubframe[] = "/load_in_subframe";
constexpr char kClientHintsURL[] = "/accept_ch_with_lifetime.html";
constexpr char kThrottleHeader[] = "porgs-header";
constexpr char kThrottleHeaderValue[] = "porgs-header-value";
} // namespace
// A response that hangs after serving the start of the response.
......@@ -75,99 +73,6 @@ class HangRequestAfterStart : public net::test_server::BasicHttpResponse {
}
};
// A delegate to cancel prefetch requests by setting |defer| to true.
class DeferringThrottle : public blink::URLLoaderThrottle {
public:
DeferringThrottle() = default;
~DeferringThrottle() override = default;
void WillStartRequest(network::ResourceRequest* request,
bool* defer) override {
*defer = true;
}
};
class ThrottleAllContentBrowserClient : public ChromeContentBrowserClient {
public:
ThrottleAllContentBrowserClient() = default;
~ThrottleAllContentBrowserClient() override = default;
// ContentBrowserClient overrides:
std::vector<std::unique_ptr<blink::URLLoaderThrottle>>
CreateURLLoaderThrottles(
const network::ResourceRequest& request,
content::BrowserContext* browser_context,
const base::RepeatingCallback<content::WebContents*()>& wc_getter,
content::NavigationUIData* navigation_ui_data,
int frame_tree_node_id) override {
std::vector<std::unique_ptr<blink::URLLoaderThrottle>> throttles;
throttles.push_back(std::make_unique<DeferringThrottle>());
return throttles;
}
};
// A delegate to cancel prefetch requests by calling cancel on |delegate_|.
class CancellingThrottle : public blink::URLLoaderThrottle {
public:
CancellingThrottle() = default;
~CancellingThrottle() override = default;
void WillStartRequest(network::ResourceRequest* request,
bool* defer) override {
delegate_->CancelWithError(net::ERR_ABORTED);
}
};
class CancelAllContentBrowserClient : public ChromeContentBrowserClient {
public:
CancelAllContentBrowserClient() = default;
~CancelAllContentBrowserClient() override = default;
// ContentBrowserClient overrides:
std::vector<std::unique_ptr<blink::URLLoaderThrottle>>
CreateURLLoaderThrottles(
const network::ResourceRequest& request,
content::BrowserContext* browser_context,
const base::RepeatingCallback<content::WebContents*()>& wc_getter,
content::NavigationUIData* navigation_ui_data,
int frame_tree_node_id) override {
std::vector<std::unique_ptr<blink::URLLoaderThrottle>> throttles;
throttles.push_back(std::make_unique<CancellingThrottle>());
return throttles;
}
};
// A delegate to add a custom header to prefetches.
class AddHeaderModifyingThrottle : public blink::URLLoaderThrottle {
public:
AddHeaderModifyingThrottle() = default;
~AddHeaderModifyingThrottle() override = default;
void WillStartRequest(network::ResourceRequest* request,
bool* defer) override {
request->headers.SetHeader(kThrottleHeader, kThrottleHeaderValue);
}
};
class AddHeaderContentBrowserClient : public ChromeContentBrowserClient {
public:
AddHeaderContentBrowserClient() = default;
~AddHeaderContentBrowserClient() override = default;
// ContentBrowserClient overrides:
std::vector<std::unique_ptr<blink::URLLoaderThrottle>>
CreateURLLoaderThrottles(
const network::ResourceRequest& request,
content::BrowserContext* browser_context,
const base::RepeatingCallback<content::WebContents*()>& wc_getter,
content::NavigationUIData* navigation_ui_data,
int frame_tree_node_id) override {
std::vector<std::unique_ptr<blink::URLLoaderThrottle>> throttles;
throttles.push_back(std::make_unique<AddHeaderModifyingThrottle>());
return throttles;
}
};
class SearchPrefetchBaseBrowserTest : public InProcessBrowserTest {
public:
SearchPrefetchBaseBrowserTest() {
......@@ -633,9 +538,6 @@ IN_PROC_BROWSER_TEST_P(SearchPrefetchServiceEnabledBrowserTest,
EXPECT_EQ(1u, search_server_prefetch_request_count());
// Make sure we don't get client hints headers by default.
EXPECT_FALSE(base::Contains(headers, "viewport-width"));
EXPECT_TRUE(base::Contains(headers, "User-Agent"));
ASSERT_TRUE(base::Contains(headers, "Upgrade-Insecure-Requests"));
EXPECT_TRUE(base::Contains(headers["Upgrade-Insecure-Requests"], "1"));
prefetch_status = search_prefetch_service->GetSearchPrefetchStatusForTesting(
base::ASCIIToUTF16(search_terms));
......@@ -643,79 +545,6 @@ IN_PROC_BROWSER_TEST_P(SearchPrefetchServiceEnabledBrowserTest,
EXPECT_EQ(SearchPrefetchStatus::kCanBeServed, prefetch_status.value());
}
IN_PROC_BROWSER_TEST_P(SearchPrefetchServiceEnabledBrowserTest,
PrefetchThrottled) {
ThrottleAllContentBrowserClient browser_client;
auto* old_client = content::SetBrowserClientForTesting(&browser_client);
auto* search_prefetch_service =
SearchPrefetchServiceFactory::GetForProfile(browser()->profile());
EXPECT_NE(nullptr, search_prefetch_service);
std::string search_terms = "prefetch_content";
GURL prefetch_url = GetSearchServerQueryURL(search_terms);
EXPECT_FALSE(search_prefetch_service->MaybePrefetchURL(prefetch_url));
auto prefetch_status =
search_prefetch_service->GetSearchPrefetchStatusForTesting(
base::ASCIIToUTF16(search_terms));
EXPECT_FALSE(prefetch_status.has_value());
content::SetBrowserClientForTesting(old_client);
}
IN_PROC_BROWSER_TEST_P(SearchPrefetchServiceEnabledBrowserTest,
PrefetchCancelledByThrottle) {
CancelAllContentBrowserClient browser_client;
auto* old_client = content::SetBrowserClientForTesting(&browser_client);
auto* search_prefetch_service =
SearchPrefetchServiceFactory::GetForProfile(browser()->profile());
EXPECT_NE(nullptr, search_prefetch_service);
std::string search_terms = "prefetch_content";
GURL prefetch_url = GetSearchServerQueryURL(search_terms);
EXPECT_FALSE(search_prefetch_service->MaybePrefetchURL(prefetch_url));
auto prefetch_status =
search_prefetch_service->GetSearchPrefetchStatusForTesting(
base::ASCIIToUTF16(search_terms));
EXPECT_FALSE(prefetch_status.has_value());
content::SetBrowserClientForTesting(old_client);
}
IN_PROC_BROWSER_TEST_P(SearchPrefetchServiceEnabledBrowserTest,
PrefetchThrottleAddsHeader) {
AddHeaderContentBrowserClient browser_client;
auto* old_client = content::SetBrowserClientForTesting(&browser_client);
auto* search_prefetch_service =
SearchPrefetchServiceFactory::GetForProfile(browser()->profile());
EXPECT_NE(nullptr, search_prefetch_service);
std::string search_terms = "prefetch_content";
GURL prefetch_url = GetSearchServerQueryURL(search_terms);
EXPECT_TRUE(search_prefetch_service->MaybePrefetchURL(prefetch_url));
auto prefetch_status =
search_prefetch_service->GetSearchPrefetchStatusForTesting(
base::ASCIIToUTF16(search_terms));
ASSERT_TRUE(prefetch_status.has_value());
EXPECT_EQ(SearchPrefetchStatus::kInFlight, prefetch_status.value());
WaitUntilStatusChanges(base::ASCIIToUTF16(search_terms));
auto headers = search_server_requests()[0].headers;
EXPECT_EQ(1u, search_server_requests().size());
ASSERT_TRUE(base::Contains(headers, kThrottleHeader));
EXPECT_TRUE(base::Contains(headers[kThrottleHeader], kThrottleHeaderValue));
prefetch_status = search_prefetch_service->GetSearchPrefetchStatusForTesting(
base::ASCIIToUTF16(search_terms));
ASSERT_TRUE(prefetch_status.has_value());
EXPECT_EQ(SearchPrefetchStatus::kCanBeServed, prefetch_status.value());
content::SetBrowserClientForTesting(old_client);
}
class HeaderObserverContentBrowserClient : public ChromeContentBrowserClient {
public:
HeaderObserverContentBrowserClient() = default;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment