diff --git a/chrome/browser/chrome_navigation_browsertest.cc b/chrome/browser/chrome_navigation_browsertest.cc index 9add49ecb8d1..ccecebd58241 100644 --- a/chrome/browser/chrome_navigation_browsertest.cc +++ b/chrome/browser/chrome_navigation_browsertest.cc @@ -14,6 +14,8 @@ #include "chrome/test/base/ui_test_utils.h" #include "components/network_session_configurator/common/network_switches.h" #include "components/url_formatter/url_formatter.h" +#include "components/variations/active_field_trials.h" +#include "components/variations/metrics_util.h" #include "content/public/browser/navigation_entry.h" #include "content/public/browser/navigation_handle.h" #include "content/public/browser/notification_service.h" @@ -447,8 +449,12 @@ class SignInIsolationBrowserTest : public ChromeNavigationBrowserTest { : https_server_(net::EmbeddedTestServer::TYPE_HTTPS) {} ~SignInIsolationBrowserTest() override {} - void SetUp() override { + virtual void InitFeatureList() { feature_list_.InitAndEnableFeature(features::kSignInProcessIsolation); + } + + void SetUp() override { + InitFeatureList(); https_server_.ServeFilesFromSourceDirectory("chrome/test/data"); ASSERT_TRUE(https_server_.InitializeAndListen()); ChromeNavigationBrowserTest::SetUp(); @@ -477,8 +483,41 @@ class SignInIsolationBrowserTest : public ChromeNavigationBrowserTest { net::EmbeddedTestServer* https_server() { return &https_server_; } - private: + bool HasSyntheticTrial(const std::string& trial_name) { + std::vector synthetic_trials; + variations::GetSyntheticTrialGroupIdsAsString(&synthetic_trials); + std::string trial_hash = + base::StringPrintf("%x", metrics::HashName(trial_name)); + + for (auto entry : synthetic_trials) { + if (base::StartsWith(entry, trial_hash, base::CompareCase::SENSITIVE)) + return true; + } + + return false; + } + + bool IsInSyntheticTrialGroup(const std::string& trial_name, + const std::string& trial_group) { + std::vector synthetic_trials; + variations::GetSyntheticTrialGroupIdsAsString(&synthetic_trials); + std::string expected_entry = base::StringPrintf( + "%x-%x", metrics::HashName(trial_name), metrics::HashName(trial_group)); + + for (auto entry : synthetic_trials) { + if (entry == expected_entry) + return true; + } + + return false; + } + + const std::string kSyntheticTrialName = "SignInProcessIsolationActive"; + + protected: base::test::ScopedFeatureList feature_list_; + + private: net::EmbeddedTestServer https_server_; DISALLOW_COPY_AND_ASSIGN(SignInIsolationBrowserTest); @@ -507,3 +546,136 @@ IN_PROC_BROWSER_TEST_F(SignInIsolationBrowserTest, NavigateToSignInPage) { manager.WaitForNavigationFinished(); EXPECT_NE(web_contents->GetMainFrame()->GetSiteInstance(), first_instance); } + +// The next four tests verify that the synthetic field trial is set correctly +// for sign-in process isolation. The synthetic field trial should be created +// when browsing to the sign-in URL for the first time, and it should reflect +// whether or not the sign-in isolation base::Feature is enabled, and whether +// or not it is force-enabled from the command line. +IN_PROC_BROWSER_TEST_F(SignInIsolationBrowserTest, SyntheticTrial) { + EXPECT_FALSE(HasSyntheticTrial(kSyntheticTrialName)); + + ui_test_utils::NavigateToURL( + browser(), https_server()->GetURL("foo.com", "/title1.html")); + EXPECT_FALSE(HasSyntheticTrial(kSyntheticTrialName)); + + GURL signin_url( + https_server()->GetURL("accounts.google.com", "/title1.html")); + + // This test class uses InitAndEnableFeature, which overrides the feature + // settings as if it came from the command line, so by default, browsing to + // the sign-in URL should create the synthetic trial with ForceEnabled. + ui_test_utils::NavigateToURL(browser(), signin_url); + EXPECT_TRUE(IsInSyntheticTrialGroup(kSyntheticTrialName, "ForceEnabled")); +} + +// This test class is used to check the synthetic sign-in trial for the Enabled +// group. It creates a new field trial (with 100% probability of being in the +// group), and initializes the test class's ScopedFeatureList using it, being +// careful to not override it using the command line (which corresponds to +// ForceEnabled). +class EnabledSignInIsolationBrowserTest : public SignInIsolationBrowserTest { + public: + EnabledSignInIsolationBrowserTest() {} + ~EnabledSignInIsolationBrowserTest() override {} + + void InitFeatureList() override {} + + void SetUpOnMainThread() override { + const std::string kTrialName = "SignInProcessIsolation"; + const std::string kGroupName = "FooGroup"; // unused + scoped_refptr trial = + base::FieldTrialList::CreateFieldTrial(kTrialName, kGroupName); + + std::unique_ptr feature_list(new base::FeatureList); + feature_list->RegisterFieldTrialOverride( + features::kSignInProcessIsolation.name, + base::FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE, trial.get()); + + feature_list_.InitWithFeatureList(std::move(feature_list)); + SignInIsolationBrowserTest::SetUpOnMainThread(); + } + + DISALLOW_COPY_AND_ASSIGN(EnabledSignInIsolationBrowserTest); +}; + +IN_PROC_BROWSER_TEST_F(EnabledSignInIsolationBrowserTest, SyntheticTrial) { + EXPECT_FALSE(HasSyntheticTrial(kSyntheticTrialName)); + EXPECT_FALSE(IsInSyntheticTrialGroup(kSyntheticTrialName, "Enabled")); + + GURL signin_url = + https_server()->GetURL("accounts.google.com", "/title1.html"); + ui_test_utils::NavigateToURL(browser(), signin_url); + EXPECT_TRUE(IsInSyntheticTrialGroup(kSyntheticTrialName, "Enabled")); + + // A repeat navigation shouldn't change the synthetic trial. + ui_test_utils::NavigateToURL( + browser(), https_server()->GetURL("accounts.google.com", "/title2.html")); + EXPECT_TRUE(IsInSyntheticTrialGroup(kSyntheticTrialName, "Enabled")); +} + +// This test class is similar to EnabledSignInIsolationBrowserTest, but for the +// Disabled group of the synthetic sign-in trial. +class DisabledSignInIsolationBrowserTest : public SignInIsolationBrowserTest { + public: + DisabledSignInIsolationBrowserTest() {} + ~DisabledSignInIsolationBrowserTest() override {} + + void InitFeatureList() override {} + + void SetUpOnMainThread() override { + const std::string kTrialName = "SignInProcessIsolation"; + const std::string kGroupName = "FooGroup"; // unused + scoped_refptr trial = + base::FieldTrialList::CreateFieldTrial(kTrialName, kGroupName); + + std::unique_ptr feature_list(new base::FeatureList); + feature_list->RegisterFieldTrialOverride( + features::kSignInProcessIsolation.name, + base::FeatureList::OverrideState::OVERRIDE_DISABLE_FEATURE, + trial.get()); + + feature_list_.InitWithFeatureList(std::move(feature_list)); + SignInIsolationBrowserTest::SetUpOnMainThread(); + } + + DISALLOW_COPY_AND_ASSIGN(DisabledSignInIsolationBrowserTest); +}; + +IN_PROC_BROWSER_TEST_F(DisabledSignInIsolationBrowserTest, SyntheticTrial) { + EXPECT_FALSE(IsInSyntheticTrialGroup(kSyntheticTrialName, "Disabled")); + GURL signin_url = + https_server()->GetURL("accounts.google.com", "/title1.html"); + ui_test_utils::NavigateToURL(browser(), signin_url); + EXPECT_TRUE(IsInSyntheticTrialGroup(kSyntheticTrialName, "Disabled")); +} + +// This test class is similar to EnabledSignInIsolationBrowserTest, but for the +// ForceDisabled group of the synthetic sign-in trial. +class ForceDisabledSignInIsolationBrowserTest + : public SignInIsolationBrowserTest { + public: + ForceDisabledSignInIsolationBrowserTest() {} + ~ForceDisabledSignInIsolationBrowserTest() override {} + + void InitFeatureList() override { + feature_list_.InitAndDisableFeature(features::kSignInProcessIsolation); + } + + DISALLOW_COPY_AND_ASSIGN(ForceDisabledSignInIsolationBrowserTest); +}; + +IN_PROC_BROWSER_TEST_F(ForceDisabledSignInIsolationBrowserTest, + SyntheticTrial) { + // Test subframe navigation in this case, since that should also trigger + // synthetic trial creation. + ui_test_utils::NavigateToURL(browser(), + https_server()->GetURL("a.com", "/iframe.html")); + content::WebContents* web_contents = + browser()->tab_strip_model()->GetActiveWebContents(); + EXPECT_FALSE(IsInSyntheticTrialGroup(kSyntheticTrialName, "ForceDisabled")); + GURL signin_url = + https_server()->GetURL("accounts.google.com", "/title1.html"); + EXPECT_TRUE(NavigateIframeToURL(web_contents, "test", signin_url)); + EXPECT_TRUE(IsInSyntheticTrialGroup(kSyntheticTrialName, "ForceDisabled")); +} diff --git a/chrome/browser/metrics/chrome_metrics_service_accessor.h b/chrome/browser/metrics/chrome_metrics_service_accessor.h index 74a0b9870480..4e45c279ecd8 100644 --- a/chrome/browser/metrics/chrome_metrics_service_accessor.h +++ b/chrome/browser/metrics/chrome_metrics_service_accessor.h @@ -15,9 +15,10 @@ #include "components/metrics/metrics_service_accessor.h" class BrowserProcessImpl; -class Profile; class ChromeMetricsServiceClient; class ChromePasswordManagerClient; +class NavigationMetricsRecorder; +class Profile; namespace { class CrashesDOMHandler; @@ -126,6 +127,7 @@ class ChromeMetricsServiceAccessor : public metrics::MetricsServiceAccessor { friend void SyzyASANRegisterExperiment(const char*, const char*); friend class ChromeMetricsServiceClient; friend class ChromePasswordManagerClient; + friend class NavigationMetricsRecorder; FRIEND_TEST_ALL_PREFIXES(ChromeMetricsServiceAccessorTest, MetricsReportingEnabled); diff --git a/chrome/browser/tab_contents/navigation_metrics_recorder.cc b/chrome/browser/tab_contents/navigation_metrics_recorder.cc index d97c7df4654b..089e3b2457b4 100644 --- a/chrome/browser/tab_contents/navigation_metrics_recorder.cc +++ b/chrome/browser/tab_contents/navigation_metrics_recorder.cc @@ -4,9 +4,11 @@ #include "chrome/browser/tab_contents/navigation_metrics_recorder.h" +#include "base/feature_list.h" #include "base/metrics/histogram_macros.h" #include "build/build_config.h" #include "chrome/browser/browser_process.h" +#include "chrome/browser/metrics/chrome_metrics_service_accessor.h" #include "components/navigation_metrics/navigation_metrics.h" #include "components/rappor/public/rappor_utils.h" #include "components/rappor/rappor_service_impl.h" @@ -17,7 +19,9 @@ #include "content/public/browser/render_view_host.h" #include "content/public/browser/render_widget_host.h" #include "content/public/browser/render_widget_host_view.h" +#include "content/public/common/content_features.h" #include "content/public/common/frame_navigate_params.h" +#include "google_apis/gaia/gaia_urls.h" #include "net/base/data_url.h" #include "url/gurl.h" #include "url/origin.h" @@ -71,7 +75,20 @@ void NavigationMetricsRecorder::set_rappor_service_for_testing( void NavigationMetricsRecorder::DidFinishNavigation( content::NavigationHandle* navigation_handle) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); - if (!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted()) + if (!navigation_handle->HasCommitted()) + return; + + // This needs to go before the IsInMainFrame() check, as we want to register + // committed navigations to the sign-in URL from both main frames and + // subframes. + // + // TODO(alexmos): See if there's a better place for this check. + if (url::Origin(navigation_handle->GetURL()) == + url::Origin(GaiaUrls::GetInstance()->gaia_url())) { + RegisterSyntheticSigninIsolationTrial(); + } + + if (!navigation_handle->IsInMainFrame()) return; content::BrowserContext* context = web_contents()->GetBrowserContext(); @@ -104,3 +121,33 @@ void NavigationMetricsRecorder::DidFinishNavigation( } } } + +void NavigationMetricsRecorder::RegisterSyntheticSigninIsolationTrial() { + // For navigations to the sign-in URL, register a synthetic field trial for + // this client. This will indicate whether the sign-in process isolation is + // active, and whether or not it was activated manually via a command-line + // flag. + // + // TODO(alexmos): Remove this after the field trial for sign-in isolation. + if (base::FeatureList::IsEnabled(features::kSignInProcessIsolation)) { + if (base::FeatureList::GetInstance()->IsFeatureOverriddenFromCommandLine( + features::kSignInProcessIsolation.name, + base::FeatureList::OVERRIDE_ENABLE_FEATURE)) { + ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial( + "SignInProcessIsolationActive", "ForceEnabled"); + } else { + ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial( + "SignInProcessIsolationActive", "Enabled"); + } + } else { + if (base::FeatureList::GetInstance()->IsFeatureOverriddenFromCommandLine( + ::features::kSignInProcessIsolation.name, + base::FeatureList::OVERRIDE_DISABLE_FEATURE)) { + ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial( + "SignInProcessIsolationActive", "ForceDisabled"); + } else { + ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial( + "SignInProcessIsolationActive", "Disabled"); + } + } +} diff --git a/chrome/browser/tab_contents/navigation_metrics_recorder.h b/chrome/browser/tab_contents/navigation_metrics_recorder.h index 6fe3d907705a..5f9773eefde2 100644 --- a/chrome/browser/tab_contents/navigation_metrics_recorder.h +++ b/chrome/browser/tab_contents/navigation_metrics_recorder.h @@ -41,6 +41,8 @@ class NavigationMetricsRecorder void DidFinishNavigation( content::NavigationHandle* navigation_handle) override; + void RegisterSyntheticSigninIsolationTrial(); + rappor::RapporServiceImpl* rappor_service_; DISALLOW_COPY_AND_ASSIGN(NavigationMetricsRecorder);