Wrap SSL error handler configuration with a lazy instance

chrome/browser/ssl/ssl_error_handler.cc

Index: chrome/browser/ssl/ssl_error_handler.cc
diff --git a/chrome/browser/ssl/ssl_error_handler.cc b/chrome/browser/ssl/ssl_error_handler.cc
index 7b8f6e776678f2696031744ee02b50ec7034e5f9..d02c7315b931f4246d07ade5999ac99c009a5d5a 100644
--- a/chrome/browser/ssl/ssl_error_handler.cc
+++ b/chrome/browser/ssl/ssl_error_handler.cc
@@ -9,9 +9,11 @@
 
 #include "base/callback_helpers.h"
 #include "base/feature_list.h"
+#include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/stringprintf.h"
+#include "base/threading/non_thread_safe.h"
 #include "base/time/clock.h"
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
@@ -36,10 +38,6 @@
 #include "chrome/browser/ssl/captive_portal_blocking_page.h"
 #endif
 
-namespace network_time {
-class NetworkTimeTracker;

[estark, 2017/01/08 17:01:51]

doh. thanks for cleaning up my mess :)

[meacer, 2017/01/09 20:48:26]

Np :)

-}
-
 namespace {
 
 #if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
@@ -50,22 +48,14 @@ const base::Feature kCaptivePortalInterstitial{
 const base::Feature kSSLCommonNameMismatchHandling{
     "SSLCommonNameMismatchHandling", base::FEATURE_ENABLED_BY_DEFAULT};
 
-// The delay in milliseconds before displaying the SSL interstitial.
+// Default delay in milliseconds before displaying the SSL interstitial.
 // This can be changed in tests.
 // - If there is a name mismatch and a suggested URL available result arrives
 //   during this time, the user is redirected to the suggester URL.
 // - If a "captive portal detected" result arrives during this time,
 //   a captive portal interstitial is displayed.
 // - Otherwise, an SSL interstitial is displayed.
-int64_t g_interstitial_delay_in_milliseconds = 3000;
-
-// Callback to call when the interstitial timer is started. Used for testing.
-SSLErrorHandler::TimerStartedCallback* g_timer_started_callback = nullptr;
-
-// The clock to use when deciding which error type to display. Used for testing.
-base::Clock* g_testing_clock = nullptr;
-
-network_time::NetworkTimeTracker* g_network_time_tracker = nullptr;
+const int64_t kInterstitialDelayInMilliseconds = 3000;
 
 // Events for UMA.
 enum SSLErrorHandlerEvent {
@@ -153,6 +143,82 @@ bool IsSSLCommonNameMismatchHandlingEnabled() {
   return base::FeatureList::IsEnabled(kSSLCommonNameMismatchHandling);
 }
 
+// Configuration for SSLErrorHandler.
+class ConfigSingleton : public base::NonThreadSafe {
+ public:
+  ConfigSingleton();
+
+  base::TimeDelta interstitial_delay() const;
+  SSLErrorHandler::TimerStartedCallback* timer_started_callback() const;
+  base::Clock* clock() const;
+  network_time::NetworkTimeTracker* network_time_tracker() const;
+
+  void SetInterstitialDelayForTest(const base::TimeDelta& delay);

[estark, 2017/01/08 17:01:51]

tiny nit: I think the suffix is technically supposed to be ForTest*ing*
(here and below)
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[meacer, 2017/01/09 20:48:26]

Done, removed ForTest* suffixes from ConfigSingleton, changed them to ForTesting
in SSLErrorHandler.

+  void SetTimerStartedCallback(SSLErrorHandler::TimerStartedCallback* callback);

[estark, 2017/01/08 17:01:51]

tiny nit: set_timer_started_callback() for consistency with
timer_started_callback()

[meacer, 2017/01/09 20:48:26]

Hmm, I feel like I'm missing something. Why would this be named differently than
the other setters? Or did you mean set_timer_started_callback_for_testing? (but
even then, the style would be different)

[estark, 2017/01/09 23:13:41]

Oh, I had missed that this setter was for testing only. My reasoning was:
1. my_data_member() and set_my_data_member() are allowed naming conventions for
getters/setters...
2. but if the getter is in that style, then the setter should be too...
3. unless the setter is for testing only, in which case it should have the
ForTesting suffix which means that it has to be SetMyDataMemberForTesting().

So I concluded that this *not* testing-only setter should be
set_my_data_member() while the other testing-only setters should be
SetMyDataMemberForTesting().

I guess they are all testing-only so they should all be SetBlahForTesting?

[meacer, 2017/01/09 23:38:06]

I see. I'm by no means an expert on these, but I seem to remember
set_data_member_for_testing() is also fine and that the decision between
SetMyDataMemberForTesting and set_data_member_for_testing is made on whether
it's a simple setter or not, no? (at least, there seem to be examples of it:
https://cs.chromium.org/search/?q=set_%5Ba-zA-Z_%5D%2Bfor_testing+chrome/&sq=...)
For now they are all test setters, but the new setter for the captive portal
list will be used from production code as well (when there is a new component
updater component). But renamed them in the meanwhile.

+  void SetClockForTest(base::Clock* clock);
+  void SetNetworkTimeTrackerForTest(network_time::NetworkTimeTracker* tracker);
+
+ private:
+  base::TimeDelta interstitial_delay_;
+
+  // Callback to call when the interstitial timer is started. Used for
+  // testing.
+  SSLErrorHandler::TimerStartedCallback* timer_started_callback_ = nullptr;

[estark, 2017/01/08 17:01:51]

Not particular to this CL, but it's a little weird to have a base::Callback*
isn't it? I feel like it's usually not a pointer. Dunno if it matters.

[meacer, 2017/01/09 20:48:26]

The original idea was so that it could be left as null as opposed to keeping it
by value or ref (since timer_started_callback was a global variable and we
didn't want complex initializers)

[estark, 2017/01/09 23:13:41]

Acknowledged.

+
+  // The clock to use when deciding which error type to display. Used for
+  // testing.
+  base::Clock* testing_clock_ = nullptr;
+
+  network_time::NetworkTimeTracker* network_time_tracker_ = nullptr;
+};
+
+ConfigSingleton::ConfigSingleton()
+    : interstitial_delay_(
+          base::TimeDelta::FromMilliseconds(kInterstitialDelayInMilliseconds)) {
+}
+
+base::TimeDelta ConfigSingleton::interstitial_delay() const {
+  return interstitial_delay_;
+}
+
+SSLErrorHandler::TimerStartedCallback* ConfigSingleton::timer_started_callback()
+    const {
+  return timer_started_callback_;
+}
+
+network_time::NetworkTimeTracker* ConfigSingleton::network_time_tracker()
+    const {
+  return network_time_tracker_ ? network_time_tracker_
+                               : g_browser_process->network_time_tracker();
+}
+
+base::Clock* ConfigSingleton::clock() const {
+  return testing_clock_;
+}
+
+void ConfigSingleton::SetInterstitialDelayForTest(
+    const base::TimeDelta& delay) {
+  interstitial_delay_ = delay;
+}
+
+void ConfigSingleton::SetTimerStartedCallback(
+    SSLErrorHandler::TimerStartedCallback* callback) {
+  DCHECK(!callback || !callback->is_null());
+  timer_started_callback_ = callback;
+}
+
+void ConfigSingleton::SetClockForTest(base::Clock* clock) {
+  testing_clock_ = clock;
+}
+
+void ConfigSingleton::SetNetworkTimeTrackerForTest(
+    network_time::NetworkTimeTracker* tracker) {
+  network_time_tracker_ = tracker;
+}
+
+static base::LazyInstance<ConfigSingleton>::Leaky g_config =
+    LAZY_INSTANCE_INITIALIZER;
+
 }  // namespace
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(SSLErrorHandler);
@@ -176,26 +242,26 @@ void SSLErrorHandler::HandleSSLError(
 }
 
 // static
-void SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta delay) {
-  g_interstitial_delay_in_milliseconds = delay.InMilliseconds();
+void SSLErrorHandler::SetInterstitialDelayForTest(
+    const base::TimeDelta& delay) {
+  g_config.Pointer()->SetInterstitialDelayForTest(delay);
 }
 
 // static
 void SSLErrorHandler::SetInterstitialTimerStartedCallbackForTest(
     TimerStartedCallback* callback) {
-  DCHECK(!callback || !callback->is_null());
-  g_timer_started_callback = callback;
+  g_config.Pointer()->SetTimerStartedCallback(callback);
 }
 
 // static
 void SSLErrorHandler::SetClockForTest(base::Clock* testing_clock) {
-  g_testing_clock = testing_clock;
+  g_config.Pointer()->SetClockForTest(testing_clock);
 }
 
 // static
 void SSLErrorHandler::SetNetworkTimeTrackerForTest(
     network_time::NetworkTimeTracker* tracker) {
-  g_network_time_tracker = tracker;
+  g_config.Pointer()->SetNetworkTimeTrackerForTest(tracker);
 }
 
 SSLErrorHandler::SSLErrorHandler(
@@ -249,11 +315,11 @@ void SSLErrorHandler::StartHandlingError() {
       return;
     }
     CheckSuggestedUrl(suggested_url);
-    timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(
-                                g_interstitial_delay_in_milliseconds),
-                 this, &SSLErrorHandler::ShowSSLInterstitial);
-    if (g_timer_started_callback)
-      g_timer_started_callback->Run(web_contents_);
+    timer_.Start(FROM_HERE, g_config.Pointer()->interstitial_delay(), this,
+                 &SSLErrorHandler::ShowSSLInterstitial);
+
+    if (g_config.Pointer()->timer_started_callback())
+      g_config.Pointer()->timer_started_callback()->Run(web_contents_);
 
     // Do not check for a captive portal in this case, because a captive
     // portal most likely cannot serve a valid certificate which passes the
@@ -273,11 +339,10 @@ void SSLErrorHandler::StartHandlingError() {
 
   if (IsCaptivePortalInterstitialEnabled()) {
     CheckForCaptivePortal();
-    timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(
-                                g_interstitial_delay_in_milliseconds),
-                 this, &SSLErrorHandler::ShowSSLInterstitial);
-    if (g_timer_started_callback)
-      g_timer_started_callback->Run(web_contents_);
+    timer_.Start(FROM_HERE, g_config.Pointer()->interstitial_delay(), this,
+                 &SSLErrorHandler::ShowSSLInterstitial);
+    if (g_config.Pointer()->timer_started_callback())
+      g_config.Pointer()->timer_started_callback()->Run(web_contents_);
     return;
   }
 #endif
@@ -432,11 +497,7 @@ void SSLErrorHandler::DeleteSSLErrorHandler() {
 
 void SSLErrorHandler::HandleCertDateInvalidError() {
   const base::TimeTicks now = base::TimeTicks::Now();
-  network_time::NetworkTimeTracker* tracker =
-      g_network_time_tracker ? g_network_time_tracker
-                             : g_browser_process->network_time_tracker();
-  timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(
-                              g_interstitial_delay_in_milliseconds),
+  timer_.Start(FROM_HERE, g_config.Pointer()->interstitial_delay(),
                base::Bind(&SSLErrorHandler::HandleCertDateInvalidErrorImpl,
                           base::Unretained(this), now));
   // Try kicking off a time fetch to get an up-to-date estimate of the
@@ -446,6 +507,8 @@ void SSLErrorHandler::HandleCertDateInvalidError() {
   // Pass a weak pointer as the callback; if the timer fires before the
   // fetch completes and shows an interstitial, this SSLErrorHandler
   // will be deleted.
+  network_time::NetworkTimeTracker* tracker =
+      g_config.Pointer()->network_time_tracker();
   if (!tracker->StartTimeFetch(
           base::Bind(&SSLErrorHandler::HandleCertDateInvalidErrorImpl,
                      weak_ptr_factory_.GetWeakPtr(), now))) {
@@ -453,8 +516,8 @@ void SSLErrorHandler::HandleCertDateInvalidError() {
     return;
   }
 
-  if (g_timer_started_callback)
-    g_timer_started_callback->Run(web_contents_);
+  if (g_config.Pointer()->timer_started_callback())
+    g_config.Pointer()->timer_started_callback()->Run(web_contents_);
 }
 
 void SSLErrorHandler::HandleCertDateInvalidErrorImpl(
@@ -465,13 +528,13 @@ void SSLErrorHandler::HandleCertDateInvalidErrorImpl(
       base::TimeDelta::FromMilliseconds(1), base::TimeDelta::FromSeconds(4),
       50);
 
-  network_time::NetworkTimeTracker* tracker =
-      g_network_time_tracker ? g_network_time_tracker
-                             : g_browser_process->network_time_tracker();
   timer_.Stop();
-  const base::Time now = g_testing_clock == nullptr
-                             ? base::Time::NowFromSystemTime()
-                             : g_testing_clock->Now();
+  base::Clock* testing_clock = g_config.Pointer()->clock();
+  const base::Time now =
+      testing_clock ? testing_clock->Now() : base::Time::NowFromSystemTime();
+
+  network_time::NetworkTimeTracker* tracker =
+      g_config.Pointer()->network_time_tracker();
   ssl_errors::ClockState clock_state = ssl_errors::GetClockState(now, tracker);
   if (clock_state == ssl_errors::CLOCK_STATE_FUTURE ||
       clock_state == ssl_errors::CLOCK_STATE_PAST) {