Attempt an on-demand time fetch when encountering a date invalid error

chrome/browser/ssl/ssl_browser_tests.cc

Index: chrome/browser/ssl/ssl_browser_tests.cc
diff --git a/chrome/browser/ssl/ssl_browser_tests.cc b/chrome/browser/ssl/ssl_browser_tests.cc
index d8f3a8f5d574adf6e028103b2bcd36c15c9544ea..ec5c947dbf6987172176abc78daed73b77349063 100644
--- a/chrome/browser/ssl/ssl_browser_tests.cc
+++ b/chrome/browser/ssl/ssl_browser_tests.cc
@@ -20,6 +20,8 @@
 #include "base/test/histogram_tester.h"
 #include "base/test/simple_test_clock.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/default_clock.h"
+#include "base/time/default_tick_clock.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
@@ -50,8 +52,9 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/network_time/network_time_test_utils.h"
 #include "components/network_time/network_time_tracker.h"
-#include "components/prefs/pref_service.h"
+#include "components/prefs/testing_pref_service.h"
 #include "components/security_interstitials/core/controller_client.h"
 #include "components/security_interstitials/core/metrics_helper.h"
 #include "components/security_state/security_state_model.h"
@@ -86,6 +89,7 @@
 #include "net/cert/mock_cert_verifier.h"
 #include "net/cert/x509_certificate.h"
 #include "net/dns/mock_host_resolver.h"
+#include "net/http/http_response_headers.h"
 #include "net/ssl/ssl_info.h"
 #include "net/test/cert_test_util.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -2853,6 +2857,485 @@ IN_PROC_BROWSER_TEST_F(SSLUITest,
       after_interstitial_ssl_status.Equals(clock_interstitial_ssl_status));
 }
 
+// A URLRequestJob that serves valid time server responses, but delays
+// them until Resume() is called. If Resume() is called before a request
+// is made, then the request will not be delayed.
+class DelayableTimeURLRequestJob : public net::URLRequestJob {

[meacer, 2016/11/02 22:43:22]

nit: rename to Delayable*Network*TimeURLRequestJob?

[estark, 2016/11/03 01:36:04]

Done.

+ public:
+  DelayableTimeURLRequestJob(net::URLRequest* request,
+                             net::NetworkDelegate* network_delegate,
+                             bool delayed)
+      : net::URLRequestJob(request, network_delegate),
+        delayed_(delayed),
+        weak_factory_(this) {}
+
+  ~DelayableTimeURLRequestJob() override {}
+
+  /// URLRequestJob:
+  void Start() override {
+    if (delayed_) {
+      // Do nothing until Resume() is called.
+      return;
+    }
+    Resume(response_callback_);
+  }
+
+  int ReadRawData(net::IOBuffer* buf, int buf_size) override {
+    int bytes_read =
+        std::min(static_cast<size_t>(buf_size),
+                 strlen(network_time::kGoodTimeResponseBody) - data_offset_);
+    memcpy(buf->data(), network_time::kGoodTimeResponseBody + data_offset_,
+           bytes_read);
+    data_offset_ += bytes_read;
+    return bytes_read;
+  }
+
+  int GetResponseCode() const override { return 200; }
+
+  void GetResponseInfo(net::HttpResponseInfo* info) override {
+    std::string raw_headers;
+    raw_headers.append(
+        "HTTP/1.1 200 OK\n"
+        "Content-type: text/plain\n");
+    raw_headers.append(base::StringPrintf(
+        "Content-Length: %1d\n",
+        static_cast<int>(strlen(network_time::kGoodTimeResponseBody))));
+    info->headers =
+        new net::HttpResponseHeaders(net::HttpUtil::AssembleRawHeaders(
+            raw_headers.c_str(), static_cast<int>(raw_headers.length())));
+    info->headers->AddHeader(
+        "x-cup-server-proof: " +
+        std::string(network_time::kGoodTimeResponseServerProofHeader));
+    if (!response_callback_.is_null()) {
+      content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                       response_callback_);
+    }
+  }
+
+  void StartAsync() {
+    if (!request_)
+      return;
+    NotifyHeadersComplete();
+  }
+
+  // Resumes delayed requests. When the response info for a request is
+  // read (i.e. GetResponseInfo() is called), then |callback| will be
+  // called.
+  void Resume(const base::Closure& callback) {

[meacer, 2016/11/02 22:43:23]

nit: dcheck for io thread?

[estark, 2016/11/03 01:36:04]

Done.

+    response_callback_ = callback;
+    // Start reading asynchronously as would a normal network request.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&DelayableTimeURLRequestJob::StartAsync,
+                              weak_factory_.GetWeakPtr()));
+  }
+
+ private:
+  bool delayed_;
+  int data_offset_ = 0;
+  base::Closure response_callback_;
+  base::WeakPtrFactory<DelayableTimeURLRequestJob> weak_factory_;
+};
+
+// A URLRequestInterceptor that intercepts requests to use
+// DelayableTimeURLRequestJobs.
+class DelayedTimeInterceptor : public net::URLRequestInterceptor {

[meacer, 2016/11/02 22:43:22]

nit: rename to DelayedNetworkTimeInterceptor?

[estark, 2016/11/03 01:36:05]

Done.

+ public:
+  DelayedTimeInterceptor() : net::URLRequestInterceptor() {}
+  ~DelayedTimeInterceptor() override {}
+
+  net::URLRequestJob* MaybeInterceptRequest(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    // If requests have been resumed before this request is created,
+    // then |resumed_| will be true and the request will not delay.

[meacer, 2016/11/02 22:43:22]

nit: Slightly confusing :) How about a "bool delayed = !resumed_;" line? 

Or perhaps you could rename resumed_ to delay_requests_? I think I like that
better.

[estark, 2016/11/03 01:36:05]

Done.

+    DelayableTimeURLRequestJob* job =
+        new DelayableTimeURLRequestJob(request, network_delegate, !resumed_);
+    delayed_requests_.push_back(job);
+    return job;
+  }
+
+  void ResumeAll(const base::Closure& callback) {

[meacer, 2016/11/02 22:43:22]

nit: Could use a DCHECK for IO thread here.

[estark, 2016/11/03 01:36:04]

Done.

+    if (resumed_) {
+      return;
+    }
+    resumed_ = true;
+    for (const auto& request : delayed_requests_) {
+      request->Resume(callback);
+    }
+    delayed_requests_.clear();
+  }
+
+ private:
+  bool resumed_ = false;
+  mutable std::vector<DelayableTimeURLRequestJob*> delayed_requests_;
+};
+
+// A helper class for tasks that SSLNetworkTimeBrowserTest needs to
+// perform on the IO thread. Installs a DelayedTimeInterceptor and
+// resumes delayed requests when ResumeDelayedNetworkTimeRequests() is
+// called.
+class SSLNetworkTimeIOThreadHelper {

[meacer, 2016/11/02 22:43:22]

nit: I think you could move the functionality in this class directly to
SSLNetworkTimeBrowserTest. It feels slightly better since the reader will need
to keep track of one fewer file. Also, one fewer indirection.

If you do that, you'll want to make the interceptor supply a weak pointer
though.

[meacer, 2016/11/02 22:44:38]

And by "one fewer file" I meant "one fewer class".

[estark, 2016/11/03 01:36:04]

Done. What do you mean by "make the interceptor supply a weak pointer" though? I
called these methods with a weak pointer to the test fixture, is that what you
mean?

[meacer, 2016/11/04 01:17:26]

Never mind that, you're not posting any tasks in the interceptor so this is
fine. You are doing that in DelayableNetworkTimeURLRequestJob which already has
a weak ptr factory.

But I don't think the weak ptr in the test fixture is necessary.

+ public:
+  SSLNetworkTimeIOThreadHelper() {}
+  virtual ~SSLNetworkTimeIOThreadHelper() {}
+
+  void SetUpOnIOThread(const GURL& intercept_url) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    interceptor_ = new DelayedTimeInterceptor();
+    net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
+        intercept_url.scheme(), intercept_url.host(),
+        std::unique_ptr<net::URLRequestInterceptor>(interceptor_));
+  }
+
+  void CleanUpOnIOThread() {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    net::URLRequestFilter::GetInstance()->ClearHandlers();
+  }
+
+  void ResumeDelayedNetworkTimeRequests(const base::Closure& callback) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    interceptor_->ResumeAll(callback);
+  }
+
+ private:
+  DelayedTimeInterceptor* interceptor_;
+};
+
+// A fixture for testing on-demand network time queries on SSL
+// certificate date errors. It can simulate delayed network time
+// requests, and it allows the user to configure the experimental
+// parameters of the NetworkTimeTracker.
+class SSLNetworkTimeBrowserTest : public SSLUITest {
+ public:
+  SSLNetworkTimeBrowserTest()
+      : SSLUITest(),
+        field_trial_test_(network_time::FieldTrialTest::CreateForBrowserTest()),
+        io_thread_helper_(new SSLNetworkTimeIOThreadHelper()) {}
+  ~SSLNetworkTimeBrowserTest() override {}
+
+  void TearDownOnMainThread() override {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&SSLNetworkTimeIOThreadHelper::CleanUpOnIOThread,
+                   base::Unretained(io_thread_helper_.get())));
+  }
+
+ protected:
+  network_time::FieldTrialTest* field_trial_test() const {
+    return field_trial_test_.get();
+  }
+
+  void SetUpNetworkTimeServer() {
+    field_trial_test()->SetNetworkQueriesWithVariationsService(
+        true, 0.0, network_time::FieldTrialTest::FETCHES_ON_DEMAND_ONLY);
+
+    ASSERT_TRUE(embedded_test_server()->Start());
+    // Install the URL interceptor that serves delayed network time
+    // responses.
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&SSLNetworkTimeIOThreadHelper::SetUpOnIOThread,
+                   base::Unretained(io_thread_helper_.get()),
+                   embedded_test_server()->GetURL("/")));

[meacer, 2016/11/02 22:43:22]

Since actual requests won't go through, there shouldn't be a need to use the
embedded server here. You can use "http://time.test" or some such.

And another option: You can change SetTimeServerURLForTesting to
GetTimeServerURLForTesting, and pass that to the interceptor. I feel this is
slightly better, since it doesn't change any state in the network time tracker.

[estark, 2016/11/03 01:36:04]

Done.

+
+    g_browser_process->network_time_tracker()->SetTimeServerURLForTesting(
+        embedded_test_server()->GetURL("/"));
+  }
+
+  void TriggerTimeResponse(const base::Closure& callback) {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(
+            &SSLNetworkTimeIOThreadHelper::ResumeDelayedNetworkTimeRequests,
+            base::Unretained(io_thread_helper_.get()), callback));
+  }
+
+ private:
+  std::unique_ptr<network_time::FieldTrialTest> field_trial_test_;
+  std::unique_ptr<SSLNetworkTimeIOThreadHelper> io_thread_helper_;
+
+  DISALLOW_COPY_AND_ASSIGN(SSLNetworkTimeBrowserTest);
+};
+
+// Tests that if an on-demand network time fetch returns that the clock
+// is okay, a normal SSL interstitial is shown.
+IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest, OnDemandFetchClockOk) {
+  ASSERT_TRUE(https_server_expired_.Start());
+  SetUpNetworkTimeServer();
+  // Use a testing clock set to the time that GoodTimeResponseHandler
+  // returns, to simulate the system clock matching the network time.
+  base::SimpleTestClock testing_clock;
+  SSLErrorHandler::SetClockForTest(&testing_clock);
+  testing_clock.SetNow(
+      base::Time::FromJsTime(network_time::kGoodTimeResponseHandlerTime));
+  // Set the build time to match the testing clock, to ensure that the
+  // build time heuristic doesn't fire.
+  ssl_errors::SetBuildTimeForTesting(testing_clock.Now());
+
+  // Set a long timeout to ensure that the on-demand time fetch completes.
+  SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta::FromHours(1));
+
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(contents);
+  SSLInterstitialTimerObserver interstitial_timer_observer(contents);
+
+  content::WindowedNotificationObserver observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::NotificationService::AllSources());
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), https_server_expired_.GetURL("/"),
+      WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE);
+
+  // Once |interstitial_timer_observer| has fired, the request has been
+  // sent. Override the nonce that NetworkTimeTracker expects so that
+  // when the response comes back, it will validate. The nonce can only
+  // be overriden for the current in-flight request, so the test must
+  // call OverrideNonceForTesting() after the request has been sent and
+  // before the response has been received.
+  interstitial_timer_observer.WaitForTimerStarted();
+  g_browser_process->network_time_tracker()->OverrideNonceForTesting(123123123);
+  base::RunLoop wait_for_response;

[meacer, 2016/11/02 22:43:22]

nit: rename wait_for_time_response? (otherwise it reads like we are waiting for
the interstitial response)

[estark, 2016/11/03 01:36:05]

Done.

+  TriggerTimeResponse(wait_for_response.QuitClosure());
+
+  EXPECT_TRUE(contents->IsLoading());
+  observer.Wait();
+  content::WaitForInterstitialAttach(contents);
+  wait_for_response.Run();
+
+  EXPECT_TRUE(contents->ShowingInterstitialPage());
+  InterstitialPage* interstitial_page = contents->GetInterstitialPage();
+  ASSERT_TRUE(interstitial_page);
+  ASSERT_EQ(SSLBlockingPage::kTypeForTesting,
+            interstitial_page->GetDelegateForTesting()->GetTypeForTesting());
+}
+
+// Tests that if an on-demand network time fetch returns that the clock
+// is wrong, a bad clock interstitial is shown.
+IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest, OnDemandFetchClockWrong) {
+  ASSERT_TRUE(https_server_expired_.Start());
+  SetUpNetworkTimeServer();
+  // Use a testing clock set to a time that is different from what
+  // GoodTimeResponseHandler returns, simulating a system clock that is
+  // 30 days ahead of the network time.
+  base::SimpleTestClock testing_clock;
+  SSLErrorHandler::SetClockForTest(&testing_clock);
+  testing_clock.SetNow(
+      base::Time::FromJsTime(network_time::kGoodTimeResponseHandlerTime));
+  testing_clock.Advance(base::TimeDelta::FromDays(30));
+  // Set the build time to match the testing clock, to ensure that the
+  // build time heuristic doesn't fire.
+  ssl_errors::SetBuildTimeForTesting(testing_clock.Now());
+
+  // Set a long timeout to ensure that the on-demand time fetch completes.
+  SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta::FromHours(1));
+
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(contents);
+  SSLInterstitialTimerObserver interstitial_timer_observer(contents);
+
+  content::WindowedNotificationObserver observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::NotificationService::AllSources());
+
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), https_server_expired_.GetURL("/"),
+      WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE);
+
+  // Once |interstitial_timer_observer| has fired, the request has been
+  // sent. Override the nonce that NetworkTimeTracker expects so that
+  // when the response comes back, it will validate. The nonce can only
+  // be overriden for the current in-flight request, so the test must
+  // call OverrideNonceForTesting() after the request has been sent and
+  // before the response has been received.
+  interstitial_timer_observer.WaitForTimerStarted();
+  g_browser_process->network_time_tracker()->OverrideNonceForTesting(123123123);
+  base::RunLoop wait_for_response;
+  TriggerTimeResponse(wait_for_response.QuitClosure());
+
+  EXPECT_TRUE(contents->IsLoading());
+  observer.Wait();
+  content::WaitForInterstitialAttach(contents);
+  wait_for_response.Run();
+
+  EXPECT_TRUE(contents->ShowingInterstitialPage());
+  InterstitialPage* interstitial_page = contents->GetInterstitialPage();
+  ASSERT_TRUE(interstitial_page);
+  ASSERT_EQ(BadClockBlockingPage::kTypeForTesting,
+            interstitial_page->GetDelegateForTesting()->GetTypeForTesting());
+}
+
+// Tests that if the timeout expires before the network time fetch
+// returns, then a normal SSL intersitial is shown.
+IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest,
+                       TimeoutExpiresBeforeFetchCompletes) {
+  ASSERT_TRUE(https_server_expired_.Start());
+  SetUpNetworkTimeServer();
+  // Set the timer to fire immediately.
+  SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta());
+
+  ui_test_utils::NavigateToURL(browser(), https_server_expired_.GetURL("/"));
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(contents);
+  content::WaitForInterstitialAttach(contents);
+
+  EXPECT_TRUE(contents->ShowingInterstitialPage());
+  InterstitialPage* interstitial_page = contents->GetInterstitialPage();
+  ASSERT_TRUE(interstitial_page);
+  ASSERT_EQ(SSLBlockingPage::kTypeForTesting,
+            interstitial_page->GetDelegateForTesting()->GetTypeForTesting());
+
+  // Navigate away and then trigger the network time response; no crash
+  // should occur.
+  ASSERT_TRUE(https_server_.Start());
+  ui_test_utils::NavigateToURL(browser(), https_server_.GetURL("/"));
+  base::RunLoop wait_for_response;
+  TriggerTimeResponse(wait_for_response.QuitClosure());
+  wait_for_response.Run();
+}
+
+// Tests that if the user stops the page load before either the network
+// time fetch completes or the timeout expires, then there is no interstitial.
+IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest, StopBeforeTimeoutExpires) {
+  ASSERT_TRUE(https_server_expired_.Start());
+  SetUpNetworkTimeServer();
+  // Set the timer to a long delay.
+  SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta::FromHours(1));
+
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(contents);
+  SSLInterstitialTimerObserver interstitial_timer_observer(contents);
+
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), https_server_expired_.GetURL("/"),
+      WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE);
+  interstitial_timer_observer.WaitForTimerStarted();
+
+  EXPECT_TRUE(contents->IsLoading());
+  content::WindowedNotificationObserver observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::NotificationService::AllSources());
+  contents->Stop();
+  observer.Wait();
+
+  // Make sure that the |SSLErrorHandler| is deleted.
+  EXPECT_FALSE(SSLErrorHandler::FromWebContents(contents));
+  EXPECT_FALSE(contents->ShowingInterstitialPage());
+  EXPECT_FALSE(contents->IsLoading());
+
+  // Navigate away and then trigger the network time response; no crash
+  // should occur.
+  ASSERT_TRUE(https_server_.Start());
+  ui_test_utils::NavigateToURL(browser(), https_server_.GetURL("/title1.html"));
+  base::RunLoop wait_for_response;
+  TriggerTimeResponse(wait_for_response.QuitClosure());
+  wait_for_response.Run();
+}
+
+// Tests that if the user reloads the page before either the network
+// time fetch completes or the timeout expires, then there is no interstitial.
+IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest, ReloadBeforeTimeoutExpires) {
+  ASSERT_TRUE(https_server_expired_.Start());
+  SetUpNetworkTimeServer();
+  // Set the timer to a long delay.
+  SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta::FromHours(1));
+
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  SSLInterstitialTimerObserver interstitial_timer_observer(contents);
+
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), https_server_expired_.GetURL("/"),
+      WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE);
+  interstitial_timer_observer.WaitForTimerStarted();
+
+  EXPECT_TRUE(contents->IsLoading());
+  content::TestNavigationObserver observer(contents, 1);
+  chrome::Reload(browser(), WindowOpenDisposition::CURRENT_TAB);
+  observer.Wait();
+
+  // Make sure that the |SSLErrorHandler| is deleted.
+  EXPECT_FALSE(SSLErrorHandler::FromWebContents(contents));
+  EXPECT_FALSE(contents->ShowingInterstitialPage());
+  EXPECT_FALSE(contents->IsLoading());
+
+  // Navigate away and then trigger the network time response; no crash
+  // should occur.
+  ASSERT_TRUE(https_server_.Start());
+  ui_test_utils::NavigateToURL(browser(), https_server_.GetURL("/"));
+  base::RunLoop wait_for_response;
+  TriggerTimeResponse(wait_for_response.QuitClosure());

[meacer, 2016/11/02 22:43:23]

What happens if there were more than one delayed network time requests?

[estark, 2016/11/03 01:36:05]

This would only wait for the first one. Subsequent requests would call
QuitClosure() again which according to the documentation should have no effect.

I think all the tests only expect one request, so maybe I should just assert
that there is only one request somewhere in the interceptor or in the
URLRequestJob. WDYT?

+  wait_for_response.Run();
+}
+
+// Tests that if the user navigates away before either the network time
+// fetch completes or the timeout expires, then there is no
+// interstitial.
+IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest,
+                       NavigateAwayBeforeTimeoutExpires) {
+  ASSERT_TRUE(https_server_expired_.Start());
+  ASSERT_TRUE(https_server_.Start());
+  SetUpNetworkTimeServer();
+  // Set the timer to a long delay.
+  SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta::FromHours(1));
+
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  SSLInterstitialTimerObserver interstitial_timer_observer(contents);
+
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), https_server_expired_.GetURL("/"),
+      WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE);
+  interstitial_timer_observer.WaitForTimerStarted();
+
+  EXPECT_TRUE(contents->IsLoading());
+  content::TestNavigationObserver observer(contents, 1);
+  browser()->OpenURL(content::OpenURLParams(
+      https_server_.GetURL("/"), content::Referrer(),
+      WindowOpenDisposition::CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false));
+  observer.Wait();
+
+  // Make sure that the |SSLErrorHandler| is deleted.
+  EXPECT_FALSE(SSLErrorHandler::FromWebContents(contents));
+  EXPECT_FALSE(contents->ShowingInterstitialPage());
+  EXPECT_FALSE(contents->IsLoading());
+
+  // Navigate away and then trigger the network time response; no crash
+  // should occur.

[meacer, 2016/11/02 22:43:23]

nit: 
"Navigate away, then trigger the network time response and wait for the
response; no crash should occur."

[estark, 2016/11/03 01:36:04]

Done.

+  ui_test_utils::NavigateToURL(browser(), https_server_.GetURL("/"));
+  base::RunLoop wait_for_response;
+  TriggerTimeResponse(wait_for_response.QuitClosure());
+  wait_for_response.Run();
+}
+
+// Tests that if the user closes the tab before the network time fetch
+// completes, it doesn't cause a crash.
+IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest,
+                       CloseTabBeforeNetworkFetchCompletes) {
+  ASSERT_TRUE(https_server_expired_.Start());
+  SetUpNetworkTimeServer();
+  // Set the timer to fire immediately.
+  SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta());
+
+  ui_test_utils::NavigateToURL(browser(), https_server_expired_.GetURL("/"));
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(contents);
+  content::WaitForInterstitialAttach(contents);
+
+  EXPECT_TRUE(contents->ShowingInterstitialPage());
+  InterstitialPage* interstitial_page = contents->GetInterstitialPage();
+  ASSERT_TRUE(interstitial_page);
+  ASSERT_EQ(SSLBlockingPage::kTypeForTesting,
+            interstitial_page->GetDelegateForTesting()->GetTypeForTesting());
+
+  // Open a second tab, close the first, and then trigger the network time
+  // response; no crash
+  // should occur.

[meacer, 2016/11/02 22:43:22]

nit: Fits into previous line

[estark, 2016/11/03 01:36:05]

Done.

+  ASSERT_TRUE(https_server_.Start());
+  AddTabAtIndex(1, https_server_.GetURL("/"), ui::PAGE_TRANSITION_TYPED);
+  chrome::CloseWebContents(browser(), contents, false);
+  base::RunLoop wait_for_response;
+  TriggerTimeResponse(wait_for_response.QuitClosure());
+  wait_for_response.Run();
+}
+
 class CommonNameMismatchBrowserTest : public CertVerifierBrowserTest {
  public:
   CommonNameMismatchBrowserTest() : CertVerifierBrowserTest() {}