Fix ExpiredCertAndGoBackTests

content/browser/ssl/ssl_error_handler.cc

 // Copyright (c) 2011 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 "content/browser/ssl/ssl_error_handler.h"
 
 #include "base/bind.h"
 #include "content/browser/renderer_host/render_view_host.h"
 #include "content/browser/renderer_host/resource_dispatcher_host.h"
 #include "content/browser/renderer_host/resource_dispatcher_host_request_info.h"
@@ skipping 117 matching lines @@
 
   // It is important that we notify the net::URLRequest only once.  If we try
   // to notify the request twice, it may no longer exist and |this| might have
   // already have been deleted.
   DCHECK(!request_has_been_notified_);
   if (request_has_been_notified_)
     return;
 
   net::URLRequest* request =
       resource_dispatcher_host_->GetURLRequest(request_id_);
-  if (request) {
+  if (request && request->is_pending()) {

[darin (slow to review), 2012/02/27 19:26:25]

oh, hmm...

have you considered cancelling the request via
ResourceDispatcherHost::CancelRequest?  requests start out their life possibly
being deferred until some time.  they are not-yet-pending during this time.

if you don't cancel the request here, then the ResourceDispatcherHost may still
start the request later.  does that matter to you?

[tpayne, 2012/02/27 19:30:51]

I haven't seen any instances where the request had not yet started. All the
instances I've looked at (not that many, granted) were cases where the request
had already been notified.

[wtc, 2012/02/28 20:02:12]

tpayne: do you know why the request may be not pending at
this point?  Because the request has been canceled by
someone else in the interim?

darin: I believe the request must have been started if it
has an SSL error.  So it is impossible for the request to
be not-yet-pending here.

[tpayne, 2012/02/28 21:53:58]

The state that is causing the flakiness is request is no-longer-pending. I
wasn't able to determine what is canceling the request before we get here. I'll
do a bit more digging and see if I can come up with a culprit.

[tpayne, 2012/02/29 01:15:11]

The original cancel is occurring with this stack:

	net::URLRequest::NotifyRequestCompleted() [0x18f4bf7]
	net::URLRequest::DoCancel() [0x18f300e]
	net::URLRequest::Cancel() [0x18f2c4b]
	ResourceDispatcherHost::CancelRequestInternal() [0x3c48f1e]
	ResourceDispatcherHost::CancelRequest() [0x3c48c89]
	ResourceDispatcherHost::OnCancelRequest() [0x3c44ed5]
	DispatchToMethod<>() [0x3c51c75]
	ResourceHostMsg_CancelRequest::Dispatch<>() [0x3c4f8b4]
	ResourceDispatcherHost::OnMessageReceived() [0x3c43787]
	ResourceMessageFilter::OnMessageReceived() [0x3c5c563]
	content::BrowserMessageFilter::DispatchMessage() [0x3cc785b]
	content::BrowserMessageFilter::OnMessageReceived() [0x3cc76f8]
	IPC::ChannelProxy::Context::TryFilters() [0x25f85b3]
	IPC::ChannelProxy::Context::OnMessageReceived() [0x25f8637]
	IPC::Channel::ChannelImpl::ProcessIncomingMessages() [0x25f49b9]
	IPC::Channel::ChannelImpl::OnFileCanReadWithoutBlocking() [0x25f6881]
	base::MessagePumpLibevent::FileDescriptorWatcher::OnFileCanReadWithoutBlocking()
[0x1673a2c]
	base::MessagePumpLibevent::OnLibeventNotification() [0x16759ff]
	event_process_active [0x1e438d9]
	event_base_loop [0x1e43c08]
	base::MessagePumpLibevent::Run() [0x1674fa6]
	MessageLoop::RunInternal() [0x16b8ce3]
	MessageLoop::RunHandler() [0x16b8b96]
	MessageLoop::Run() [0x16b84cb]
	base::Thread::Run() [0x17088b2]
	base::Thread::ThreadMain() [0x1708a32]
	base::(anonymous namespace)::ThreadFunc() [0x16fefb3]
	start_thread [0x7fc82dfbb9ca]
	0x7fc82804a70d


The (flakily) failing attempt to cancel is occuring with this stack:


	SSLErrorHandler::CancelRequest() [0x3d83b10]
	SSLPolicy::OnAllowCertificate() [0x3c7ae77]
	base::internal::RunnableAdapter<>::Run() [0x3c7b76c]
	base::internal::InvokeHelper<>::MakeItSo() [0x3c7b674]
	base::internal::Invoker<>::Run() [0x3c7b5be]
	base::Callback<>::Run() [0xbab004]
	SSLBlockingPage::NotifyDenyCertificate() [0x43e0d75]
	SSLBlockingPage::OnDontProceed() [0x43e0d38]
	InterstitialPageImpl::DontProceed() [0x3c7e31d]
	NavigationControllerImpl::NavigateToPendingEntry() [0x3c85ff7]
	NavigationControllerImpl::GoBack() [0x3c82ab9]
	Browser::GoBack() [0xe3b0a4]
	SSLUITest_DISABLED_TestHTTPSExpiredCertAndGoBackViaButton_Test::RunTestOnMainThread()
[0x66d80e]
	InProcessBrowserTest::RunTestOnMainThreadLoop() [0x165a8d8]
	BrowserTestBase::ProxyRunTestOnMainThreadLoop() [0x436906d]
	base::internal::RunnableAdapter<>::Run() [0x4369385]
	base::internal::InvokeHelper<>::MakeItSo() [0x436931b]
	base::internal::Invoker<>::Run() [0x43692d3]
	base::Callback<>::Run() [0x94286b]
	ChromeBrowserMainParts::PreMainMessageLoopRunImpl() [0x43fa142]
	ChromeBrowserMainParts::PreMainMessageLoopRun() [0x43f8f96]
	content::BrowserMainLoop::CreateThreads() [0x3b0839e]
	(anonymous namespace)::BrowserMainRunnerImpl::Initialize() [0x3ccd93f]
	BrowserMain() [0x4a3e21f]
	BrowserTestBase::SetUp() [0x4369020]
	InProcessBrowserTest::SetUp() [0x1659fc1]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x1b3f09a]
	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x1b3c71e]
	testing::Test::Run() [0x1b31b01]
	testing::TestInfo::Run() [0x1b3237a]
	testing::TestCase::Run() [0x1b32a70]
	testing::internal::UnitTestImpl::RunAllTests() [0x1b37847]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x1b3fec1]
	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x1b3cecd]
	testing::UnitTest::Run() [0x1b36364]
	base::TestSuite::Run() [0x1737df9]
	ChromeTestLauncherDelegate::RunTestSuite() [0x7ef257]
	test_launcher::LaunchTests() [0x862f68]
	main [0x7ef05d]
	0x7fc827f82c4d
	0x43c539

[tpayne, 2012/02/29 01:58:24]

One more level up, it turns out that NavigateToPendingEntry has 2 paths that
cause the request to be cancelled:

TabContents::Stop triggers NotifyRequestCompleted and
InterstitialPageImpl::DontProceed triggers SSLErrorHandler::CancelRequest. If
NotifyRequestCompleted happens first, we get the failure due to is_pending_
getting cleared. Hope that helps.

     // The request can be NULL if it was cancelled by the renderer (as the
     // result of the user navigating to a new page from the location bar).
     DVLOG(1) << "CompleteCancelRequest() url: " << request->url().spec();
     SSLCertErrorHandler* cert_error = AsSSLCertErrorHandler();
     if (cert_error)
       request->SimulateSSLError(error, cert_error->ssl_info());
     else
       request->SimulateError(error);
   }
   request_has_been_notified_ = true;
@@ skipping 34 matching lines @@
   // already have been deleted.
   DCHECK(!request_has_been_notified_);
   if (request_has_been_notified_)
     return;
 
   request_has_been_notified_ = true;
 
   // We're done with this object on the IO thread.
   Release();
 }