Skip to content

Commit

Permalink
Add synthetic field trial for sign-in isolation. (Merge to M61)
Browse files Browse the repository at this point in the history 
This will allow filtering results for the sign-in process isolation
trial to users that have actually browsed to accounts.google.com.

[email protected]

(cherry picked from commit 95e8906)

Bug: 739418
Change-Id: I31c9b36614cbcf1440c692d29450a851a2e29aa3
Reviewed-on: https://chromium-review.googlesource.com/579629
Commit-Queue: Alex Moshchuk <[email protected]>
Reviewed-by: Avi Drissman <[email protected]>
Reviewed-by: Charlie Reis <[email protected]>
Reviewed-by: Lukasz Anforowicz <[email protected]>
Reviewed-by: Steven Holte <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#488532}
Reviewed-on: https://chromium-review.googlesource.com/586075
Reviewed-by: Alex Moshchuk <[email protected]>
Cr-Commit-Position: refs/branch-heads/3163@{#62}
Cr-Branched-From: ff259ba-refs/heads/master@{#488528}
  • Loading branch information
Alex Moshchuk committed Jul 26, 2017
1 parent 58b7f63 commit d522a7c
Show file tree
Hide file tree
Showing 4 changed files with 227 additions and 4 deletions.
176 changes: 174 additions & 2 deletions chrome/browser/chrome_navigation_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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<std::string> 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<std::string> 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);
Expand Down Expand Up @@ -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<base::FieldTrial> trial =
base::FieldTrialList::CreateFieldTrial(kTrialName, kGroupName);

std::unique_ptr<base::FeatureList> 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<base::FieldTrial> trial =
base::FieldTrialList::CreateFieldTrial(kTrialName, kGroupName);

std::unique_ptr<base::FeatureList> 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"));
}
4 changes: 3 additions & 1 deletion chrome/browser/metrics/chrome_metrics_service_accessor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
49 changes: 48 additions & 1 deletion chrome/browser/tab_contents/navigation_metrics_recorder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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");
}
}
}
2 changes: 2 additions & 0 deletions chrome/browser/tab_contents/navigation_metrics_recorder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ class NavigationMetricsRecorder
void DidFinishNavigation(
content::NavigationHandle* navigation_handle) override;

void RegisterSyntheticSigninIsolationTrial();

rappor::RapporServiceImpl* rappor_service_;

DISALLOW_COPY_AND_ASSIGN(NavigationMetricsRecorder);
Expand Down

0 comments on commit d522a7c

Please sign in to comment.