Skip to content

Commit

Permalink
Remove UserAgentOverrideExperiment and related code
Browse files Browse the repository at this point in the history
This experiment is now expired and was conducted to study the
performance effect of various checks on the User-Agent string
overrides.

This CL is a major cleanup, expect minor cleanup CLs as follow ups to
this.

Bug: 1336959
Change-Id: Id7d14c62bb421163abda83b099eb6e9bef0779b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4192069
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Jonathan Njeunje <njeunje@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@google.com>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: James Maclean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1099444}
  • Loading branch information
njeunje-g authored and Chromium LUCI CQ committed Jan 31, 2023
1 parent 618c973 commit 90d3038
Show file tree
Hide file tree
Showing 19 changed files with 11 additions and 404 deletions.
107 changes: 1 addition & 106 deletions chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@
#include "components/page_load_metrics/browser/page_load_tracker.h"
#include "components/prefs/pref_service.h"
#include "components/sessions/content/content_test_helper.h"
#include "components/sessions/core/serialized_navigation_entry.h"
#include "components/sessions/core/serialized_navigation_entry_test_helper.h"
#include "components/ukm/test_ukm_recorder.h"
#include "content/public/browser/back_forward_cache.h"
#include "content/public/browser/browser_thread.h"
Expand Down Expand Up @@ -102,7 +100,6 @@
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/chrome_debug_urls.h"
#include "third_party/blink/public/common/features.h"
#include "third_party/blink/public/common/user_agent/user_agent_metadata.h"
#include "third_party/blink/public/mojom/use_counter/metrics/css_property_id.mojom.h"
#include "third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom.h"
#include "ui/gfx/geometry/size.h"
Expand Down Expand Up @@ -187,8 +184,7 @@ class PageLoadMetricsBrowserTest : public InProcessBrowserTest {
PageLoadMetricsBrowserTest() {
scoped_feature_list_.InitWithFeatures(
{ukm::kUkmFeature, blink::features::kPortals,
blink::features::kPortalsCrossOrigin,
blink::features::kUserAgentOverrideExperiment},
blink::features::kPortalsCrossOrigin},
{});
}

Expand Down Expand Up @@ -2293,107 +2289,6 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, LoadingMetrics) {
waiter->Wait();
}

IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest,
UseCounterUserAgentOverride) {
ASSERT_TRUE(embedded_test_server()->Start());

content::WebContents* web_contents =
browser()->tab_strip_model()->GetActiveWebContents();
std::string original_ua = embedder_support::GetUserAgent();
std::string ua_no_substring = "foo";
std::string ua_prefix = "foo" + original_ua;
std::string ua_suffix = original_ua + "foo";

{
base::HistogramTester histogram;
web_contents->SetUserAgentOverride(
blink::UserAgentOverride::UserAgentOnly(ua_no_substring), false);
web_contents->GetController()
.GetLastCommittedEntry()
->SetIsOverridingUserAgent(true);
auto waiter = CreatePageLoadMetricsTestWaiter("waiter");
waiter->AddUseCounterFeatureExpectation({
blink::mojom::UseCounterFeatureType::kUserAgentOverride,
blink::UserAgentOverride::UserAgentOverriden,
});
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL("/empty.html")));
content::EvalJsResult result = EvalJs(web_contents, "navigator.userAgent;");
waiter->Wait();
NavigateToUntrackedUrl();
content::FetchHistogramsFromChildProcesses();
// Expect 2; one in the navigation stack and one in the renderer
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverriden, 2);
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverrideSubstring, 0);
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverrideSuffix, 0);
}

{
base::HistogramTester histogram;
web_contents->SetUserAgentOverride(
blink::UserAgentOverride::UserAgentOnly(ua_prefix), false);
web_contents->GetController()
.GetLastCommittedEntry()
->SetIsOverridingUserAgent(true);
auto waiter = CreatePageLoadMetricsTestWaiter("waiter");
waiter->AddUseCounterFeatureExpectation({
blink::mojom::UseCounterFeatureType::kUserAgentOverride,
blink::UserAgentOverride::UserAgentOverrideSubstring,
});
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL("/empty.html")));
content::EvalJsResult result = EvalJs(web_contents, "navigator.userAgent;");
waiter->Wait();
NavigateToUntrackedUrl();
content::FetchHistogramsFromChildProcesses();
// Expect 2; one in the navigation stack and one in the renderer
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverriden, 0);
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverrideSubstring, 2);
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverrideSuffix, 0);
}
{
base::HistogramTester histogram;
web_contents->SetUserAgentOverride(
blink::UserAgentOverride::UserAgentOnly(ua_suffix), false);
web_contents->GetController()
.GetLastCommittedEntry()
->SetIsOverridingUserAgent(true);
auto waiter = CreatePageLoadMetricsTestWaiter("waiter");
waiter->AddUseCounterFeatureExpectation({
blink::mojom::UseCounterFeatureType::kUserAgentOverride,
blink::UserAgentOverride::UserAgentOverrideSuffix,
});
ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL("/empty.html")));
content::EvalJsResult result = EvalJs(web_contents, "navigator.userAgent;");
waiter->Wait();
NavigateToUntrackedUrl();
content::FetchHistogramsFromChildProcesses();
// Expect 2; one in the navigation stack and one in the renderer
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverriden, 0);
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverrideSubstring, 0);
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverrideSuffix, 2);
}
}

class SessionRestorePageLoadMetricsBrowserTest
: public PageLoadMetricsBrowserTest {
public:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ using UkmFeatureList = UseCounterMetricsRecorder::UkmFeatureList;
using WebFeature = blink::mojom::WebFeature;
using CSSSampleId = blink::mojom::CSSSampleId;
using PermissionsPolicyFeature = blink::mojom::PermissionsPolicyFeature;
using UserAgentOverrideHistogram =
blink::UserAgentOverride::UserAgentOverrideHistogram;

#define FEATURE_HISTOGRAM_NAME(name, is_in_fenced_frames) \
is_in_fenced_frames ? "Blink.UseCounter.FencedFrames." name \
Expand Down Expand Up @@ -89,11 +87,6 @@ UseCounterMetricsRecorder::UseCounterMetricsRecorder(bool is_in_fenced_frame)
uma_permissions_policy_header2_(
AtMostOnceEnumUmaDeferrer<blink::mojom::PermissionsPolicyFeature>(
FEATURE_HISTOGRAM_NAME("PermissionsPolicy.Header2",
is_in_fenced_frame))),
uma_user_agent_override_(
AtMostOnceEnumUmaDeferrer<
blink::UserAgentOverride::UserAgentOverrideHistogram>(
FEATURE_HISTOGRAM_NAME("UserAgentOverride",
is_in_fenced_frame))) {}

UseCounterMetricsRecorder::~UseCounterMetricsRecorder() = default;
Expand Down Expand Up @@ -135,7 +128,6 @@ void UseCounterMetricsRecorder::DisableDeferAndFlush() {
uma_permissions_policy_violation_enforce_.DisableDeferAndFlush();
uma_permissions_policy_allow2_.DisableDeferAndFlush();
uma_permissions_policy_header2_.DisableDeferAndFlush();
uma_user_agent_override_.DisableDeferAndFlush();
}

void UseCounterMetricsRecorder::RecordOrDeferUseCounterFeature(
Expand Down Expand Up @@ -179,10 +171,6 @@ void UseCounterMetricsRecorder::RecordOrDeferUseCounterFeature(
uma_permissions_policy_allow2_.RecordOrDefer(
static_cast<PermissionsPolicyFeature>(feature.value()));
break;
case FeatureType::kUserAgentOverride:
uma_user_agent_override_.RecordOrDefer(
static_cast<UserAgentOverrideHistogram>(feature.value()));
break;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,6 @@ class UseCounterMetricsRecorder {

AtMostOnceEnumUmaDeferrer<blink::mojom::PermissionsPolicyFeature>
uma_permissions_policy_header2_;
AtMostOnceEnumUmaDeferrer<
blink::UserAgentOverride::UserAgentOverrideHistogram>
uma_user_agent_override_;

// To keep tracks of which features have been measured.
std::bitset<static_cast<size_t>(blink::mojom::WebFeature::kNumberOfFeatures)>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,6 @@ const char* GetUseCounterHistogramName(
return "Blink.UseCounter.FencedFrames.PermissionsPolicy.Header2";
case FeatureType::kPermissionsPolicyIframeAttribute:
return "Blink.UseCounter.FencedFrames.PermissionsPolicy.Allow2";
case FeatureType::kUserAgentOverride:
return "Blink.UseCounter.FencedFrames.UserAgentOverride";
}
} else {
if (is_in_main_frame) {
Expand All @@ -71,8 +69,6 @@ const char* GetUseCounterHistogramName(
return "Blink.UseCounter.PermissionsPolicy.Header2";
case FeatureType::kPermissionsPolicyIframeAttribute:
return "Blink.UseCounter.PermissionsPolicy.Allow2";
case FeatureType::kUserAgentOverride:
return "Blink.UseCounter.UserAgentOverride";
}
}
}
Expand Down
28 changes: 2 additions & 26 deletions content/browser/renderer_host/navigation_request.cc
Original file line number Diff line number Diff line change
Expand Up @@ -345,31 +345,6 @@ bool DoesHeaderContainClientHint(
return headers.GetHeader(header, &value) && value == "?1";
}

void LogUserAgentOverrideHistogram(const std::string& user_agent) {
std::string ua_original = GetContentClient()->browser()->GetUserAgent();
base::UmaHistogramEnumeration("Navigation.UserAgentStringType",
UserAgentStringType::kOverriden);

if (!base::FeatureList::IsEnabled(
blink::features::kUserAgentOverrideExperiment)) {
return;
}

auto it = user_agent.find(ua_original);
blink::UserAgentOverride::UserAgentOverrideHistogram histogram =
blink::UserAgentOverride::UserAgentOverrideHistogram::UserAgentOverriden;
if (it == 0) {
histogram = blink::UserAgentOverride::UserAgentOverrideHistogram::
UserAgentOverrideSuffix;
} else if (it != std::string::npos) {
histogram = blink::UserAgentOverride::UserAgentOverrideHistogram::
UserAgentOverrideSubstring;
}

base::UmaHistogramEnumeration(
blink::UserAgentOverride::kUserAgentOverrideHistogram, histogram);
}

// Computes the value that should be set for the User-Agent header, based on the
// values of relevant headers like Sec-CH-UA-Reduced or Sec-CH-UA-Full. If
// `user_agent_override` is non-empty, `user_agent_override` is returned as the
Expand All @@ -378,7 +353,8 @@ std::string ComputeUserAgentValue(const net::HttpRequestHeaders& headers,
const std::string& user_agent_override,
content::BrowserContext* context) {
if (!user_agent_override.empty()) {
LogUserAgentOverrideHistogram(user_agent_override);
base::UmaHistogramEnumeration("Navigation.UserAgentStringType",
UserAgentStringType::kOverriden);
return user_agent_override;
}

Expand Down
88 changes: 0 additions & 88 deletions content/browser/web_contents/web_contents_impl_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2549,94 +2549,6 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, UserAgentOverride) {
EvalJs(shell()->web_contents(), "document.body.textContent;"));
}

class WebContentsImplBrowserTestUserAgentOverrideSubstring
: public WebContentsImplBrowserTest {
public:
void SetUp() override {
scoped_feature_list_.Reset();
scoped_feature_list_.InitAndEnableFeature(
blink::features::kUserAgentOverrideExperiment);
WebContentsImplBrowserTest::SetUp();
}
};

// Verify UserAgentOverride histograms
IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTestUserAgentOverrideSubstring,
UserAgentOverrideHistogram) {
ASSERT_TRUE(embedded_test_server()->Start());
const std::string kHeaderPath =
std::string("/echoheader?") + net::HttpRequestHeaders::kUserAgent;
const GURL kUrl(embedded_test_server()->GetURL(kHeaderPath));
EXPECT_TRUE(NavigateToURL(shell(), kUrl));

std::string original_ua = ShellContentBrowserClient::Get()->GetUserAgent();
std::string ua_no_substring = "foo";
std::string ua_prefix = "foo" + original_ua;
std::string ua_suffix = original_ua + "foo";
{
base::HistogramTester histogram;
shell()->web_contents()->SetUserAgentOverride(
blink::UserAgentOverride::UserAgentOnly(ua_no_substring), false);
shell()
->web_contents()
->GetController()
.GetLastCommittedEntry()
->SetIsOverridingUserAgent(true);
EXPECT_TRUE(NavigateToURL(shell(), kUrl));
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverriden, 1);
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverrideSubstring, 0);
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverrideSuffix, 0);
}

{
base::HistogramTester histogram;
shell()->web_contents()->SetUserAgentOverride(
blink::UserAgentOverride::UserAgentOnly(ua_prefix), false);
shell()
->web_contents()
->GetController()
.GetLastCommittedEntry()
->SetIsOverridingUserAgent(true);
EXPECT_TRUE(NavigateToURL(shell(), kUrl));
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverriden, 0);
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverrideSubstring, 1);
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverrideSuffix, 0);
}

{
base::HistogramTester histogram;
shell()->web_contents()->SetUserAgentOverride(
blink::UserAgentOverride::UserAgentOnly(ua_suffix), false);
shell()
->web_contents()
->GetController()
.GetLastCommittedEntry()
->SetIsOverridingUserAgent(true);
EXPECT_TRUE(NavigateToURL(shell(), kUrl));
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverriden, 0);
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverrideSubstring, 0);
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverrideSuffix, 1);
}
}

// Verifies the user-agent string may be changed in DidStartNavigation().
IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
SetUserAgentOverrideFromDidStartNavigation) {
Expand Down
11 changes: 2 additions & 9 deletions extensions/browser/guest_view/web_view/web_view_apitest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -930,10 +930,8 @@ class WebViewAPITestUserAgentOverride
: public WebViewAPITest {
public:
void SetUp() override {
scoped_feature_list_.InitWithFeatures(
{blink::features::kUserAgentOverrideExperiment,
blink::features::kUACHOverrideBlank},
{});
scoped_feature_list_.InitWithFeatures({blink::features::kUACHOverrideBlank},
{});
WebViewAPITest::SetUp();
}

Expand All @@ -959,13 +957,8 @@ IN_PROC_BROWSER_TEST_F(WebViewAPITestUserAgentOverride, TestSetUserAgentOverride
https_server.SetSSLConfig(
net::test_server::EmbeddedTestServer::CERT_COMMON_NAME_IS_DOMAIN);
ASSERT_TRUE(https_server.Start());
base::HistogramTester histogram;
test_config_.SetByDottedPath(kTestServerPort, https_server.port());
RunTest("testSetUserAgentOverride", "web_view/apitest");
content::FetchHistogramsFromChildProcesses();
histogram.ExpectBucketCount(
blink::UserAgentOverride::kUserAgentOverrideHistogram,
blink::UserAgentOverride::UserAgentOverriden, 1);
}

} // namespace extensions
5 changes: 0 additions & 5 deletions third_party/blink/common/use_counter/use_counter_feature.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,6 @@ bool UseCounterFeature::IsValid() const {
return value_ < static_cast<UseCounterFeature::EnumValue>(
mojom::PermissionsPolicyFeature::kMaxValue) +
1;
case mojom::UseCounterFeatureType::kUserAgentOverride:
return value_ < static_cast<UseCounterFeature::EnumValue>(
blink::UserAgentOverride::UserAgentOverrideHistogram::
kMaxValue) +
1;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ bool IsReservedFeature(const blink::UseCounterFeature& feature) {
return feature.value() ==
static_cast<blink::UseCounterFeature::EnumValue>(
blink::mojom::PermissionsPolicyFeature::kNotFound);
case blink::mojom::UseCounterFeatureType::kUserAgentOverride:
return false;
}
}
} // namespace
Expand Down

0 comments on commit 90d3038

Please sign in to comment.