Add MarkHttpAsDangerous test and correct other MarkNonSecureAs tests

chrome/browser/ssl/ssl_browser_tests.cc

Index: chrome/browser/ssl/ssl_browser_tests.cc
diff --git a/chrome/browser/ssl/ssl_browser_tests.cc b/chrome/browser/ssl/ssl_browser_tests.cc
index fe83aa907a7652395726717625abc553a385ffba..7bd3b57de3aa96264872e57ea41623bb8bb91bb5 100644
--- a/chrome/browser/ssl/ssl_browser_tests.cc
+++ b/chrome/browser/ssl/ssl_browser_tests.cc
@@ -699,6 +699,17 @@ class SSLUITest : public InProcessBrowserTest {
   DISALLOW_COPY_AND_ASSIGN(SSLUITest);
 };
 
+class SSLUITestIgnoringFieldTrialConfig : public SSLUITest {
+ public:
+  SSLUITestIgnoringFieldTrialConfig() : SSLUITest() {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    // Ensure that fieldtrial_testing_config.json does not interfere.
+    command_line->AppendSwitch(
+        variations::switches::kDisableFieldTrialTestingConfig);

[Ilya Sherman, 2017/06/05 23:21:12]

Is this still needed given that you are removing the config?

[elawrence, 2017/06/06 01:05:17]

An important caveat for this CL is that we will very shortly (e.g. this week) be
adding new states to mark-non-secure-as/MarkNonsecureAs as a part of our HTTPBad
Phase 2 work. Soon after we introduce the new states, we will likely reintroduce
MarkNonsecureAs override in the fieldtrial_testing_config.json file with a new
"default" setting that we expect will eventually be the M61 shipping default
value. As a consequence, yes, we should have this call here to ensure that tests
based on this fixture work as intended and do not vary based on the JSON.

[Ilya Sherman, 2017/06/06 05:51:13]

I see.  I'm not very thrilled about this choice, as it basically means that
these tests are opting out of coverage by *all other field trials*.  That means
that if there's any interaction between those trials and the code under test
here, it won't be caught by the bots.

Could you instead register the appropriate field trial during SetUp()?

[elawrence, 2017/06/06 19:50:02]

Yeah, that's not great.
Unfortunately, that doesn't appear to work, as the Field Trials system isn't
initialized at that point and calling CreateFieldTrial() fails the
DHECK(global_) test at the start of the function. 

I suppose we can change the tests so that they don't use field trials at all
(instead using the command-line flag to control the feature).

+  }
+};
+
 class SSLUITestBlock : public SSLUITest {
  public:
   SSLUITestBlock() : SSLUITest() {}
@@ -1309,10 +1320,11 @@ IN_PROC_BROWSER_TEST_F(SSLUITest, TestWSSInvalidCertAndGoForward) {
 
 // Ensure that non-standard origins are marked correctly when the
 // MarkNonSecureAs field trial is enabled.
-IN_PROC_BROWSER_TEST_F(SSLUITest, MarkFileAsNonSecure) {
+IN_PROC_BROWSER_TEST_F(SSLUITestIgnoringFieldTrialConfig, MarkFileAsNonSecure) {
   scoped_refptr<base::FieldTrial> trial =
       base::FieldTrialList::CreateFieldTrial(
           "MarkNonSecureAs", security_state::switches::kMarkHttpAsDangerous);

[Ilya Sherman, 2017/06/05 23:21:12]

If the code is launched to stable, then why do you need to test different field
trial states?

[elawrence, 2017/06/06 01:05:17]

Marking of HTTP is controlled by chrome://flags#mark-non-secure-as and a
matching field trial named MarkNonsecureAs. In Chrome versions prior to 56, this
setting had as many as four states. 

In Chrome 60, it now has two states (Default, MarkHttpAsDangerous). The existing
tests use the field trial group name to exercise the different codepaths that
respect the states.

The fieldtrial_testing_config.json file currently reiterates the default state
of the feature. The fieldtrial_testing_config file overrides any settings set in
the test, so reiterating the default state in the JSON makes the
CreateFieldTrial calls ineffective. To fix that, in the first version of this
CL, I set the command line flag for the relevant test fixtures to ignore the
JSON. 

Emily pointed out that we should also remove the redundant declaration in the
JSON anyway (as it has no real purpose at this time).

+  ASSERT_TRUE(trial != nullptr);
 
   content::WebContents* contents =
       browser()->tab_strip_model()->GetActiveWebContents();
@@ -1328,10 +1340,12 @@ IN_PROC_BROWSER_TEST_F(SSLUITest, MarkFileAsNonSecure) {
   EXPECT_EQ(security_state::NONE, security_info.security_level);
 }
 
-IN_PROC_BROWSER_TEST_F(SSLUITest, MarkAboutAsNonSecure) {
+IN_PROC_BROWSER_TEST_F(SSLUITestIgnoringFieldTrialConfig,
+                       MarkAboutAsNonSecure) {
   scoped_refptr<base::FieldTrial> trial =
       base::FieldTrialList::CreateFieldTrial(
           "MarkNonSecureAs", security_state::switches::kMarkHttpAsDangerous);
+  ASSERT_TRUE(trial != nullptr);
 
   content::WebContents* contents =
       browser()->tab_strip_model()->GetActiveWebContents();
@@ -1363,10 +1377,32 @@ IN_PROC_BROWSER_TEST_F(SSLUITest, MarkDataAsNonSecure) {
   EXPECT_EQ(security_state::HTTP_SHOW_WARNING, security_info.security_level);
 }
 
-IN_PROC_BROWSER_TEST_F(SSLUITest, MarkBlobAsNonSecure) {
+// Ensure that HTTP origins are marked correctly when the MarkNonSecureAs field
+// trial is set to MarkHttpAsDangerous.
+IN_PROC_BROWSER_TEST_F(SSLUITestIgnoringFieldTrialConfig, MarkHTTPAsDangerous) {
   scoped_refptr<base::FieldTrial> trial =
       base::FieldTrialList::CreateFieldTrial(
           "MarkNonSecureAs", security_state::switches::kMarkHttpAsDangerous);
+  ASSERT_TRUE(trial != nullptr);
+
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Navigate to a non-local HTTP page.
+  ui_test_utils::NavigateToURL(browser(), GURL("http://example.com/"));
+  WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
+  SecurityStateTabHelper* helper = SecurityStateTabHelper::FromWebContents(tab);
+  ASSERT_TRUE(helper);
+
+  security_state::SecurityInfo security_info;
+  helper->GetSecurityInfo(&security_info);
+  EXPECT_EQ(security_state::DANGEROUS, security_info.security_level);
+}
+
+IN_PROC_BROWSER_TEST_F(SSLUITestIgnoringFieldTrialConfig, MarkBlobAsNonSecure) {
+  scoped_refptr<base::FieldTrial> trial =
+      base::FieldTrialList::CreateFieldTrial(
+          "MarkNonSecureAs", security_state::switches::kMarkHttpAsDangerous);
+  ASSERT_TRUE(trial != nullptr);
 
   content::WebContents* contents =
       browser()->tab_strip_model()->GetActiveWebContents();