|
|
Created:
8 years, 10 months ago by tpayne Modified:
8 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, vandebo (ex-Chrome), Takashi Toyoshima Visibility:
Public. |
DescriptionFix 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
Messages
Total messages: 14 (0 generated)
This fixes the ExpiredCertAndGoBackViaXXXTests. Tests passed 1000 times and all browser_tests still pass.
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()) { 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?
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/27 19:26:25, darin wrote: > 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? 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.
I see. In that case, this is a reasonable approach. If you had a way to assert that, it might be nice.
On 2012/02/27 19:33:37, darin wrote: > I see. In that case, this is a reasonable approach. If you had a way to assert > that, it might be nice. To assert that it's already been started? I could add a was_started_ member variable to UrlRequest. Should I?
I'm not sure that I would add a member to URLRequest for this. Since these requests are all going through the ResourceDispatcherHost, you could add a field to ResourceDispatcherHostRequestInfo. Ideally, though, you would do this in test-only code so that we don't need to bloat Chrome for something that only a test needs. -Darin On Mon, Feb 27, 2012 at 11:35 AM, <tpayne@chromium.org> wrote: > On 2012/02/27 19:33:37, darin wrote: > >> I see. In that case, this is a reasonable approach. If you had a way to >> > assert > >> that, it might be nice. >> > > To assert that it's already been started? I could add a was_started_ member > variable to UrlRequest. Should I? > > http://codereview.chromium.**org/9453028/<http://codereview.chromium.org/9453... >
I'm pretty sure this is a real bug. The test code isn't doing anything interesting here. On Mon, Feb 27, 2012 at 11:39 AM, Darin Fisher <darin@chromium.org> wrote: > I'm not sure that I would add a member to URLRequest for this. Since these > requests are all going through the ResourceDispatcherHost, you could add a > field to ResourceDispatcherHostRequestInfo. Ideally, though, you would do > this in test-only code so that we don't need to bloat Chrome for something > that only a test needs. > > -Darin > > > On Mon, Feb 27, 2012 at 11:35 AM, <tpayne@chromium.org> wrote: > >> On 2012/02/27 19:33:37, darin wrote: >> >>> I see. In that case, this is a reasonable approach. If you had a way to >>> >> assert >> >>> that, it might be nice. >>> >> >> To assert that it's already been started? I could add a was_started_ >> member >> variable to UrlRequest. Should I? >> >> http://codereview.chromium.**org/9453028/<http://codereview.chromium.org/9453... >> > >
Sorry, I got confused. I thought for a second I was looking at test-only code :-/ This patch looks reasonable to me. I noticed that we can only reach this code after URLRequest::Start has been called. LGTM -Darin On Mon, Feb 27, 2012 at 11:41 AM, Tony Payne <tpayne@google.com> wrote: > I'm pretty sure this is a real bug. The test code isn't doing anything > interesting here. > > > On Mon, Feb 27, 2012 at 11:39 AM, Darin Fisher <darin@chromium.org> wrote: > >> I'm not sure that I would add a member to URLRequest for this. Since >> these >> requests are all going through the ResourceDispatcherHost, you could add a >> field to ResourceDispatcherHostRequestInfo. Ideally, though, you would do >> this in test-only code so that we don't need to bloat Chrome for something >> that only a test needs. >> >> -Darin >> >> >> On Mon, Feb 27, 2012 at 11:35 AM, <tpayne@chromium.org> wrote: >> >>> On 2012/02/27 19:33:37, darin wrote: >>> >>>> I see. In that case, this is a reasonable approach. If you had a way >>>> to >>>> >>> assert >>> >>>> that, it might be nice. >>>> >>> >>> To assert that it's already been started? I could add a was_started_ >>> member >>> variable to UrlRequest. Should I? >>> >>> http://codereview.chromium.**org/9453028/<http://codereview.chromium.org/9453... >>> >> >> >
Patch Set 2 LGTM. toyoshim: please note the change to content/browser/ssl/ssl_error_handler.cc. Don't lose it in your CL https://chromiumcodereview.appspot.com/9406001/. (You need to make the same change to content/browser/renderer_host/resource_dispatcher_host.cc in your CL.) tpayne,darin: I don't fully understand why it doesn't make sense to call CancelRequest if it's not pending, but this NOTREACHED assertion seems to support that: void URLRequest::SimulateSSLError(int error, const SSLInfo& ssl_info) { // This should only be called on a started request. if (!is_pending_ || !job_ || job_->has_response_started()) { NOTREACHED(); return; } DoCancel(error, ssl_info); } Should we move that if statement to URLRequest::DoCancel, which is called by all three methodss of canceling a URLRequest? http://codereview.chromium.org/9453028/diff/5001/chrome/browser/ssl/ssl_brows... File chrome/browser/ssl/ssl_browser_tests.cc (right): http://codereview.chromium.org/9453028/diff/5001/chrome/browser/ssl/ssl_brows... chrome/browser/ssl/ssl_browser_tests.cc:401: // Disabled because its flaky: http://crbug.com/40932, http://crbug.com/43575. This comment should be deleted. 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/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.
On Tue, Feb 28, 2012 at 12:02 PM, <wtc@chromium.org> wrote: > Patch Set 2 LGTM. > > toyoshim: please note the change to > content/browser/ssl/ssl_error_**handler.cc. Don't lose it in > your CL https://chromiumcodereview.**appspot.com/9406001/<https://chromiumcodereview.... > . > (You need to make the same change to > content/browser/renderer_host/**resource_dispatcher_host.cc > in your CL.) > > tpayne,darin: I don't fully understand why it doesn't make > sense to call CancelRequest if it's not pending, but this > NOTREACHED assertion seems to support that: > > void URLRequest::SimulateSSLError(**int error, const SSLInfo& ssl_info) { > // This should only be called on a started request. > if (!is_pending_ || !job_ || job_->has_response_started()) { > NOTREACHED(); > return; > } > DoCancel(error, ssl_info); > } > > Should we move that if statement to URLRequest::DoCancel, > which is called by all three methodss of canceling a > URLRequest? > I think checking is_pending_ makes sense generically, but the has_response_started() portion of the check only makes sense for SimulateSSLError. The point is that we shouldn't try to simulate an SSL error when we are not in the state where an SSL error could occur. > > > http://codereview.chromium.**org/9453028/diff/5001/chrome/** > browser/ssl/ssl_browser_tests.**cc<http://codereview.chromium.org/9453028/diff/5001/chrome/browser/ssl/ssl_browser_tests.cc> > File chrome/browser/ssl/ssl_**browser_tests.cc (right): > > http://codereview.chromium.**org/9453028/diff/5001/chrome/** > browser/ssl/ssl_browser_tests.**cc#newcode401<http://codereview.chromium.org/9453028/diff/5001/chrome/browser/ssl/ssl_browser_tests.cc#newcode401> > chrome/browser/ssl/ssl_**browser_tests.cc:401: // Disabled because its > > flaky: http://crbug.com/40932, http://crbug.com/43575. > > This comment should be deleted. > > > http://codereview.chromium.**org/9453028/diff/5001/content/** > browser/ssl/ssl_error_handler.**cc<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<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: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. > > Yes, I concluded the same after studying how this code may be reached. -Darin > http://codereview.chromium.**org/9453028/<http://codereview.chromium.org/9453... >
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/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.
On 2012/02/28 21:21:12, darin wrote: > > I think checking is_pending_ makes sense generically, but the > has_response_started() > portion of the check only makes sense for SimulateSSLError. The point is > that we > shouldn't try to simulate an SSL error when we are not in the state where > an SSL error > could occur. darin: thank you for the reply. I looked into the callers of URLRequest::SimulateError(). I found that ResourceDispatcherHost may call it on a URLRequest that hasn't been started, so I think even the is_pending_ test is inappropriate for URLRequest::SimulateError(). Here is the ResourceDispatcherHost code: void ResourceDispatcherHost::BeginRequestInternal(net::URLRequest* request) { ... // If enqueing/starting this request will exceed our per-process memory // bound, abort it right away. if (memory_cost > max_outstanding_requests_cost_per_process_) { // We call "SimulateError()" as a way of setting the net::URLRequest's // status -- it has no effect beyond this, since the request hasn't started. request->SimulateError(net::ERR_INSUFFICIENT_RESOURCES); // TODO(eroman): this is kinda funky -- we insert the unstarted request into // |pending_requests_| simply to please ResponseCompleted(). GlobalRequestID global_id(info->child_id(), info->request_id()); pending_requests_[global_id] = request; ResponseCompleted(request); return; } ...
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/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
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. |