Add custom interstitial for captive portals.

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
new file mode 100644
index 0000000000000000000000000000000000000000..ced60669827aab5cb3988c5a1d5048845801592f
--- /dev/null
+++ b/chrome/browser/ssl/ssl_error_handler.cc
@@ -0,0 +1,196 @@
+// 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 "base/time/time.h"
+#include "base/timer/timer.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ssl/captive_portal_blocking_page.h"
+#include "chrome/browser/ssl/ssl_blocking_page.h"
+#include "chrome/common/chrome_version_info.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_source.h"
+#include "content/public/browser/web_contents.h"
+
+#if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
+#include "chrome/browser/captive_portal/captive_portal_service.h"
+#include "chrome/browser/captive_portal/captive_portal_service_factory.h"
+#endif
+
+namespace {

[mmenke, 2014/10/30 19:28:02]

nit:  blank line after namespace starts

[meacer, 2014/11/06 21:21:56]

Done.

+// Events for UMA.
+enum SSLErrorHandlerEvent {
+  HANDLE_ALL,
+  CAPTIVE_PORTAL_CHECK,
+  CAPTIVE_PORTAL_RESULT_ALREADY_HANDLED,
+  CAPTIVE_PORTAL_RESULT_BEFORE_TIMER_TRIGGER,
+  CAPTIVE_PORTAL_RESULT_AFTER_TIMER_EXPIRE,

[mmenke, 2014/10/30 19:28:02]

nit:  TRIGGERED / EXPIRED

[meacer, 2014/11/06 21:21:56]

Done.

+  SSL_WARNING_ALREADY_HANDLED,
+  SHOW_CAPTIVE_PORTAL_INTERSTITIAL,
+  SHOW_CAPTIVE_PORTAL_INTERSTITIAL_OVERRIDABLE,
+  SHOW_SSL_INTERSTITIAL,
+  SHOW_SSL_INTERSTITIAL_OVERRIDABLE,
+  SHOW_SSL_INTERSTITIAL_WITHOUT_CAPTIVE_PORTAL_CHECK,
+  SHOW_SSL_INTERSTITIAL_WITHOUT_CAPTIVE_PORTAL_CHECK_OVERRIDABLE,
+  SSL_ERROR_HANDLER_EVENT_COUNT
+};
+
+void RecordUMA(SSLErrorHandlerEvent event) {
+  UMA_HISTOGRAM_ENUMERATION("interstitial.ssl_error_handler",
+                            event,
+                            SSL_ERROR_HANDLER_EVENT_COUNT);
+}
+
+bool IsCaptivePortalInterstitialEnabled() {
+  chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel();
+  return channel <= chrome::VersionInfo::CHANNEL_DEV;
+}
+
+}  // namespace
+
+SSLErrorHandler::SSLErrorHandler(content::WebContents* web_contents,
+                                 int cert_error,
+                                 const net::SSLInfo& ssl_info,
+                                 const GURL& request_url,
+                                 const int options_mask,
+                                 const base::TimeDelta ssl_error_delay,
+                                 const base::Callback<void(bool)>& callback)
+    : content::WebContentsObserver(web_contents),
+      web_contents_(web_contents),
+      cert_error_(cert_error),
+      ssl_info_(ssl_info),
+      request_url_(request_url),
+      options_mask_(options_mask),
+      callback_(callback),
+      handled_(false),
+      captive_portal_check_count_(0),
+      ssl_interstitial_show_count_(0),
+      captive_portal_interstitial_show_count_(0),
+      ssl_interstitial_display_delay_(ssl_error_delay),
+      create_interstitials_(true),
+      timer_expired_(false) {
+}
+
+SSLErrorHandler::~SSLErrorHandler() {
+}
+
+bool SSLErrorHandler::overridable() const {

[mmenke, 2014/10/30 19:28:02]

Definition order should match declaration order

[mmenke, 2014/10/30 19:28:02]

nit:  Should either change naming style, or inline this method.

[meacer, 2014/11/06 21:21:56]

Moved to SSLBlockingPage.

+  return (options_mask_ & SSLBlockingPage::OVERRIDABLE) &&
+         !(options_mask_ & SSLBlockingPage::STRICT_ENFORCEMENT);

[mmenke, 2014/10/30 19:28:02]

I have no idea what these enum values mean, and they're not documented.

[meacer, 2014/11/06 21:21:56]

Added descriptions in ssl_blocking_page.h

+}
+
+void SSLErrorHandler::Handle() {
+  if (handled_) {
+    RecordUMA(SSL_WARNING_ALREADY_HANDLED);

[mmenke, 2014/10/30 19:33:44]

Also, this doesn't get anything.

Should just DCHECK(!handled_) and get rid of the histogram value.  In general,
we shouldn't have histograms checking for every obscure, unlikely regression.

[meacer, 2014/11/06 21:21:56]

OnCaptivePortalResult might get calledbefore Handle() is called (e.g. the portal
check was triggered by another SSL error), so it's possible that handled_ is
sometimes true, although the window is very small. I removed the histogram entry
though.

[mmenke, 2014/11/06 22:12:40]

That can't happen, because we don't spin the message loop between creating the
SSLErrorHandler and calling Handle(), and I don't think that's likely to change.
 I actually think it makes more sense to do a DCHECK(!handled_) than have an if
statement here - generally, if something can't possibly happen, the Chrome
philosophy is not to handle it.

[meacer, 2014/11/06 23:23:18]

Okay, I was assuming that could happen so I had added a test for it earlier
(ShouldShowCaptivePortalInterstitialOnTooEarlyCaptivePortalResult).

In the test as soon as I send a captive portal notification, it's received in
SSLErrorHandler. I'm not sure why, since I'm not running the message loop there.
If it can't happen, then there is no point in testing that scenario. 

[mmenke, 2014/11/06 23:39:07]

In the test it happens because you send the notification yourself between
creation and calling Handle().  When running in Chrome, those are called
synchronously on the same thread.  Any notification about captive portal status
will have to wait until the current task is complete before it can be run on the
UI thread - the current task will include both calling the constructor and the
call to Handle().

+    return;
+  }
+  RecordUMA(HANDLE_ALL);
+
+#if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
+  if (IsCaptivePortalInterstitialEnabled()) {
+    CheckForCaptivePortal();
+    timer_.Start(FROM_HERE, ssl_interstitial_display_delay_,
+                 base::Bind(&SSLErrorHandler::OnTimerExpired,
+                            base::Unretained(this)));
+    return;
+  }
+#endif
+  // Display an SSL interstitial.
+  RecordUMA(overridable() ?
+            SHOW_SSL_INTERSTITIAL_WITHOUT_CAPTIVE_PORTAL_CHECK_OVERRIDABLE :
+            SHOW_SSL_INTERSTITIAL_WITHOUT_CAPTIVE_PORTAL_CHECK);

[mmenke, 2014/10/30 19:28:02]

What does this get us?  ENABLE_CAPTIVE_PORTAL_DETECTION is defined on a
per-platform basis, and IsCaptivePortalInterstitialEnabled varies based on
build-time flags.

[meacer, 2014/11/06 21:21:56]

Removed.

+  ShowSSLInterstitial();
+  handled_ = true;
+}
+
+void SSLErrorHandler::CheckForCaptivePortal() {
+  captive_portal_check_count_++;
+  // No need to do an actual captive portal check if no interstitial is shown.
+  if (!create_interstitials_)
+    return;
+
+#if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
+  RecordUMA(CAPTIVE_PORTAL_CHECK);
+  Profile* profile = Profile::FromBrowserContext(
+      web_contents_->GetBrowserContext());
+  CaptivePortalService* captive_portal_service =
+      CaptivePortalServiceFactory::GetForProfile(profile);
+  captive_portal_service->DetectCaptivePortal();
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT,
+                 content::Source<Profile>(profile));
+#endif
+}
+
+void SSLErrorHandler::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  // TODO(meacer): Refactor this method in SSLBlockingPage.
+#if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
+  if (type == chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT) {
+    CaptivePortalService::Results* results =
+        content::Details<CaptivePortalService::Results>(details).ptr();
+    if (results->result == captive_portal::RESULT_BEHIND_CAPTIVE_PORTAL)
+      OnCaptivePortalResult();

[mmenke, 2014/10/30 19:28:02]

If we don't see a captive portal, and handled_ is false, shouldn't we just call
ShowSSLInterstitial()?

[meacer, 2014/11/06 21:21:56]

Hmm, isn't it possible that we could see a captive portal in a second
notification before the timer expires? I guess it's a very edge case given the
exponential backoff behavior though.

[mmenke, 2014/11/06 22:12:40]

Yes, it's possible, theoretically, but due to exponential backoff, in practice
it won't.  I think it makes more sense to trust the result we get, rather than
wait for another one that won't actually happen, and only show an error page
after a timeout.

Better responsiveness in the average case is better than better handling of a
corner case that probably never happens, in my opinion.

[meacer, 2014/11/06 23:23:18]

Sure, will do.

+  }
+#endif
+}
+
+void SSLErrorHandler::OnCaptivePortalResult() {

[mmenke, 2014/10/30 19:28:02]

"CaptivePortalResult" is a very confusing name here, and for the histogram enum.
 It actually means "I have a CaptivePortalService::Results, and that indicated a
captive portal".

I suggest inlining this function, since it's clearer when inlined (For tests,
send a bogus notification instead - that'll exercise the logic in
SSLErrorHandler::Observe as well), and renaming
CAPTIVE_PORTAL_RESULT_AFTER_TIMER_EXPIRE to
CAPTIVE_PORTAL_DETECTED_AFTER_TIMER_EXPIRE.

[meacer, 2014/11/06 21:21:56]

Done.

+  if (!timer_.IsRunning()) {
+    RecordUMA(timer_expired_ ?
+              CAPTIVE_PORTAL_RESULT_AFTER_TIMER_EXPIRE :
+              CAPTIVE_PORTAL_RESULT_BEFORE_TIMER_TRIGGER);
+  }
+  if (handled_) {

[mmenke, 2014/10/30 19:28:02]

Is there any case where handled_ != timer_.IsRunning() here?

[meacer, 2014/11/06 21:21:56]

That would be the general case where timer is running and a captive portal is
detected before timer expires, right?

1. Handle() -> start timer. IsRunning = true, handled_ = false
2. OnCaptivePortalResult -> IsRunning = true, handled_ = false

[mmenke, 2014/11/06 22:12:40]

Sorry, I meant handled_ != !timer_.IsRunning().  i.e., isn't this the same as
the above timer_.IsRunning() check?  Both cases where we set handled_ to true,
we also stop the timer, so the check is redundant.

[meacer, 2014/11/06 23:23:18]

Good point, looks like CAPTIVE_PORTAL_DETECTED_AFTER_TIMER_EXPIRED and
BEFORE_TIMER_TRIGGERED aren't needed since we'd show the interstitial when that
happens and the SSLErrorHandler will go away anyways.

+    RecordUMA(CAPTIVE_PORTAL_RESULT_ALREADY_HANDLED);
+    return;
+  }
+  timer_.Stop();
+  ShowCaptivePortalInterstitial();
+  handled_ = true;
+}
+
+void SSLErrorHandler::OnTimerExpired() {
+  timer_expired_ = true;
+  if (handled_) {
+    return;
+  }
+  ShowSSLInterstitial();
+  handled_ = true;
+}
+
+void SSLErrorHandler::ShowCaptivePortalInterstitial() {
+  captive_portal_interstitial_show_count_++;

[mmenke, 2014/10/30 19:28:02]

If you want to go this route for testing, I think it makes more sense to make
ShowCaptivePortalInterstitial / ShowSSLInterstitial virtual, and have the
derived class deal with the counts.  Then you can get rid of
create_interstitials_, too.

[meacer, 2014/11/06 21:21:56]

Done.

+  if (!create_interstitials_)
+    return;
+  // Show captive portal blocking page. The interstitial owns the blocking page.
+  RecordUMA(overridable() ?
+            SHOW_CAPTIVE_PORTAL_INTERSTITIAL_OVERRIDABLE :
+            SHOW_CAPTIVE_PORTAL_INTERSTITIAL);
+  (new CaptivePortalBlockingPage(web_contents_, request_url_))->Show();
+}
+
+void SSLErrorHandler::ShowSSLInterstitial() {
+  ssl_interstitial_show_count_++;
+  if (!create_interstitials_)
+    return;
+  // Show SSL blocking page. The interstitial owns the blocking page.
+  RecordUMA(overridable() ?
+            SHOW_SSL_INTERSTITIAL_OVERRIDABLE :
+            SHOW_SSL_INTERSTITIAL);
+  (new SSLBlockingPage(web_contents_, cert_error_, ssl_info_, request_url_,
+                       options_mask_, callback_))->Show();
+}
+
+void SSLErrorHandler::DidStopLoading(
+    content::RenderViewHost* render_view_host) {
+  delete this;
+}
+
+void SSLErrorHandler::WebContentsDestroyed() {
+  delete this;
+}