Remove DHE.

components/ssl_config/ssl_config_service_manager_pref_unittest.cc

Index: components/ssl_config/ssl_config_service_manager_pref_unittest.cc
diff --git a/components/ssl_config/ssl_config_service_manager_pref_unittest.cc b/components/ssl_config/ssl_config_service_manager_pref_unittest.cc
index 07aaef88f48dbed2b72f3e8318ce3ea3400e32f3..b17d4c5da55945e9c2de5e53030622bce733240c 100644
--- a/components/ssl_config/ssl_config_service_manager_pref_unittest.cc
+++ b/components/ssl_config/ssl_config_service_manager_pref_unittest.cc
@@ -5,6 +5,7 @@
 #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"
@@ -197,3 +198,26 @@ TEST_F(SSLConfigServiceManagerPrefTest, NoTLS1Fallback) {
   // 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);
+}