Fix flaky browser test SSLUITest.BadCertFollowedByGoodCert

chrome/browser/ssl/ssl_browser_tests.cc

 // Copyright (c) 2012 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 "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
 #include "base/prefs/pref_service.h"
@@ skipping 24 matching lines @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/variations/variations_associated_data.h"
 #include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/interstitial_page.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"

[davidben, 2015/04/24 14:53:00]

No longer needed.

[jww, 2015/04/24 17:26:35]

Done.

 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/security_style.h"
 #include "content/public/common/ssl_status.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/download_test_observer.h"
 #include "content/public/test/test_renderer_host.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/net_errors.h"
 #include "net/base/test_data_directory.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/cert/test_root_certs.h"

[davidben, 2015/04/24 14:53:00]

No longer needed.

[jww, 2015/04/24 17:26:35]

Done.

 #include "net/cert/x509_certificate.h"
 #include "net/dns/host_resolver.h"
 #include "net/dns/mock_host_resolver.h"

[davidben, 2015/04/24 14:53:00]

Not sure the two host resolver includes were ever needed.

[jww, 2015/04/24 17:26:35]

Yeah, I added those by accident (or rather, forgot to remove them after I was
messing around with ideas). Removed.

 #include "net/http/http_transaction_factory.h"

[davidben, 2015/04/24 14:53:00]

No longer needed.

[jww, 2015/04/24 17:26:35]

Done.

 #include "net/ssl/ssl_info.h"
 #include "net/test/spawned_test_server/spawned_test_server.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"

[davidben, 2015/04/24 14:53:00]

No longer needed.

[jww, 2015/04/24 17:26:35]

Done.

 
 #if defined(USE_NSS_CERTS)
 #include "chrome/browser/net/nss_context.h"
 #include "net/base/crypto_module.h"
 #include "net/cert/nss_cert_database.h"
 #endif  // defined(USE_NSS_CERTS)
 
 using base::ASCIIToUTF16;
 using chrome_browser_interstitials::SecurityInterstitialIDNTest;
 using chrome_browser_net::CertificateErrorReporter;
@@ skipping 188 matching lines @@
 
  private:
   const scoped_refptr<SafeBrowsingUIManager> safe_browsing_ui_manager_;
   bool reported_;
   bool expect_report_;
   base::Closure report_sent_callback_;
 };
 
 }  // namespace CertificateReporting
 
-void RootCertsChangedOnIOThread(
-    const scoped_refptr<net::URLRequestContextGetter> context_getter) {
-  net::CertDatabase::GetInstance()->NotifyObserversOfCACertChanged(NULL);
-  context_getter->GetURLRequestContext()
-      ->http_transaction_factory()
-      ->GetSession()
-      ->CloseAllConnections();
-}
-
-// Alerts the URLRequestContext for the given WebContents that a root
-// certificate has changed state or been removed. This, in turn, clears any
-// cached certificate validation in the cert verifier. This will also close all
-// connections in the socket pool of |contents|, so calls to this should be made
-// with care.
-void RootCertsChanged(WebContents* contents) {
-  scoped_refptr<net::URLRequestContextGetter> url_request_context =
-      contents->GetBrowserContext()->GetRequestContextForRenderProcess(
-          contents->GetRenderProcessHost()->GetID());
-  base::RunLoop run_loop;
-  content::BrowserThread::PostTaskAndReply(
-      content::BrowserThread::IO, FROM_HERE,
-      base::Bind(&RootCertsChangedOnIOThread, url_request_context),
-      run_loop.QuitClosure());
-  run_loop.Run();
-  base::RunLoop().RunUntilIdle();
-}
-
 }  // namespace
 
 class SSLUITest : public InProcessBrowserTest {
  public:
   SSLUITest()
       : https_server_(net::SpawnedTestServer::TYPE_HTTPS,
                       SSLOptions(SSLOptions::CERT_OK),
                       base::FilePath(kDocRoot)),
         https_server_expired_(net::SpawnedTestServer::TYPE_HTTPS,
                               SSLOptions(SSLOptions::CERT_EXPIRED),
@@ skipping 2029 matching lines @@
 
   browser()->tab_strip_model()->ActivateTabAt(1, true);
   EXPECT_TRUE(tab->GetRenderWidgetHostView()->IsShowing());
 }
 
 // Verifies that if a bad certificate is seen for a host and the user proceeds
 // through the interstitial, the decision to proceed is initially remembered.
 // However, if this is followed by another visit, and a good certificate
 // is seen for the same host, the original exception is forgotten.
 IN_PROC_BROWSER_TEST_F(SSLUITest, BadCertFollowedByGoodCert) {
+  // It is necessary to use |https_server_expired_| rather than
+  // |https_server_mismatched| because the former shares a host with
+  // |https_server_| and cert exceptions are per host.
+  ASSERT_TRUE(https_server_expired_.Start());
   ASSERT_TRUE(https_server_.Start());
+
+  std::string https_server_expired_host =
+      https_server_.GetURL("files/ssl/google.html").host();
   std::string https_server_host =
       https_server_.GetURL("files/ssl/google.html").host();
+  ASSERT_EQ(https_server_expired_host, https_server_host);

[Ryan Sleevi, 2015/04/24 11:03:40]

This isn't part of the contract of net::TestServer, so I'm uneasy about
depending on it.

I mean, it'll work, but it just adds to the 'hidden API' cost. David probably
has thouhts here.

[davidben, 2015/04/24 14:53:00]

I think this is fine. We'll notice if it breaks and "CERT_EXPIRED and CERT_OK
differ only the notAfter field" is a fine assumption to make, IMO.

 
   WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
-  net::TestRootCerts* root_certs = net::TestRootCerts::GetInstance();
-
-  ASSERT_TRUE(root_certs);
-  root_certs->Clear();
 
   Profile* profile = Profile::FromBrowserContext(tab->GetBrowserContext());
   ChromeSSLHostStateDelegate* state =
       reinterpret_cast<ChromeSSLHostStateDelegate*>(
           profile->GetSSLHostStateDelegate());
 
-  ui_test_utils::NavigateToURL(browser(),
-                               https_server_.GetURL("files/ssl/google.html"));
+  ui_test_utils::NavigateToURL(
+      browser(), https_server_expired_.GetURL("files/ssl/google.html"));
 
   ProceedThroughInterstitial(tab);
   EXPECT_TRUE(state->HasAllowException(https_server_host));
 
-  ASSERT_TRUE(https_server_.LoadTestRootCert());
-  RootCertsChanged(tab);
   ui_test_utils::NavigateToURL(browser(),
                                https_server_.GetURL("files/ssl/google.html"));
   ASSERT_FALSE(tab->GetInterstitialPage());
   EXPECT_FALSE(state->HasAllowException(https_server_host));
 }
 
 class SSLBlockingPageIDNTest : public SecurityInterstitialIDNTest {
  protected:
   // SecurityInterstitialIDNTest implementation
   SecurityInterstitialPage* CreateInterstitial(
@@ skipping 16 matching lines @@
 
 // Visit a page over https that contains a frame with a redirect.
 
 // XMLHttpRequest insecure content in synchronous mode.
 
 // XMLHttpRequest insecure content in asynchronous mode.
 
 // XMLHttpRequest over bad ssl in synchronous mode.
 
 // XMLHttpRequest over OK ssl in synchronous mode.