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..c358d24c41c7aa60530049d0823cb62ab9715c7b 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,477 @@ 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 DelayableNetworkTimeURLRequestJob : public net::URLRequestJob {
+ public:
+  DelayableNetworkTimeURLRequestJob(net::URLRequest* request,
+                                    net::NetworkDelegate* network_delegate,
+                                    bool delayed)
+      : net::URLRequestJob(request, network_delegate),
+        delayed_(delayed),
+        weak_factory_(this) {}
+
+  ~DelayableNetworkTimeURLRequestJob() override {}
+
+  /// URLRequestJob:

[mmenke, 2016/11/07 15:27:45]

nit:  Remove extra slash.

[estark, 2016/11/07 23:24:00]

Done.

+  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_,

[mmenke, 2016/11/07 15:27:45]

Should include net/base/io_buffer.h

[estark, 2016/11/07 23:24:00]

Done.

+           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(

[mmenke, 2016/11/07 15:27:45]

Terminology is confusing here, but the term "raw headers" is actually used to
refer to the format output by AssembleRawHeaders, not the data passed in to it.

[estark, 2016/11/07 23:24:00]

Done.

+        "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_)

[mmenke, 2016/11/07 15:27:45]

Not need to check this - the request owns the URLRequestJob (Way back when, the
URLRequestJob was refcounted, but that's since been fixed).

[estark, 2016/11/07 23:24:00]

Done.

+      return;
+    NotifyHeadersComplete();
+  }
+
+  // Resumes delayed requests. When the response info for a request is
+  // read (i.e. GetResponseInfo() is called), then |quit_closure| will be
+  // called.

[mmenke, 2016/11/07 15:27:45]

Putting this in GetResponseInfo isn't right - there's no guarantee on the number
of times this method will be called, when it will be called, or if it will be
called at all.

[estark, 2016/11/07 23:24:00]

I removed this callback. (See question above -- not sure what, if anything, to
do instead.)

+  void Resume(const base::Closure& quit_closure) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    response_callback_ = quit_closure;
+    // Start reading asynchronously as would a normal network request.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&DelayableNetworkTimeURLRequestJob::StartAsync,
+                              weak_factory_.GetWeakPtr()));

[mmenke, 2016/11/07 15:27:45]

Should handle jobs that haven't yet had Start() called on them yet when Resume()
is called.  Currently Start() is always called right after job creation, I
believe, but there's no API contract about that.

[mmenke, 2016/11/07 15:27:45]

You're using weak_ptrs here, but DelayedNetworkTimeInterceptor does not remove
requests from its list if they are destroyed before ResumeAll is called.  Should
this code handle premature request destruction or not?

[estark, 2016/11/07 23:24:00]

Done. If Resume() is called before Start(), then we now unset |delayed_| so that
when Start() is called, it'll start the request immediately.

[estark, 2016/11/07 23:24:00]

I think this should handle premature request destruction, so I changed
DelayedNetworkTimeInterceptor to use a WeakPtr to its request.

+  }
+
+ private:
+  bool delayed_;
+  int data_offset_ = 0;
+  base::Closure response_callback_;
+  base::WeakPtrFactory<DelayableNetworkTimeURLRequestJob> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(DelayableNetworkTimeURLRequestJob);
+};
+
+// A URLRequestInterceptor that intercepts requests to use
+// DelayableNetworkTimeURLRequestJobs.
+class DelayedNetworkTimeInterceptor : public net::URLRequestInterceptor {
+ public:
+  DelayedNetworkTimeInterceptor() : net::URLRequestInterceptor() {}

[mmenke, 2016/11/07 15:27:45]

net::URLRequestInterceptor() not needed.

[estark, 2016/11/07 23:24:00]

Done.

+  ~DelayedNetworkTimeInterceptor() override {}
+
+  net::URLRequestJob* MaybeInterceptRequest(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    // If requests have been resumed before this request is created,
+    // then |delay_requests_| will be false and the request will not delay.
+    DelayableNetworkTimeURLRequestJob* job =
+        new DelayableNetworkTimeURLRequestJob(request, network_delegate,
+                                              delay_requests_);
+    if (delay_requests_) {
+      delayed_requests_.push_back(job);
+    }

[mmenke, 2016/11/07 15:27:45]

nit:  Remove braces for two-line ifs.

[estark, 2016/11/07 23:24:00]

Done.

+    return job;
+  }
+
+  void ResumeAll(const base::Closure& quit_closure) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    if (!delay_requests_) {
+      return;
+    }

[mmenke, 2016/11/07 15:27:45]

nit:  Remove braces for two-line ifs.

[estark, 2016/11/07 23:24:00]

Done.

+    delay_requests_ = false;
+    for (const auto& request : delayed_requests_) {
+      request->Resume(quit_closure);

[mmenke, 2016/11/07 15:27:45]

If no requests have been started yet, quit closure will never be called.  If
there are multiple requests, they'll all call quit closure, so just the first
one will be guaranteed to reach the quit point before the message loop is quit.

My suggestion is to either only support one delayed request, or if you need/want
more, be sure to get all cases correct.

[estark, 2016/11/07 23:24:00]

Done; I changed the interceptor to expect only one delayed request. (And now
that the callback is being called on NetworkDelegate::OnCompleted, I think this
works properly if Resume() is called before a request is intercepted. In this
case, the Resume() call sets the callback on the network delegate, which will
get called if a request gets intercepted later.)

[mmenke, 2016/11/08 15:24:21]

I don't see any waiting on NetworkDelegate::OnCompleted here, am I missing
something?  You could just wait until the URLRequestJob is destroyed.

[estark, 2016/11/08 17:54:01]

Oh no, I'm sorry, I uploaded this in a weird intermediate state and must have
forgotten to look it over before I sent mail. References to NetworkDelegate are
removed now (I was experimenting with that for a while but it didn't work out).

I could call a callback when the URLRequestJob is destroyed, but I don't think I
actually need this request-completion callback at all. See my comments at the
start of https://codereview.chromium.org/2449193002/#msg68. The only purpose of
the callback is to ensure that nothing crashes in OnURLFetchComplete for the
URLFetcher that initiated these requests... but I don't think I need to wait on
anything for that, because OnURLFetchComplete will (I think) get run when the
test is torn down if it hasn't been run already.

+    }
+    delayed_requests_.clear();
+  }
+
+ private:
+  bool delay_requests_ = true;
+  mutable std::vector<DelayableNetworkTimeURLRequestJob*> delayed_requests_;
+
+  DISALLOW_COPY_AND_ASSIGN(DelayedNetworkTimeInterceptor);
+};
+
+// Helper method for SSLNetworkTimeBrowserTest.
+void CleanUpOnIOThread() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  net::URLRequestFilter::GetInstance()->ClearHandlers();
+}
+
+// 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()),
+        interceptor_(nullptr) {}
+  ~SSLNetworkTimeBrowserTest() override {}
+
+  void TearDownOnMainThread() override {
+    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+                                     base::Bind(&CleanUpOnIOThread));
+  }
+
+ 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);
+
+    // Install the URL interceptor that serves delayed network time
+    // responses.
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(
+            &SSLNetworkTimeBrowserTest::SetUpNetworkTimeInterceptorOnIOThread,
+            base::Unretained(this), g_browser_process->network_time_tracker()
+                                        ->GetTimeServerURLForTesting()));
+  }
+
+  void TriggerTimeResponse(const base::Closure& quit_closure) {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&SSLNetworkTimeBrowserTest::ResumeDelayedNetworkTimeRequests,
+                   base::Unretained(this), quit_closure));
+  }
+
+ private:
+  void SetUpNetworkTimeInterceptorOnIOThread(const GURL& time_server_url) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    interceptor_ = new DelayedNetworkTimeInterceptor();

[mmenke, 2016/11/07 15:27:45]

This works.  My own personal style preference would be to make interceptor_ on
the UI thread, keep a raw pointer to it in the fixture, and then make these
methods static (i.e., no non-static member methods of the class fixture called
on the IO thread), but that's a matter of personal preference, not correctness,
or officially preferred style.

[estark, 2016/11/07 17:17:19]

If I were to do that, would I pass a WeakPtr to the interceptor when calling the
static methods? Or is it okay to pass a base::Unretained pointer because the
interceptor will stay alive until ClearAllHandlers is called?

[mmenke, 2016/11/07 17:21:41]

It would be an unretained pointer.  The interceptor is guaranteed to stay alive
long enough.

A weak pointer would be nifty, but you can only dereference them on the thread
you created them on, so you'd need to create a WeakPtr on the IO thread.  You
could do that, though then you'd need a PostTaskAndReplyWithResult or something
to create the interceptor.  Maybe there's a DetachFromThread method for weak
pointers or something that I'm not remembering, though.

[estark, 2016/11/07 23:24:00]

Done.

+    net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
+        time_server_url.scheme(), time_server_url.host(),
+        std::unique_ptr<DelayedNetworkTimeInterceptor>(interceptor_));
+  }
+
+  void ResumeDelayedNetworkTimeRequests(const base::Closure& quit_closure) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    interceptor_->ResumeAll(quit_closure);
+  }
+
+  std::unique_ptr<network_time::FieldTrialTest> field_trial_test_;
+
+  // Should only be accessed on the IO thread.
+  DelayedNetworkTimeInterceptor* interceptor_;
+
+  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::kGoodTimeResponseHandlerJsTime));
+  // 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_time_response;
+  TriggerTimeResponse(wait_for_time_response.QuitClosure());
+
+  EXPECT_TRUE(contents->IsLoading());
+  observer.Wait();
+  content::WaitForInterstitialAttach(contents);
+  wait_for_time_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::kGoodTimeResponseHandlerJsTime));
+  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_time_response;
+  TriggerTimeResponse(wait_for_time_response.QuitClosure());
+
+  EXPECT_TRUE(contents->IsLoading());
+  observer.Wait();
+  content::WaitForInterstitialAttach(contents);
+  wait_for_time_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 and wait
+  // for the response; no crash should occur.
+  ASSERT_TRUE(https_server_.Start());
+  ui_test_utils::NavigateToURL(browser(), https_server_.GetURL("/"));
+  base::RunLoop wait_for_time_response;
+  TriggerTimeResponse(wait_for_time_response.QuitClosure());
+  wait_for_time_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 and wait
+  // for the response; no crash should occur.
+  ASSERT_TRUE(https_server_.Start());
+  ui_test_utils::NavigateToURL(browser(), https_server_.GetURL("/title1.html"));
+  base::RunLoop wait_for_time_response;
+  TriggerTimeResponse(wait_for_time_response.QuitClosure());
+  wait_for_time_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 and wait
+  // for the response; no crash should occur.
+  ASSERT_TRUE(https_server_.Start());
+  ui_test_utils::NavigateToURL(browser(), https_server_.GetURL("/"));
+  base::RunLoop wait_for_time_response;
+  TriggerTimeResponse(wait_for_time_response.QuitClosure());
+  wait_for_time_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 and wait
+  // for the response; no crash should occur.
+  ui_test_utils::NavigateToURL(browser(), https_server_.GetURL("/"));
+  base::RunLoop wait_for_time_response;
+  TriggerTimeResponse(wait_for_time_response.QuitClosure());
+  wait_for_time_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 and wait for the response; no crash should occur.
+  ASSERT_TRUE(https_server_.Start());
+  AddTabAtIndex(1, https_server_.GetURL("/"), ui::PAGE_TRANSITION_TYPED);
+  chrome::CloseWebContents(browser(), contents, false);
+  base::RunLoop wait_for_time_response;
+  TriggerTimeResponse(wait_for_time_response.QuitClosure());
+  wait_for_time_response.Run();
+}
+
 class CommonNameMismatchBrowserTest : public CertVerifierBrowserTest {
  public:
   CommonNameMismatchBrowserTest() : CertVerifierBrowserTest() {}