Remove DHE.

components/ssl_config/ssl_config_service_manager_pref_unittest.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 <memory>
 #include <utility>
 
+#include "base/feature_list.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/ssl_config/ssl_config_prefs.h"
 #include "components/ssl_config/ssl_config_service_manager.h"
 #include "components/ssl_config/ssl_config_switches.h"
 #include "net/ssl/ssl_config.h"
 #include "net/ssl/ssl_config_service.h"
@@ skipping 172 matching lines @@
           &local_state, base::ThreadTaskRunnerHandle::Get()));
   ASSERT_TRUE(config_manager.get());
   scoped_refptr<SSLConfigService> config_service(config_manager->Get());
   ASSERT_TRUE(config_service.get());
 
   SSLConfig ssl_config;
   config_service->GetSSLConfig(&ssl_config);
   // The command-line option must not have been honored.
   EXPECT_EQ(net::SSL_PROTOCOL_VERSION_TLS1_2, ssl_config.version_fallback_min);
 }
+
+// Tests that DHE may be re-enabled via features.
+TEST_F(SSLConfigServiceManagerPrefTest, DHEFeature) {
+  // Toggle the feature.
+  base::FeatureList::ClearInstanceForTesting();

[Ryan Sleevi, 2016/06/15 01:23:25]

Doesn't the end of this test leave DHE enabled? Why not toggle it via the Prefs?


Caveat: I'm not a fan of FeatureList, and from looking at
ClearInstanceForTesting() call sites, I feel my hate is a bit justified. Most
consumers - other than a few, such as ScopedSubresourceFilterFeatureToggle,
TestBlinkWebUnitTestSupport, and VariationsSeedProcessorTest - end up leaving
the feature test enabled, and are relying on the side-effect that flaky tests
will be retried in new processes.

I'm inclined to suggest a test-level scoper (like ScopedSubresource...) to
handle this. You could get away with just

ScopedClosureRunner
cleanup(base::Bind(&base::FeatureList::ClearInstanceForTesting));

at the top, followed by line 205.

[davidben, 2016/06/15 19:21:20]

The whole point of this test is to make sure the FeatureList plumbing works.
Metrics folks asked that I use FeatureList the last time this came up. This is
just the previous version of this test. But I agree this mess of globals is a
problem.
That doesn't work because base::FeatureList does not start out cleared. Tests
which call FeatureList::IsEnabled will crash instead. Unfortunately, asvitkine
is OOO for a while. I'm thinking we'd need a base::ScopedFeatureListInstance
that takes ownership of the current FeatureList, replace with a new one, and
then swaps it back afterwards.

That's enough of a mess that I'd rather deal with it asynchronously.

[davidben, 2016/06/15 19:49:18]

I started trying to put this together, but there's enough mess around
initializing things that this probably should be done by base::FeatureList
folks.

In the meantime, I suggest we either leave it as-is, or I end the test with
base::FeatureList::SetInstance(base::WrapUnique(new FeatureList)) in hopes that
it's *probably* not messing with too much state.

+  std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
+  feature_list->InitializeFromCommandLine("DHECiphers", std::string());
+  base::FeatureList::SetInstance(std::move(feature_list));
+
+  TestingPrefServiceSimple local_state;
+  SSLConfigServiceManager::RegisterPrefs(local_state.registry());
+
+  std::unique_ptr<SSLConfigServiceManager> config_manager(
+      SSLConfigServiceManager::CreateDefaultManager(
+          &local_state, base::ThreadTaskRunnerHandle::Get()));
+  scoped_refptr<SSLConfigService> config_service(config_manager->Get());
+  ASSERT_TRUE(config_service.get());
+
+  // The feature should have switched the default version_fallback_min value.
+  SSLConfig ssl_config;
+  config_service->GetSSLConfig(&ssl_config);
+  EXPECT_TRUE(ssl_config.dhe_enabled);
+}