HTTPS Server Previews V2 Only: Use the page_id from the NavigationUIData
Pulls the page_id from the NavigationUIData for use in constructing the
headers, and in the pingback.
Note that this CL, like its parent, has to workaround the drp_page_id
not being set on every navigation. The examples that came up most often
in test on forward navigations and client redirect.
Bug: 952523
Change-Id: I75d5b42121c40f8388b7aea243a791fbb598e256
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1572649
Commit-Queue: Robert Ogden <robertogden@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652810}diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
index c37954b..bd2dcf4 100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -4965,6 +4965,9 @@
}
#endif
+ ChromeNavigationUIData* chrome_navigation_ui_data =
+ static_cast<ChromeNavigationUIData*>(navigation_ui_data);
+
// TODO(ryansturm): Once this is on the UI thread, stop passing
// |network_loader_factory| and have interceptors create one themselves.
// https://crbug.com/931786
@@ -4973,7 +4976,9 @@
previews::features::kHTTPSServerPreviewsUsingURLLoader)) {
interceptors.push_back(
std::make_unique<previews::PreviewsLitePageURLLoaderInterceptor>(
- network_loader_factory, frame_tree_node_id));
+ network_loader_factory,
+ chrome_navigation_ui_data->data_reduction_proxy_page_id(),
+ frame_tree_node_id));
}
return interceptors;
diff --git a/chrome/browser/previews/previews_content_util.cc b/chrome/browser/previews/previews_content_util.cc
index 1ceb736e..0e04b67 100644
--- a/chrome/browser/previews/previews_content_util.cc
+++ b/chrome/browser/previews/previews_content_util.cc
@@ -20,6 +20,7 @@
#include "chrome/browser/previews/previews_service.h"
#include "chrome/browser/previews/previews_service_factory.h"
#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/renderer_host/chrome_navigation_ui_data.h"
#include "components/content_settings/core/browser/cookie_settings.h"
#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h"
#include "components/previews/content/previews_user_data.h"
@@ -168,11 +169,11 @@
const net::HttpRequestHeaders& headers =
navigation_handle->GetRequestHeaders();
- base::Optional<uint64_t> page_id = data_reduction_proxy::
- DataReductionProxyRequestOptions::GetPageIdFromRequestHeaders(headers);
- if (page_id) {
- server_lite_page_info->page_id = page_id.value();
- }
+ const ChromeNavigationUIData* chrome_navigation_ui_data =
+ static_cast<const ChromeNavigationUIData*>(
+ navigation_handle->GetNavigationUIData());
+ server_lite_page_info->page_id =
+ chrome_navigation_ui_data->data_reduction_proxy_page_id();
base::Optional<std::string> session_key =
data_reduction_proxy::DataReductionProxyRequestOptions::
diff --git a/chrome/browser/previews/previews_lite_page_browsertest.cc b/chrome/browser/previews/previews_lite_page_browsertest.cc
index c34834d..b4a083c6 100644
--- a/chrome/browser/previews/previews_lite_page_browsertest.cc
+++ b/chrome/browser/previews/previews_lite_page_browsertest.cc
@@ -41,6 +41,7 @@
#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_test_utils.h"
#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h"
#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h"
#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h"
#include "components/data_reduction_proxy/core/common/data_reduction_proxy_features.h"
#include "components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.h"
@@ -549,6 +550,7 @@
}
const GURL& client_redirect_url() const { return client_redirect_url_; }
const GURL& subframe_url() const { return subframe_url_; }
+ uint64_t got_page_id() const { return got_page_id_; }
int subresources_requested() const { return subresources_requested_; }
void WaitForPingback() {
@@ -683,6 +685,12 @@
net::HttpStatusCode::HTTP_PROXY_AUTHENTICATION_REQUIRED);
return response;
}
+ net::HttpRequestHeaders headers;
+ headers.AddHeaderFromString("chrome-proxy: " +
+ request.headers.find("chrome-proxy")->second);
+ got_page_id_ = data_reduction_proxy::DataReductionProxyRequestOptions::
+ GetPageIdFromRequestHeaders(headers)
+ .value();
if (request.GetURL().spec().find("redirect_loop") != std::string::npos) {
response->set_code(net::HTTP_TEMPORARY_REDIRECT);
@@ -801,6 +809,7 @@
GURL subframe_url_;
GURL previews_server_url_;
GURL slow_http_url_;
+ uint64_t got_page_id_ = 0;
int subresources_requested_ = 0;
base::OnceClosure waiting_for_pingback_closure_;
};
@@ -1452,35 +1461,49 @@
->data_reduction_proxy_service()
->SetPingbackClientForTesting(pingback_client);
- ui_test_utils::NavigateToURL(browser(), HttpsLitePageURL(kSuccess));
- VerifyPreviewLoaded();
+ // This test should pass whether or not
+ // |kDataReductionProxyPopulatePreviewsPageIDToPingback| is enabled.
+ for (const bool drp_pageid_feature_enabled : {false, true}) {
+ base::test::ScopedFeatureList scoped_feature_list;
+ scoped_feature_list.InitWithFeatureState(
+ data_reduction_proxy::features::
+ kDataReductionProxyPopulatePreviewsPageIDToPingback,
+ drp_pageid_feature_enabled);
- PreviewsUITabHelper* ui_tab_helper =
- PreviewsUITabHelper::FromWebContents(GetWebContents());
- previews::PreviewsUserData* previews_data =
- ui_tab_helper->previews_user_data();
- // Grab the page id and session now because they may change after the reload.
- uint64_t expected_page_id = previews_data->server_lite_page_info()->page_id;
- std::string expected_session_key =
- previews_data->server_lite_page_info()->drp_session_key;
+ ui_test_utils::NavigateToURL(browser(), HttpsLitePageURL(kSuccess));
+ VerifyPreviewLoaded();
- // Starting a new page load will send a pingback for the previous page load.
- GetWebContents()->GetController().Reload(content::ReloadType::NORMAL, false);
- pingback_client->WaitForPingback();
+ PreviewsUITabHelper* ui_tab_helper =
+ PreviewsUITabHelper::FromWebContents(GetWebContents());
+ previews::PreviewsUserData* previews_data =
+ ui_tab_helper->previews_user_data();
+ // Grab the page id and session now because they may change after the
+ // reload.
+ uint64_t expected_page_id = previews_data->server_lite_page_info()->page_id;
+ std::string expected_session_key =
+ previews_data->server_lite_page_info()->drp_session_key;
- data_reduction_proxy::DataReductionProxyData* data = pingback_client->data();
- EXPECT_TRUE(data->used_data_reduction_proxy());
- EXPECT_TRUE(data->lite_page_received());
- EXPECT_FALSE(data->lofi_policy_received());
- EXPECT_FALSE(data->lofi_received());
- EXPECT_FALSE(data->was_cached_data_reduction_proxy_response());
+ // Starting a new page load will send a pingback for the previous page load.
+ GetWebContents()->GetController().Reload(content::ReloadType::NORMAL,
+ false);
+ pingback_client->WaitForPingback();
- // TODO(crbug.com/952523): Fix and remove this early return.
- if (GetParam())
- return;
+ data_reduction_proxy::DataReductionProxyData* data =
+ pingback_client->data();
+ EXPECT_TRUE(data->used_data_reduction_proxy());
+ EXPECT_TRUE(data->lite_page_received());
+ EXPECT_FALSE(data->lofi_policy_received());
+ EXPECT_FALSE(data->lofi_received());
+ EXPECT_FALSE(data->was_cached_data_reduction_proxy_response());
- EXPECT_EQ(data->session_key(), expected_session_key);
- EXPECT_EQ(data->page_id().value(), expected_page_id);
+ // TODO(crbug.com/952523): Fix and remove this early exit.
+ if (GetParam())
+ continue;
+
+ EXPECT_EQ(data->page_id().value(), expected_page_id);
+ EXPECT_EQ(data->page_id().value(), got_page_id());
+ EXPECT_EQ(data->session_key(), expected_session_key);
+ }
}
class PreviewsLitePageServerTimeoutBrowserTest
diff --git a/chrome/browser/previews/previews_lite_page_navigation_throttle.cc b/chrome/browser/previews/previews_lite_page_navigation_throttle.cc
index 0841b60d..bd51c9d 100644
--- a/chrome/browser/previews/previews_lite_page_navigation_throttle.cc
+++ b/chrome/browser/previews/previews_lite_page_navigation_throttle.cc
@@ -32,6 +32,7 @@
#include "chrome/browser/previews/previews_service_factory.h"
#include "chrome/browser/previews/previews_ui_tab_helper.h"
#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/renderer_host/chrome_navigation_ui_data.h"
#include "components/base32/base32.h"
#include "components/content_settings/core/browser/cookie_settings.h"
#include "components/content_settings/core/common/cookie_settings_base.h"
@@ -737,7 +738,16 @@
info->original_navigation_start = navigation_handle->NavigationStart();
if (session_id.has_value())
info->drp_session_key = session_id.value();
- info->page_id = manager->GeneratePageID();
+
+ const ChromeNavigationUIData* chrome_navigation_ui_data =
+ static_cast<const ChromeNavigationUIData*>(
+ navigation_handle->GetNavigationUIData());
+ info->page_id = chrome_navigation_ui_data->data_reduction_proxy_page_id();
+ // The page id may not be set in some corner cases (like forward navigation),
+ // so make sure it gets set here.
+ if (info->page_id == 0U)
+ info->page_id = manager->GeneratePageID();
+
return info;
}
diff --git a/chrome/browser/previews/previews_lite_page_url_loader_interceptor.cc b/chrome/browser/previews/previews_lite_page_url_loader_interceptor.cc
index f94b142..5d5126b 100644
--- a/chrome/browser/previews/previews_lite_page_url_loader_interceptor.cc
+++ b/chrome/browser/previews/previews_lite_page_url_loader_interceptor.cc
@@ -44,8 +44,8 @@
return true;
}
-net::HttpRequestHeaders GetChromeProxyHeaders(
- content::ResourceContext* context) {
+net::HttpRequestHeaders GetChromeProxyHeaders(content::ResourceContext* context,
+ uint64_t page_id) {
net::HttpRequestHeaders headers;
// Return empty headers for unittests.
if (!context)
@@ -58,8 +58,7 @@
DCHECK(data_reduction_proxy::params::IsEnabledWithNetworkService());
data_reduction_proxy::DataReductionProxyRequestOptions* request_options =
io_data->data_reduction_proxy_io_data()->request_options();
- request_options->AddRequestHeader(&headers,
- request_options->GeneratePageId());
+ request_options->AddRequestHeader(&headers, page_id != 0U ? page_id : 1);
headers.SetHeader(data_reduction_proxy::chrome_proxy_ect_header(),
net::GetNameForEffectiveConnectionType(
@@ -75,8 +74,10 @@
PreviewsLitePageURLLoaderInterceptor::PreviewsLitePageURLLoaderInterceptor(
const scoped_refptr<network::SharedURLLoaderFactory>&
network_loader_factory,
+ uint64_t page_id,
int frame_tree_node_id)
: network_loader_factory_(network_loader_factory),
+ page_id_(page_id),
frame_tree_node_id_(frame_tree_node_id) {}
PreviewsLitePageURLLoaderInterceptor::~PreviewsLitePageURLLoaderInterceptor() {}
@@ -136,8 +137,8 @@
// |redirect_url_loader_| can be null after this call.
redirect_url_loader_->StartRedirectToPreview(
- GetChromeProxyHeaders(resource_context), network_loader_factory_,
- frame_tree_node_id_);
+ GetChromeProxyHeaders(resource_context, page_id_),
+ network_loader_factory_, frame_tree_node_id_);
}
void PreviewsLitePageURLLoaderInterceptor::CreateOriginalURLLoader(
diff --git a/chrome/browser/previews/previews_lite_page_url_loader_interceptor.h b/chrome/browser/previews/previews_lite_page_url_loader_interceptor.h
index e44ad39..804a279 100644
--- a/chrome/browser/previews/previews_lite_page_url_loader_interceptor.h
+++ b/chrome/browser/previews/previews_lite_page_url_loader_interceptor.h
@@ -5,6 +5,7 @@
#ifndef CHROME_BROWSER_PREVIEWS_PREVIEWS_LITE_PAGE_URL_LOADER_INTERCEPTOR_H_
#define CHROME_BROWSER_PREVIEWS_PREVIEWS_LITE_PAGE_URL_LOADER_INTERCEPTOR_H_
+#include <stdint.h>
#include <memory>
#include <set>
@@ -28,6 +29,7 @@
PreviewsLitePageURLLoaderInterceptor(
const scoped_refptr<network::SharedURLLoaderFactory>&
network_loader_factory,
+ uint64_t page_id,
int frame_tree_node_id);
~PreviewsLitePageURLLoaderInterceptor() override;
@@ -75,6 +77,9 @@
// Factory to create a network service URLLoader.
scoped_refptr<network::SharedURLLoaderFactory> network_loader_factory_;
+ // Used in the chrome-proxy header if a preview is attempted.
+ uint64_t page_id_;
+
// Used to create the network service URLLoader.
int frame_tree_node_id_;
diff --git a/chrome/browser/previews/previews_lite_page_url_loader_interceptor_unittest.cc b/chrome/browser/previews/previews_lite_page_url_loader_interceptor_unittest.cc
index b52c434c..63810137c 100644
--- a/chrome/browser/previews/previews_lite_page_url_loader_interceptor_unittest.cc
+++ b/chrome/browser/previews/previews_lite_page_url_loader_interceptor_unittest.cc
@@ -41,7 +41,7 @@
void SetUp() override {
interceptor_ = std::make_unique<PreviewsLitePageURLLoaderInterceptor>(
- shared_factory_, 1);
+ shared_factory_, 1, 2);
}
void SetFakeResponse(const GURL& url,
@@ -62,7 +62,7 @@
void ResetTest() {
interceptor_ = std::make_unique<PreviewsLitePageURLLoaderInterceptor>(
- shared_factory_, 1);
+ shared_factory_, 1, 2);
callback_was_empty_ = base::nullopt;
}
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
index 06122ea..583e7a9 100644
--- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
+++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
@@ -300,6 +300,14 @@
&page_id)) {
return page_id;
}
+
+ // Also attempt parsing the page_id as a hex string.
+ if (base::HexStringToUInt64(
+ base::TrimWhitespaceASCII(kv_pair.second, base::TRIM_ALL)
+ .as_string(),
+ &page_id)) {
+ return page_id;
+ }
}
}
return base::nullopt;
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc
index 7fd0632..f92a1f8 100644
--- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc
+++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc
@@ -424,6 +424,7 @@
{"chrome-proxy", "something=something_else, pid=, key=value", 0, false},
{"chrome-proxy", "something=something_else, key=value", 0, false},
{"chrome-proxy", "pid=123", 123, true},
+ {"chrome-proxy", "pid=123abc", 1194684, true},
{"chrome-proxy", " pid = 123 ", 123, true},
{"some_other_header", "pid=123", 0, false},
};
