From 78c87006a6fec3cf7c2123d845ba65162bd1b25c Mon Sep 17 00:00:00 2001 From: Pranjal Jumde Date: Tue, 21 May 2019 00:04:17 -0700 Subject: [PATCH 1/5] Issue 4552: Restore features to correct state after disabling field trials auditors: @bridiver, @bbondy, @diracdeltas, @bsclifton, @simonhong --- app/brave_main_delegate.cc | 7 +- app/brave_main_delegate_browsertest.cc | 15 +++ .../location_bar_view_browsertest.cc | 117 ++++++++++++++++++ .../browser/location_bar_model_impl.cc | 22 ++++ .../service/field_trial_unittest.cc | 20 +++ ...mponents-variations-service-BUILD.gn.patch | 13 ++ test/BUILD.gn | 4 +- 7 files changed, 192 insertions(+), 6 deletions(-) create mode 100644 chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc create mode 100644 chromium_src/components/omnibox/browser/location_bar_model_impl.cc create mode 100644 chromium_src/components/variations/service/field_trial_unittest.cc create mode 100644 patches/components-variations-service-BUILD.gn.patch diff --git a/app/brave_main_delegate.cc b/app/brave_main_delegate.cc index 2ad419acc9c0..e6637135851e 100644 --- a/app/brave_main_delegate.cc +++ b/app/brave_main_delegate.cc @@ -26,11 +26,10 @@ #include "chrome/common/chrome_switches.h" #include "components/autofill/core/common/autofill_features.h" #include "components/autofill/core/common/autofill_payments_features.h" +#include "components/omnibox/common/omnibox_features.h" #include "components/password_manager/core/common/password_manager_features.h" #include "components/unified_consent/feature.h" -#include "content/public/common/content_features.h" #include "extensions/common/extension_features.h" -#include "gpu/config/gpu_finch_features.h" #include "services/network/public/cpp/features.h" #include "third_party/widevine/cdm/buildflags.h" #include "ui/base/ui_base_features.h" @@ -139,17 +138,15 @@ bool BraveMainDelegate::BasicStartupComplete(int* exit_code) { extensions_features::kNewExtensionUpdaterService.name, #endif features::kDesktopPWAWindowing.name, + omnibox::kSimplifyHttpsIndicator.name, }; // Disabled features. const std::unordered_set disabled_features = { autofill::features::kAutofillSaveCardSignInAfterLocalSave.name, autofill::features::kAutofillServerCommunication.name, - features::kAudioServiceOutOfProcess.name, - features::kDefaultEnableOopRasterization.name, network::features::kNetworkService.name, unified_consent::kUnifiedConsent.name, - features::kLookalikeUrlNavigationSuggestionsUI.name, }; command_line.AppendFeatures(enabled_features, disabled_features); diff --git a/app/brave_main_delegate_browsertest.cc b/app/brave_main_delegate_browsertest.cc index c0e587c355f0..f97872f1f643 100644 --- a/app/brave_main_delegate_browsertest.cc +++ b/app/brave_main_delegate_browsertest.cc @@ -11,10 +11,12 @@ #include "chrome/browser/domain_reliability/service_factory.h" #include "components/autofill/core/common/autofill_features.h" #include "components/autofill/core/common/autofill_payments_features.h" +#include "components/omnibox/common/omnibox_features.h" #include "components/unified_consent/feature.h" #include "content/public/browser/render_view_host.h" #include "content/public/common/content_features.h" #include "content/public/common/web_preferences.h" +#include "extensions/common/extension_features.h" #include "gpu/config/gpu_finch_features.h" #include "services/network/public/cpp/features.h" @@ -52,3 +54,16 @@ IN_PROC_BROWSER_TEST_F(BraveMainDelegateBrowserTest, DisabledFeatures) { for (const auto* feature : disabled_features) EXPECT_FALSE(base::FeatureList::IsEnabled(*feature)); } + +IN_PROC_BROWSER_TEST_F(BraveMainDelegateBrowserTest, EnabledFeatures) { + const base::Feature* enabled_features[] = { +#if BUILDFLAG(ENABLE_EXTENSIONS) + &extensions_features::kNewExtensionUpdaterService, +#endif + &features::kDesktopPWAWindowing, + &omnibox::kSimplifyHttpsIndicator, + }; + + for (const auto* feature : enabled_features) + EXPECT_TRUE(base::FeatureList::IsEnabled(*feature)); +} diff --git a/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc b/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc new file mode 100644 index 000000000000..a63a51b6abe5 --- /dev/null +++ b/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc @@ -0,0 +1,117 @@ +/* Copyright (c) 2019 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "chrome/browser/ui/views/location_bar/location_bar_view.h" + +#include "chrome/browser/ssl/security_state_tab_helper.h" +#include "chrome/browser/ui/views/frame/browser_view.h" +#include "chrome/test/base/in_process_browser_test.h" +#include "chrome/test/base/ui_test_utils.h" +#include "components/omnibox/browser/location_bar_model_impl.h" +#include "components/omnibox/common/omnibox_features.h" +#include "content/public/test/url_loader_interceptor.h" +#include "net/cert/ct_policy_status.h" +#include "net/ssl/ssl_info.h" +#include "net/test/cert_test_util.h" +#include "net/test/test_data_directory.h" + +const char kMockSecureHostname[] = "example-secure.test"; +const GURL kMockSecureURL = GURL("https://example-secure.test"); + +class SecurityIndicatorTest : public InProcessBrowserTest { + public: + SecurityIndicatorTest() : InProcessBrowserTest(), cert_(nullptr) {} + + void SetUpInProcessBrowserTestFixture() override { + cert_ = + net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); + ASSERT_TRUE(cert_); + ASSERT_TRUE(embedded_test_server()->Start()); + } + + LocationBarView* GetLocationBarView() { + BrowserView* browser_view = + BrowserView::GetBrowserViewForBrowser(browser()); + return browser_view->GetLocationBarView(); + } + + void SetUpInterceptor(net::CertStatus cert_status) { + url_loader_interceptor_ = std::make_unique( + base::BindRepeating(&SecurityIndicatorTest::InterceptURLLoad, + base::Unretained(this), cert_status)); + } + + void ResetInterceptor() { url_loader_interceptor_.reset(); } + + bool InterceptURLLoad(net::CertStatus cert_status, + content::URLLoaderInterceptor::RequestParams* params) { + if (params->url_request.url.host() != kMockSecureHostname) + return false; + net::SSLInfo ssl_info; + ssl_info.cert = cert_; + ssl_info.cert_status = cert_status; + ssl_info.ct_policy_compliance = + net::ct::CTPolicyCompliance::CT_POLICY_COMPLIES_VIA_SCTS; + network::ResourceResponseHead resource_response; + resource_response.mime_type = "text/html"; + resource_response.ssl_info = ssl_info; + params->client->OnReceiveResponse(resource_response); + // Send an empty response's body. This pipe is not filled with data. + mojo::DataPipe pipe; + params->client->OnStartLoadingResponseBody(std::move(pipe.consumer_handle)); + network::URLLoaderCompletionStatus completion_status; + completion_status.ssl_info = ssl_info; + params->client->OnComplete(completion_status); + return true; + } + + private: + scoped_refptr cert_; + std::unique_ptr url_loader_interceptor_; + + DISALLOW_COPY_AND_ASSIGN(SecurityIndicatorTest); +}; + +IN_PROC_BROWSER_TEST_F(SecurityIndicatorTest, CheckIndicatorText) { + const GURL kMockNonsecureURL = + embedded_test_server()->GetURL("example.test", "/"); + const base::string16 kEmptyString = base::string16(); + + const struct { + GURL url; + net::CertStatus cert_status; + security_state::SecurityLevel security_level; + bool should_show_text; + base::string16 indicator_text; + } cases[]{// Default + {kMockSecureURL, net::CERT_STATUS_IS_EV, security_state::EV_SECURE, + false, kEmptyString}, + {kMockSecureURL, 0, security_state::SECURE, false, kEmptyString}, + {kMockNonsecureURL, 0, security_state::NONE, false, kEmptyString}}; + + content::WebContents* tab = + browser()->tab_strip_model()->GetActiveWebContents(); + ASSERT_TRUE(tab); + + // After SetUpInterceptor() is called, requests to this hostname will be + // mocked and use specified certificate validation results. This allows tests + // to mock Extended Validation (EV) certificate connections. + SecurityStateTabHelper* helper = SecurityStateTabHelper::FromWebContents(tab); + ASSERT_TRUE(helper); + LocationBarView* location_bar_view = GetLocationBarView(); + + for (const auto& c : cases) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature(omnibox::kSimplifyHttpsIndicator); + SetUpInterceptor(c.cert_status); + ui_test_utils::NavigateToURL(browser(), c.url); + EXPECT_EQ(c.security_level, helper->GetSecurityLevel()); + EXPECT_EQ(c.should_show_text, + location_bar_view->location_icon_view()->ShouldShowLabel()); + EXPECT_EQ(c.indicator_text, + location_bar_view->location_icon_view()->GetText()); + ResetInterceptor(); + } +} diff --git a/chromium_src/components/omnibox/browser/location_bar_model_impl.cc b/chromium_src/components/omnibox/browser/location_bar_model_impl.cc new file mode 100644 index 000000000000..88a869a0a7ce --- /dev/null +++ b/chromium_src/components/omnibox/browser/location_bar_model_impl.cc @@ -0,0 +1,22 @@ +/* Copyright (c) 2019 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "components/omnibox/browser/location_bar_model_impl.h" + +#include "components/omnibox/browser/omnibox_field_trial.h" + +#define GetFieldTrialParamValueByFeature \ + GetFieldTrialParamValueByFeature_ChromiumImpl + +namespace base { +std::string GetFieldTrialParamValueByFeature(const base::Feature& feature, + const std::string& param_name) { + return OmniboxFieldTrial::kSimplifyHttpsIndicatorParameterBothToLock; +} +} // namespace base + +#include "../../../../../../../components/omnibox/browser/location_bar_model_impl.cc" // NOLINT + +#undef GetFieldTrialParamValueByFeature diff --git a/chromium_src/components/variations/service/field_trial_unittest.cc b/chromium_src/components/variations/service/field_trial_unittest.cc new file mode 100644 index 000000000000..2e676f2f3b93 --- /dev/null +++ b/chromium_src/components/variations/service/field_trial_unittest.cc @@ -0,0 +1,20 @@ +/* Copyright (c) 2019 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "components/variations/service/buildflags.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace { +typedef testing::Test FieldTrialsTest; + +TEST_F(FieldTrialsTest, FieldTrialsTestingEnabled) { + bool enabled = false; +#if BUILDFLAG(FIELDTRIAL_TESTING_ENABLED) + enabled = true +#endif // BUILDFLAG(FIELDTRIAL_TESTING_ENABLED) + EXPECT_FALSE(enabled); +} + +} // namespace diff --git a/patches/components-variations-service-BUILD.gn.patch b/patches/components-variations-service-BUILD.gn.patch new file mode 100644 index 000000000000..7389260dc62e --- /dev/null +++ b/patches/components-variations-service-BUILD.gn.patch @@ -0,0 +1,13 @@ +diff --git a/components/variations/service/BUILD.gn b/components/variations/service/BUILD.gn +index bdabd37ad8d0f98aad110d68f8058885d3f3c31a..98864341b6223ca591bcd19dc9097d1b6fb829b0 100644 +--- a/components/variations/service/BUILD.gn ++++ b/components/variations/service/BUILD.gn +@@ -11,7 +11,7 @@ declare_args() { + # Note: this setting is ignored if is_chrome_branded. + # TODO(thakis): It's strange this is called "_like_official_build" but then + # checks is_chrome_branded, not is_official_build. +- fieldtrial_testing_like_official_build = is_chrome_branded ++ fieldtrial_testing_like_official_build = true + } + + fieldtrial_testing_enabled = diff --git a/test/BUILD.gn b/test/BUILD.gn index 296d56da2931..74c1b6043fed 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -74,8 +74,9 @@ test("brave_unit_tests") { "//brave/chromium_src/components/metrics/enabled_state_provider_unittest.cc", "//brave/chromium_src/components/search_engines/brave_template_url_prepopulate_data_unittest.cc", "//brave/chromium_src/components/search_engines/brave_template_url_service_util_unittest.cc", - "//brave/chromium_src/components/translate/core/browser/translate_manager_unittest.cc", + "//brave/chromium_src/components/variations/service/field_trial_unittest.cc", "//brave/chromium_src/components/version_info/brave_version_info_unittest.cc", + "//brave/chromium_src/components/translate/core/browser/translate_manager_unittest.cc", "//brave/chromium_src/net/cookies/brave_canonical_cookie_unittest.cc", "//brave/common/brave_content_client_unittest.cc", "//brave/common/importer/brave_mock_importer_bridge.cc", @@ -303,6 +304,7 @@ test("brave_browser_tests") { "//brave/browser/autocomplete/brave_autocomplete_provider_client_browsertest.cc", "//brave/browser/brave_scheme_load_browsertest.cc", "//brave/chromium_src/chrome/browser/google/chrome_google_url_tracker_client_browsertest.cc", + "//brave/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc", "//brave/chromium_src/components/content_settings/core/browser/brave_content_settings_registry_browsertest.cc", "//brave/chromium_src/third_party/blink/renderer/modules/battery/navigator_batterytest.cc", "//brave/chromium_src/third_party/blink/renderer/modules/bluetooth/navigator_bluetoothtest.cc", From d29d4e38c7418213c0be3292446ccebec5520f9b Mon Sep 17 00:00:00 2001 From: Pranjal Jumde Date: Wed, 22 May 2019 15:04:00 -0700 Subject: [PATCH 2/5] fix lint --- app/brave_main_delegate_browsertest.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/app/brave_main_delegate_browsertest.cc b/app/brave_main_delegate_browsertest.cc index f97872f1f643..de49c9189144 100644 --- a/app/brave_main_delegate_browsertest.cc +++ b/app/brave_main_delegate_browsertest.cc @@ -3,12 +3,12 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at http://mozilla.org/MPL/2.0/. */ +#include "chrome/browser/domain_reliability/service_factory.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/tabs/tab_strip_model.h" -#include "chrome/test/base/in_process_browser_test.h" #include "chrome/common/chrome_features.h" #include "chrome/common/chrome_switches.h" -#include "chrome/browser/domain_reliability/service_factory.h" +#include "chrome/test/base/in_process_browser_test.h" #include "components/autofill/core/common/autofill_features.h" #include "components/autofill/core/common/autofill_payments_features.h" #include "components/omnibox/common/omnibox_features.h" @@ -27,12 +27,12 @@ IN_PROC_BROWSER_TEST_F(BraveMainDelegateBrowserTest, EXPECT_TRUE(base::CommandLine::ForCurrentProcess()->HasSwitch( switches::kDisableDomainReliability)); EXPECT_FALSE(domain_reliability::DomainReliabilityServiceFactory:: - ShouldCreateService()); + ShouldCreateService()); } IN_PROC_BROWSER_TEST_F(BraveMainDelegateBrowserTest, DisableHyperlinkAuditing) { - EXPECT_TRUE(base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kNoPings)); + EXPECT_TRUE( + base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoPings)); content::WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents(); const content::WebPreferences prefs = @@ -56,12 +56,12 @@ IN_PROC_BROWSER_TEST_F(BraveMainDelegateBrowserTest, DisabledFeatures) { } IN_PROC_BROWSER_TEST_F(BraveMainDelegateBrowserTest, EnabledFeatures) { - const base::Feature* enabled_features[] = { + const base::Feature* enabled_features[] = { #if BUILDFLAG(ENABLE_EXTENSIONS) - &extensions_features::kNewExtensionUpdaterService, + &extensions_features::kNewExtensionUpdaterService, #endif - &features::kDesktopPWAWindowing, - &omnibox::kSimplifyHttpsIndicator, + &features::kDesktopPWAWindowing, + &omnibox::kSimplifyHttpsIndicator, }; for (const auto* feature : enabled_features) From afafa0b0229fe374b67f126db0076a4118339eb7 Mon Sep 17 00:00:00 2001 From: Pranjal Jumde Date: Thu, 23 May 2019 08:29:37 -0700 Subject: [PATCH 3/5] 1. Using base::EmptyString16 instead of std::string 2. Using config.js instead of patch to disable field trials --- .../location_bar/location_bar_view_browsertest.cc | 3 ++- .../components-variations-service-BUILD.gn.patch | 13 ------------- 2 files changed, 2 insertions(+), 14 deletions(-) delete mode 100644 patches/components-variations-service-BUILD.gn.patch diff --git a/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc b/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc index a63a51b6abe5..90470fdd6992 100644 --- a/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc +++ b/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc @@ -5,6 +5,7 @@ #include "chrome/browser/ui/views/location_bar/location_bar_view.h" +#include "base/strings/string_util.h" #include "chrome/browser/ssl/security_state_tab_helper.h" #include "chrome/browser/ui/views/frame/browser_view.h" #include "chrome/test/base/in_process_browser_test.h" @@ -77,7 +78,7 @@ class SecurityIndicatorTest : public InProcessBrowserTest { IN_PROC_BROWSER_TEST_F(SecurityIndicatorTest, CheckIndicatorText) { const GURL kMockNonsecureURL = embedded_test_server()->GetURL("example.test", "/"); - const base::string16 kEmptyString = base::string16(); + const base::string16 kEmptyString = base::EmptyString16(); const struct { GURL url; diff --git a/patches/components-variations-service-BUILD.gn.patch b/patches/components-variations-service-BUILD.gn.patch deleted file mode 100644 index 7389260dc62e..000000000000 --- a/patches/components-variations-service-BUILD.gn.patch +++ /dev/null @@ -1,13 +0,0 @@ -diff --git a/components/variations/service/BUILD.gn b/components/variations/service/BUILD.gn -index bdabd37ad8d0f98aad110d68f8058885d3f3c31a..98864341b6223ca591bcd19dc9097d1b6fb829b0 100644 ---- a/components/variations/service/BUILD.gn -+++ b/components/variations/service/BUILD.gn -@@ -11,7 +11,7 @@ declare_args() { - # Note: this setting is ignored if is_chrome_branded. - # TODO(thakis): It's strange this is called "_like_official_build" but then - # checks is_chrome_branded, not is_official_build. -- fieldtrial_testing_like_official_build = is_chrome_branded -+ fieldtrial_testing_like_official_build = true - } - - fieldtrial_testing_enabled = From b6e88ef9d034dfa124830d6c2f9a261dd77acce3 Mon Sep 17 00:00:00 2001 From: Pranjal Jumde Date: Thu, 23 May 2019 12:43:36 -0700 Subject: [PATCH 4/5] Setting kExtensionsInstallVerification to enforce-strict for BravePDFDownloadTest.VerifyChromiumPDFExntension --- app/brave_command_line_helper.cc | 7 +++++++ app/brave_command_line_helper.h | 1 + app/brave_main_delegate.cc | 2 ++ 3 files changed, 10 insertions(+) diff --git a/app/brave_command_line_helper.cc b/app/brave_command_line_helper.cc index e5ae3c78d40c..63500065e89d 100644 --- a/app/brave_command_line_helper.cc +++ b/app/brave_command_line_helper.cc @@ -93,3 +93,10 @@ void BraveCommandLineHelper::AppendCSV( } command_line_.AppendSwitchASCII(switch_key, ss.str()); } + +void BraveCommandLineHelper::AppendSwitchASCII( + const char* switch_key, + const char* value) { + if (!command_line_.HasSwitch(switch_key)) + command_line_.AppendSwitchASCII(switch_key, value); +} diff --git a/app/brave_command_line_helper.h b/app/brave_command_line_helper.h index 17409bfc1b92..7c2dd7eb736b 100644 --- a/app/brave_command_line_helper.h +++ b/app/brave_command_line_helper.h @@ -21,6 +21,7 @@ class BraveCommandLineHelper { inline ~BraveCommandLineHelper() = default; void AppendSwitch(const char* switch_key); + void AppendSwitchASCII(const char* switch_key, const char* value); void AppendFeatures(const std::unordered_set& enabled, const std::unordered_set& disabled); diff --git a/app/brave_main_delegate.cc b/app/brave_main_delegate.cc index e6637135851e..a65fd6aaf779 100644 --- a/app/brave_main_delegate.cc +++ b/app/brave_main_delegate.cc @@ -131,6 +131,8 @@ bool BraveMainDelegate::BasicStartupComplete(int* exit_code) { command_line.AppendSwitch(switches::kDisableDomainReliability); command_line.AppendSwitch(switches::kDisableChromeGoogleURLTrackingClient); command_line.AppendSwitch(switches::kNoPings); + command_line.AppendSwitchASCII(switches::kExtensionsInstallVerification, + switches::kExtensionContentVerificationEnforceStrict); // Enabled features. const std::unordered_set enabled_features = { From 58a37c56ac76c639304389c8d3e4db4a7ee4fa3b Mon Sep 17 00:00:00 2001 From: Pranjal Jumde Date: Fri, 24 May 2019 13:25:22 -0700 Subject: [PATCH 5/5] lint --- app/brave_command_line_helper.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/brave_command_line_helper.cc b/app/brave_command_line_helper.cc index 63500065e89d..beed38227a15 100644 --- a/app/brave_command_line_helper.cc +++ b/app/brave_command_line_helper.cc @@ -94,9 +94,8 @@ void BraveCommandLineHelper::AppendCSV( command_line_.AppendSwitchASCII(switch_key, ss.str()); } -void BraveCommandLineHelper::AppendSwitchASCII( - const char* switch_key, - const char* value) { - if (!command_line_.HasSwitch(switch_key)) +void BraveCommandLineHelper::AppendSwitchASCII(const char* switch_key, + const char* value) { + if (!command_line_.HasSwitch(switch_key)) command_line_.AppendSwitchASCII(switch_key, value); }