Wrap SSL error handler configuration with a lazy instance

chrome/browser/ssl/ssl_error_handler.cc

 // Copyright 2014 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 "chrome/browser/ssl/ssl_error_handler.h"
 
 #include <stdint.h>
 #include <utility>
 
 #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"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ssl/bad_clock_blocking_page.h"
 #include "chrome/browser/ssl/ssl_blocking_page.h"
 #include "chrome/browser/ssl/ssl_cert_reporter.h"
 #include "chrome/common/features.h"
 #include "components/network_time/network_time_tracker.h"
 #include "components/ssl_errors/error_classification.h"
 #include "components/ssl_errors/error_info.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/net_errors.h"
 
 #if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
 #include "chrome/browser/captive_portal/captive_portal_service.h"
 #include "chrome/browser/captive_portal/captive_portal_service_factory.h"
 #include "chrome/browser/captive_portal/captive_portal_tab_helper.h"
 #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)
 const base::Feature kCaptivePortalInterstitial{
     "CaptivePortalInterstitial", base::FEATURE_ENABLED_BY_DEFAULT};
 #endif
 
 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 {
   HANDLE_ALL,
   SHOW_CAPTIVE_PORTAL_INTERSTITIAL_NONOVERRIDABLE,
   SHOW_CAPTIVE_PORTAL_INTERSTITIAL_OVERRIDABLE,
   SHOW_SSL_INTERSTITIAL_NONOVERRIDABLE,
   SHOW_SSL_INTERSTITIAL_OVERRIDABLE,
   WWW_MISMATCH_FOUND,
   WWW_MISMATCH_URL_AVAILABLE,
@@ skipping 67 matching lines @@
 #if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
 bool IsCaptivePortalInterstitialEnabled() {
   return base::FeatureList::IsEnabled(kCaptivePortalInterstitial);
 }
 #endif
 
 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);
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(CommonNameMismatchRedirectObserver);
 
 void SSLErrorHandler::HandleSSLError(
     content::WebContents* web_contents,
     int cert_error,
     const net::SSLInfo& ssl_info,
     const GURL& request_url,
     int options_mask,
     std::unique_ptr<SSLCertReporter> ssl_cert_reporter,
     const base::Callback<void(content::CertificateRequestResultType)>&
         callback) {
   DCHECK(!FromWebContents(web_contents));
   SSLErrorHandler* error_handler =
       new SSLErrorHandler(web_contents, cert_error, ssl_info, request_url,
                           options_mask, std::move(ssl_cert_reporter), callback);
   web_contents->SetUserData(UserDataKey(), error_handler);
   error_handler->StartHandlingError();
 }
 
 // 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(
     content::WebContents* web_contents,
     int cert_error,
     const net::SSLInfo& ssl_info,
     const GURL& request_url,
     int options_mask,
     std::unique_ptr<SSLCertReporter> ssl_cert_reporter,
     const base::Callback<void(content::CertificateRequestResultType)>& callback)
@@ skipping 33 matching lines @@
 
     // Show the SSL intersitial if |CERT_STATUS_COMMON_NAME_INVALID| is not
     // the only error. Need not check for captive portal in this case.
     // (See the comment below).
     if (net::IsCertStatusError(extra_cert_errors) &&
         !net::IsCertStatusMinorError(ssl_info_.cert_status)) {
       ShowSSLInterstitial();
       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
     // similarity check.
     return;
   }
 
 #if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
   CaptivePortalTabHelper* captive_portal_tab_helper =
       CaptivePortalTabHelper::FromWebContents(web_contents_);
   if (captive_portal_tab_helper) {
     captive_portal_tab_helper->OnSSLCertError(ssl_info_);
   }
 
   registrar_.Add(this, chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT,
                  content::Source<Profile>(profile_));
 
   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
   // Display an SSL interstitial.
   ShowSSLInterstitial();
 }
 
 void SSLErrorHandler::CheckForCaptivePortal() {
 #if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
   CaptivePortalService* captive_portal_service =
@@ skipping 134 matching lines @@
   if (common_name_mismatch_handler_) {
     common_name_mismatch_handler_->Cancel();
     common_name_mismatch_handler_.reset();
   }
   // Deletes |this| and also destroys the timer.
   web_contents_->RemoveUserData(UserDataKey());
 }
 
 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
   // true time. This will only have an effect if network time is
   // unavailable or if there is not already a query in progress.
   //
   // 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))) {
     HandleCertDateInvalidErrorImpl(now);
     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(
     base::TimeTicks started_handling_error) {
   UMA_HISTOGRAM_CUSTOM_TIMES(
       "interstitial.ssl_error_handler.cert_date_error_delay",
       base::TimeTicks::Now() - started_handling_error,
       base::TimeDelta::FromMilliseconds(1), base::TimeDelta::FromSeconds(4),
       50);
 
+  timer_.Stop();
+  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_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();
+      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) {
     ShowBadClockInterstitial(now, clock_state);
     return;  // |this| is deleted after showing the interstitial.
   }
   ShowSSLInterstitial();
 }