Add MarkHttpAsDangerous test and correct other MarkNonSecureAs tests

chrome/browser/ssl/ssl_browser_tests.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <utility>
 
 #include "base/base64.h"
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 681 matching lines @@
     ui_test_utils::NavigateToURL(browser(),
                                  https_server_.GetURL(replacement_path));
   }
 
  private:
   typedef net::SpawnedTestServer::SSLOptions SSLOptions;
 
   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() {}
 
   // Browser will not run insecure content.
   void SetUpCommandLine(base::CommandLine* command_line) override {
     // By overriding SSLUITest, we won't apply the flag that allows running
     // insecure content.
   }
 };
@@ skipping 590 matching lines @@
   ProceedThroughInterstitial(tab);
 
   // Test page run a WebSocket wss connection test. The result will be shown
   // as page title.
   const base::string16 result = watcher.WaitAndGetTitle();
   EXPECT_TRUE(base::LowerCaseEqualsASCII(result, "pass"));
 }
 
 // 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();
   ASSERT_TRUE(contents);
 
   SecurityStateTabHelper* helper =
       SecurityStateTabHelper::FromWebContents(contents);
   ASSERT_TRUE(helper);
 
   ui_test_utils::NavigateToURL(browser(), GURL("file:///"));
   security_state::SecurityInfo security_info;
   helper->GetSecurityInfo(&security_info);
   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();
   ASSERT_TRUE(contents);
 
   SecurityStateTabHelper* helper =
       SecurityStateTabHelper::FromWebContents(contents);
   ASSERT_TRUE(helper);
 
   ui_test_utils::NavigateToURL(browser(), GURL("about:blank"));
@@ skipping 11 matching lines @@
   SecurityStateTabHelper* helper =
       SecurityStateTabHelper::FromWebContents(contents);
   ASSERT_TRUE(helper);
 
   ui_test_utils::NavigateToURL(browser(), GURL("data:text/plain,hello"));
   security_state::SecurityInfo security_info;
   helper->GetSecurityInfo(&security_info);
   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();
   ASSERT_TRUE(contents);
 
   SecurityStateTabHelper* helper =
       SecurityStateTabHelper::FromWebContents(contents);
   ASSERT_TRUE(helper);
 
   ui_test_utils::NavigateToURL(
@@ skipping 3346 matching lines @@
 
 // Visit a page over https that contains a frame with a redirect.
 
 // XMLHttpRequest insecure content in synchronous mode.
 
 // XMLHttpRequest insecure content in asynchronous mode.
 
 // XMLHttpRequest over bad ssl in synchronous mode.
 
 // XMLHttpRequest over OK ssl in synchronous mode.