Fix improper cancellation when URLFetcher has started throttling requests.

BUG=32844
TEST=URLFetcherCancelTest.CancelWhileDelayedStartTaskPending

Review URL: http://codereview.chromium.org/552107

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@36901 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 63 insertions, 34 deletions

diff --git a/chrome/browser/net/url_fetcher.cc b/chrome/browser/net/url_fetcher.cc
index dcfc2bf..14539a2 100644
--- a/chrome/browser/net/url_fetcher.cc
+++ b/chrome/browser/net/url_fetcher.cc
@@ -96,6 +96,9 @@ class URLFetcher::Core
   // specified by the protection manager, we'll give up.
   int num_retries_;
 
+  // True if the URLFetcher has been cancelled.
+  bool was_cancelled_;
+
   friend class URLFetcher;
   DISALLOW_COPY_AND_ASSIGN(Core);
 };
@@ -136,7 +139,8 @@ URLFetcher::Core::Core(URLFetcher* fetcher,
       buffer_(new net::IOBuffer(kBufferSize)),
       protect_entry_(URLFetcherProtectManager::GetInstance()->Register(
           original_url_.host())),
-      num_retries_(0) {
+      num_retries_(0),
+      was_cancelled_(false) {
 }
 
 void URLFetcher::Core::Start() {
@@ -200,6 +204,13 @@ void URLFetcher::Core::OnReadCompleted(URLRequest* request, int bytes_read) {
 
 void URLFetcher::Core::StartURLRequest() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
+
+  if (was_cancelled_) {
+    // Since StartURLRequest() is posted as a *delayed* task, it may
+    // run after the URLFetcher was already stopped.
+    return;
+  }
+
   CHECK(request_context_getter_);
   DCHECK(!request_);
 
@@ -254,6 +265,7 @@ void URLFetcher::Core::CancelURLRequest() {
   // delete the object, but we cannot delay the destruction of the request
   // context.
   request_context_getter_ = NULL;
+  was_cancelled_ = true;
 }
 
 void URLFetcher::Core::OnCompletedURLRequest(const URLRequestStatus& status) {
diff --git a/chrome/browser/net/url_fetcher_unittest.cc b/chrome/browser/net/url_fetcher_unittest.cc
index a9141e9..d0aa795 100644
--- a/chrome/browser/net/url_fetcher_unittest.cc
+++ b/chrome/browser/net/url_fetcher_unittest.cc
@@ -141,39 +141,25 @@ class URLFetcherCancelTest : public URLFetcherTest {
                                   const std::string& data);
 
   void CancelRequest();
-  void TestContextReleased();
 
  private:
   base::OneShotTimer<URLFetcherCancelTest> timer_;
-  bool context_released_;
 };
 
-// Version of TestURLRequestContext that let us know if the request context
-// is properly released.
+// Version of TestURLRequestContext that posts a Quit task to the IO
+// thread once it is deleted.
 class CancelTestURLRequestContext : public TestURLRequestContext {
- public:
-  explicit CancelTestURLRequestContext(bool* destructor_called)
-      : destructor_called_(destructor_called) {
-    *destructor_called_ = false;
-  }
-
- private:
   virtual ~CancelTestURLRequestContext() {
-    *destructor_called_ = true;
+    ChromeThread::PostTask(
+        ChromeThread::IO, FROM_HERE, new MessageLoop::QuitTask());
   }
-
-  bool* destructor_called_;
 };
 
 class CancelTestURLRequestContextGetter : public URLRequestContextGetter {
  public:
-  explicit CancelTestURLRequestContextGetter(bool* destructor_called)
-      : destructor_called_(destructor_called) {
-  }
-
   virtual URLRequestContext* GetURLRequestContext() {
     if (!context_)
-      context_ = new CancelTestURLRequestContext(destructor_called_);
+      context_ = new CancelTestURLRequestContext();
     return context_;
   }
 
@@ -181,7 +167,6 @@ class CancelTestURLRequestContextGetter : public URLRequestContextGetter {
   ~CancelTestURLRequestContextGetter() {}
 
   scoped_refptr<URLRequestContext> context_;
-  bool* destructor_called_;
 };
 
 // Wrapper that lets us call CreateFetcher() on a thread of our choice.  We
@@ -195,7 +180,7 @@ class FetcherWrapperTask : public Task {
       : test_(test), url_(url) { }
   virtual void Run() {
     test_->CreateFetcher(url_);
-  };
+  }
 
  private:
   URLFetcherTest* test_;
@@ -336,8 +321,12 @@ void URLFetcherBadHTTPSTest::OnURLFetchComplete(
 
 void URLFetcherCancelTest::CreateFetcher(const GURL& url) {
   fetcher_ = new URLFetcher(url, URLFetcher::GET, this);
-  fetcher_->set_request_context(
-      new CancelTestURLRequestContextGetter(&context_released_));
+  // We need to force the creation of the URLRequestContext, since we
+  // rely on it being destroyed as a signal to end the test.
+  URLRequestContextGetter* context_getter =
+      new CancelTestURLRequestContextGetter();
+  context_getter->GetURLRequestContext();
+  fetcher_->set_request_context(context_getter);
   fetcher_->Start();
   // Make sure we give the IO thread a chance to run.
   timer_.Start(TimeDelta::FromMilliseconds(300), this,
@@ -360,16 +349,9 @@ void URLFetcherCancelTest::OnURLFetchComplete(const URLFetcher* source,
 void URLFetcherCancelTest::CancelRequest() {
   delete fetcher_;
   timer_.Stop();
-  // Make sure we give the IO thread a chance to run.
-  timer_.Start(TimeDelta::FromMilliseconds(300), this,
-               &URLFetcherCancelTest::TestContextReleased);
-}
-
-void URLFetcherCancelTest::TestContextReleased() {
-  EXPECT_TRUE(context_released_);
-  timer_.Stop();
-  ChromeThread::PostTask(
-      ChromeThread::IO, FROM_HERE, new MessageLoop::QuitTask());
+  // The URLFetcher's test context will post a Quit task once it is
+  // deleted. So if this test simply hangs, it means cancellation
+  // did not work.
 }
 
 TEST_F(URLFetcherTest, SameThreadsTest) {
@@ -500,4 +482,39 @@ TEST_F(URLFetcherCancelTest, ReleasesContext) {
   MessageLoop::current()->Run();
 }
 
+TEST_F(URLFetcherCancelTest, CancelWhileDelayedStartTaskPending) {
+  scoped_refptr<HTTPTestServer> server =
+      HTTPTestServer::CreateServer(L"chrome/test/data", NULL);
+  ASSERT_TRUE(NULL != server.get());
+  GURL url = GURL(server->TestServerPage("files/server-unavailable.html"));
+
+  // Register an entry for test url.
+  //
+  // Ideally we would mock URLFetcherProtectEntry to return XXX seconds
+  // in response to entry->UpdateBackoff(SEND).
+  //
+  // Unfortunately this function is time sensitive, so we fudge some numbers
+  // to make it at least somewhat likely to have a non-zero deferred
+  // delay when running.
+  //
+  // Using a sliding window of 2 seconds, and max of 1 request, under a fast
+  // run we expect to have a 4 second delay when posting the Start task.
+  URLFetcherProtectManager* manager = URLFetcherProtectManager::GetInstance();
+  URLFetcherProtectEntry* entry =
+      new URLFetcherProtectEntry(2000, 1, 2, 2000, 2.0, 0, 4000);
+  EXPECT_EQ(0, entry->UpdateBackoff(URLFetcherProtectEntry::SEND));
+  entry->UpdateBackoff(URLFetcherProtectEntry::SEND);  // Returns about 2000.
+  manager->Register(url.host(), entry);
+
+  // The next request we try to send will be delayed by ~4 seconds.
+  // The slower the test runs, the less the delay will be (since it takes the
+  // time difference from now).
+
+  base::Thread t("URLFetcher test thread");
+  ASSERT_TRUE(t.Start());
+  t.message_loop()->PostTask(FROM_HERE, new FetcherWrapperTask(this, url));
+
+  MessageLoop::current()->Run();
+}
+
 }  // namespace.