Captive portal certificate list should be checked when name mismatch is the only error

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 <unordered_set>
 #include <utility>
 
@@ skipping 529 matching lines @@
 
 void SSLErrorHandler::StartHandlingError() {
   RecordUMA(HANDLE_ALL);
 
   if (ssl_errors::ErrorInfo::NetErrorToErrorType(cert_error_) ==
       ssl_errors::ErrorInfo::CERT_DATE_INVALID) {
     HandleCertDateInvalidError();
     return;
   }
 
+  const net::CertStatus non_name_mismatch_errors =
+      ssl_info_.cert_status ^ net::CERT_STATUS_COMMON_NAME_INVALID;
+  const bool only_error_is_name_mismatch =
+      cert_error_ == net::ERR_CERT_COMMON_NAME_INVALID &&
+      (!net::IsCertStatusError(non_name_mismatch_errors) ||
+       net::IsCertStatusMinorError(ssl_info_.cert_status));
+
 #if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
+  // Check known captive portal certificate list if the only error is
+  // name-mismatch.

[estark, 2017/02/25 01:33:18]

nit: since we keep forgetting, it would probably be helpful to elaborate here on
why this is the right thing to do. Something along the lines of: "If there are
multiple errors, it indicates that the captive portal landing page itself will
have SSL errors, and so it's not a very helpful place to direct the user to go."
(is that right? I can't remember)

[meacer, 2017/02/27 23:50:58]

I think that's right, because I can't find any other justification for this.
Added the note.

I also wrote a doc to enumerate various scenarios:
https://docs.google.com/document/d/128GcLCNHyQLtv-kYOk1-6nwU_78iAhQpQWyPkN5b6...

   if (base::FeatureList::IsEnabled(kCaptivePortalCertificateList) &&
-      cert_error_ == net::ERR_CERT_COMMON_NAME_INVALID &&
+      only_error_is_name_mismatch &&
       g_config.Pointer()->IsKnownCaptivePortalCert(ssl_info_)) {
     RecordUMA(CAPTIVE_PORTAL_CERT_FOUND);
     ShowCaptivePortalInterstitial(
         GURL(captive_portal::CaptivePortalDetector::kDefaultURL));
     return;
   }
 #endif
 
   std::vector<std::string> dns_names;
   ssl_info_.cert->GetDNSNames(&dns_names);
   DCHECK(!dns_names.empty());
   GURL suggested_url;
   if (IsSSLCommonNameMismatchHandlingEnabled() &&
       cert_error_ == net::ERR_CERT_COMMON_NAME_INVALID &&
       delegate_->IsErrorOverridable() &&
       delegate_->GetSuggestedUrl(dns_names, &suggested_url)) {
     RecordUMA(WWW_MISMATCH_FOUND);
-    net::CertStatus extra_cert_errors =
-        ssl_info_.cert_status ^ net::CERT_STATUS_COMMON_NAME_INVALID;
 
     // 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)) {
+    if (!only_error_is_name_mismatch) {
       ShowSSLInterstitial();
       return;
     }
     delegate_->CheckSuggestedUrl(
         suggested_url,
         base::Bind(&SSLErrorHandler::CommonNameMismatchHandlerCallback,
                    weak_ptr_factory_.GetWeakPtr()));
     timer_.Start(FROM_HERE, g_config.Pointer()->interstitial_delay(), this,
                  &SSLErrorHandler::ShowSSLInterstitial);
 
@@ skipping 172 matching lines @@
   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) {
     ShowBadClockInterstitial(now, clock_state);
     return;  // |this| is deleted after showing the interstitial.
   }
   ShowSSLInterstitial();
 }