Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(50)

Issue 9453028: Fix ExpiredCertAndGoBackTests (Closed)

Created:
8 years, 10 months ago by tpayne
Modified:
8 years, 9 months ago
Reviewers:
darin (slow to review), wtc, tpayne
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, vandebo (ex-Chrome), Takashi Toyoshima
Visibility:
Public.

Description

Fix ExpiredCertAndGoBackTests. According to Darin, it doesn't make sense to call CancelRequest if it's not pending. Checking is_pending before calling CancelRequest fixes the flakiness (passed on 1,000 tries) BUG=40932 TEST=NONE Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=123834

Patch Set 1 #

Patch Set 2 : Remove old comment #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -4 lines) Patch
M chrome/browser/ssl/ssl_browser_tests.cc View 1 2 chunks +2 lines, -3 lines 1 comment Download
M content/browser/ssl/ssl_error_handler.cc View 1 chunk +1 line, -1 line 6 comments Download

Messages

Total messages: 14 (0 generated)
tpayne
This fixes the ExpiredCertAndGoBackViaXXXTests. Tests passed 1000 times and all browser_tests still pass.
8 years, 10 months ago (2012-02-27 18:36:38 UTC) #1
darin (slow to review)
http://codereview.chromium.org/9453028/diff/5001/content/browser/ssl/ssl_error_handler.cc File content/browser/ssl/ssl_error_handler.cc (right): http://codereview.chromium.org/9453028/diff/5001/content/browser/ssl/ssl_error_handler.cc#newcode138 content/browser/ssl/ssl_error_handler.cc:138: if (request && request->is_pending()) { oh, hmm... have you ...
8 years, 10 months ago (2012-02-27 19:26:24 UTC) #2
tpayne
http://codereview.chromium.org/9453028/diff/5001/content/browser/ssl/ssl_error_handler.cc File content/browser/ssl/ssl_error_handler.cc (right): http://codereview.chromium.org/9453028/diff/5001/content/browser/ssl/ssl_error_handler.cc#newcode138 content/browser/ssl/ssl_error_handler.cc:138: if (request && request->is_pending()) { On 2012/02/27 19:26:25, darin ...
8 years, 10 months ago (2012-02-27 19:30:50 UTC) #3
darin (slow to review)
I see. In that case, this is a reasonable approach. If you had a way ...
8 years, 10 months ago (2012-02-27 19:33:37 UTC) #4
tpayne
On 2012/02/27 19:33:37, darin wrote: > I see. In that case, this is a reasonable ...
8 years, 10 months ago (2012-02-27 19:35:09 UTC) #5
darin (slow to review)
I'm not sure that I would add a member to URLRequest for this. Since these ...
8 years, 10 months ago (2012-02-27 19:39:38 UTC) #6
tpayne_google.com
I'm pretty sure this is a real bug. The test code isn't doing anything interesting ...
8 years, 10 months ago (2012-02-27 19:42:02 UTC) #7
darin (slow to review)
Sorry, I got confused. I thought for a second I was looking at test-only code ...
8 years, 10 months ago (2012-02-27 21:07:23 UTC) #8
wtc
Patch Set 2 LGTM. toyoshim: please note the change to content/browser/ssl/ssl_error_handler.cc. Don't lose it in ...
8 years, 9 months ago (2012-02-28 20:02:12 UTC) #9
darin (slow to review)
On Tue, Feb 28, 2012 at 12:02 PM, <wtc@chromium.org> wrote: > Patch Set 2 LGTM. ...
8 years, 9 months ago (2012-02-28 21:21:12 UTC) #10
tpayne
http://codereview.chromium.org/9453028/diff/5001/content/browser/ssl/ssl_error_handler.cc File content/browser/ssl/ssl_error_handler.cc (right): http://codereview.chromium.org/9453028/diff/5001/content/browser/ssl/ssl_error_handler.cc#newcode138 content/browser/ssl/ssl_error_handler.cc:138: if (request && request->is_pending()) { On 2012/02/28 20:02:12, wtc ...
8 years, 9 months ago (2012-02-28 21:53:58 UTC) #11
wtc
On 2012/02/28 21:21:12, darin wrote: > > I think checking is_pending_ makes sense generically, but ...
8 years, 9 months ago (2012-02-28 22:22:40 UTC) #12
tpayne
http://codereview.chromium.org/9453028/diff/5001/content/browser/ssl/ssl_error_handler.cc File content/browser/ssl/ssl_error_handler.cc (right): http://codereview.chromium.org/9453028/diff/5001/content/browser/ssl/ssl_error_handler.cc#newcode138 content/browser/ssl/ssl_error_handler.cc:138: if (request && request->is_pending()) { On 2012/02/28 21:53:58, tpayne ...
8 years, 9 months ago (2012-02-29 01:15:11 UTC) #13
tpayne
8 years, 9 months ago (2012-02-29 01:58:23 UTC) #14
http://codereview.chromium.org/9453028/diff/5001/content/browser/ssl/ssl_erro...
File content/browser/ssl/ssl_error_handler.cc (right):

http://codereview.chromium.org/9453028/diff/5001/content/browser/ssl/ssl_erro...
content/browser/ssl/ssl_error_handler.cc:138: if (request &&
request->is_pending()) {
On 2012/02/29 01:15:11, tpayne wrote:
> On 2012/02/28 21:53:58, tpayne wrote:
> > On 2012/02/28 20:02:12, wtc wrote:
> > > 
> > > On 2012/02/27 19:30:51, tpayne wrote:
> > > >
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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

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.

Powered by Google App Engine
This is Rietveld 408576698