If ResourceDispatcherHost receives a CancelRequest, it

should call OnResponseComplete() only if the cancellation
is actually processed.  Otherwise, it might destroy a
request that is still active.

In addition, make sure DownloadRequestLimiter::Callback is
only invoked from the IO thread and prevent
DownloadThrottlingResourceHandler from processing
callbacks after the request has been closed.

BUG=76202
TEST=none

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

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

7 files changed, 70 insertions, 42 deletions

diff --git a/chrome/browser/download/download_request_limiter.cc b/chrome/browser/download/download_request_limiter.cc
index 0a828a1..579c172 100644
--- a/chrome/browser/download/download_request_limiter.cc
+++ b/chrome/browser/download/download_request_limiter.cc
@@ -190,6 +190,7 @@ DownloadRequestLimiter::DownloadStatus
 
 void DownloadRequestLimiter::CanDownloadOnIOThread(int render_process_host_id,
                                                    int render_view_id,
+                                                   int request_id,
                                                    Callback* callback) {
   // This is invoked on the IO thread. Schedule the task to run on the UI
   // thread so that we can query UI state.
@@ -197,7 +198,8 @@ void DownloadRequestLimiter::CanDownloadOnIOThread(int render_process_host_id,
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(this, &DownloadRequestLimiter::CanDownload,
-                        render_process_host_id, render_view_id, callback));
+                        render_process_host_id, render_view_id, request_id,
+                        callback));
 }
 
 void DownloadRequestLimiter::OnUserGesture(TabContents* tab) {
@@ -233,6 +235,7 @@ DownloadRequestLimiter::TabDownloadState* DownloadRequestLimiter::
 
 void DownloadRequestLimiter::CanDownload(int render_process_host_id,
                                          int render_view_id,
+                                         int request_id,
                                          Callback* callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -243,16 +246,17 @@ void DownloadRequestLimiter::CanDownload(int render_process_host_id,
     ScheduleNotification(callback, false);
     return;
   }
-  CanDownloadImpl(originating_tab, callback);
+  CanDownloadImpl(originating_tab, request_id, callback);
 }
 
 void DownloadRequestLimiter::CanDownloadImpl(
     TabContents* originating_tab,
+    int request_id,
     Callback* callback) {
   // FYI: Chrome Frame overrides CanDownload in ExternalTabContainer in order
   // to cancel the download operation in chrome and let the host browser
   // take care of it.
-  if (!originating_tab->CanDownload(callback->GetRequestId())) {
+  if (!originating_tab->CanDownload(request_id)) {
     ScheduleNotification(callback, false);
     return;
   }
diff --git a/chrome/browser/download/download_request_limiter.h b/chrome/browser/download/download_request_limiter.h
index 1caae9a..0281750 100644
--- a/chrome/browser/download/download_request_limiter.h
+++ b/chrome/browser/download/download_request_limiter.h
@@ -59,7 +59,6 @@ class DownloadRequestLimiter
    public:
     virtual void ContinueDownload() = 0;
     virtual void CancelDownload() = 0;
-    virtual int GetRequestId() = 0;
 
    protected:
     virtual ~Callback() {}
@@ -178,6 +177,7 @@ class DownloadRequestLimiter
   // the caller to ensure the callback is valid until the request is complete.
   void CanDownloadOnIOThread(int render_process_host_id,
                              int render_view_id,
+                             int request_id,
                              Callback* callback);
 
   // Invoked when the user presses the mouse, enter key or space bar. This may
@@ -218,11 +218,13 @@ class DownloadRequestLimiter
   // tab and invokes CanDownloadImpl.
   void CanDownload(int render_process_host_id,
                    int render_view_id,
+                   int request_id,
                    Callback* callback);
 
   // Does the work of updating the download status on the UI thread and
   // potentially prompting the user.
   void CanDownloadImpl(TabContents* originating_tab,
+                       int request_id,
                        Callback* callback);
 
   // Invoked on the UI thread. Schedules a call to NotifyCallback on the io
diff --git a/chrome/browser/download/download_request_limiter_unittest.cc b/chrome/browser/download/download_request_limiter_unittest.cc
index e74c7ee..d4422d1 100644
--- a/chrome/browser/download/download_request_limiter_unittest.cc
+++ b/chrome/browser/download/download_request_limiter_unittest.cc
@@ -40,13 +40,9 @@ class DownloadRequestLimiterTest
     cancel_count_++;
   }
 
-  virtual int GetRequestId() {
-    return -1;
-  }
-
   void CanDownload() {
     download_request_limiter_->CanDownloadImpl(
-        controller().tab_contents(), this);
+        controller().tab_contents(), -1, this);
     message_loop_.RunAllPending();
   }
 
diff --git a/chrome/browser/renderer_host/download_throttling_resource_handler.cc b/chrome/browser/renderer_host/download_throttling_resource_handler.cc
index d0648b1..b6c4998 100644
--- a/chrome/browser/renderer_host/download_throttling_resource_handler.cc
+++ b/chrome/browser/renderer_host/download_throttling_resource_handler.cc
@@ -27,12 +27,18 @@ DownloadThrottlingResourceHandler::DownloadThrottlingResourceHandler(
       render_view_id_(render_view_id),
       request_id_(request_id),
       tmp_buffer_length_(0),
-      ignore_on_read_complete_(in_complete) {
+      ignore_on_read_complete_(in_complete),
+      request_closed_(false) {
   // Pause the request.
   host_->PauseRequest(render_process_host_id_, request_id_, true);
-  host_->download_request_limiter()->CanDownloadOnIOThread(
-      render_process_host_id_, render_view_id, this);
 
+  // Add a reference to ourselves to keep this object alive until we
+  // receive a callback from DownloadRequestLimiter. The reference is
+  // released in ContinueDownload() and CancelDownload().
+  AddRef();
+
+  host_->download_request_limiter()->CanDownloadOnIOThread(
+      render_process_host_id_, render_view_id, request_id, this);
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableFunction(&download_util::NotifyDownloadInitiated,
@@ -45,6 +51,7 @@ DownloadThrottlingResourceHandler::~DownloadThrottlingResourceHandler() {
 bool DownloadThrottlingResourceHandler::OnUploadProgress(int request_id,
                                                          uint64 position,
                                                          uint64 size) {
+  DCHECK(!request_closed_);
   if (download_handler_.get())
     return download_handler_->OnUploadProgress(request_id, position, size);
   return true;
@@ -55,6 +62,7 @@ bool DownloadThrottlingResourceHandler::OnRequestRedirected(
     const GURL& url,
     ResourceResponse* response,
     bool* defer) {
+  DCHECK(!request_closed_);
   if (download_handler_.get()) {
     return download_handler_->OnRequestRedirected(
         request_id, url, response, defer);
@@ -66,6 +74,7 @@ bool DownloadThrottlingResourceHandler::OnRequestRedirected(
 bool DownloadThrottlingResourceHandler::OnResponseStarted(
     int request_id,
     ResourceResponse* response) {
+  DCHECK(!request_closed_);
   if (download_handler_.get())
     return download_handler_->OnResponseStarted(request_id, response);
   response_ = response;
@@ -75,6 +84,7 @@ bool DownloadThrottlingResourceHandler::OnResponseStarted(
 bool DownloadThrottlingResourceHandler::OnWillStart(int request_id,
                                                     const GURL& url,
                                                     bool* defer) {
+  DCHECK(!request_closed_);
   if (download_handler_.get())
     return download_handler_->OnWillStart(request_id, url, defer);
   return true;
@@ -84,6 +94,7 @@ bool DownloadThrottlingResourceHandler::OnWillRead(int request_id,
                                                    net::IOBuffer** buf,
                                                    int* buf_size,
                                                    int min_size) {
+  DCHECK(!request_closed_);
   if (download_handler_.get())
     return download_handler_->OnWillRead(request_id, buf, buf_size, min_size);
 
@@ -103,6 +114,7 @@ bool DownloadThrottlingResourceHandler::OnWillRead(int request_id,
 
 bool DownloadThrottlingResourceHandler::OnReadCompleted(int request_id,
                                                         int* bytes_read) {
+  DCHECK(!request_closed_);
   if (ignore_on_read_complete_) {
     // See comments above definition for details on this.
     ignore_on_read_complete_ = false;
@@ -127,6 +139,7 @@ bool DownloadThrottlingResourceHandler::OnResponseCompleted(
     int request_id,
     const net::URLRequestStatus& status,
     const std::string& security_info) {
+  DCHECK(!request_closed_);
   if (download_handler_.get())
     return download_handler_->OnResponseCompleted(request_id, status,
                                                   security_info);
@@ -142,38 +155,41 @@ bool DownloadThrottlingResourceHandler::OnResponseCompleted(
 }
 
 void DownloadThrottlingResourceHandler::OnRequestClosed() {
+  DCHECK(!request_closed_);
   if (download_handler_.get())
     download_handler_->OnRequestClosed();
+  request_closed_ = true;
 }
 
 void DownloadThrottlingResourceHandler::CancelDownload() {
-  host_->CancelRequest(render_process_host_id_, request_id_, false);
+  if (!request_closed_)
+    host_->CancelRequest(render_process_host_id_, request_id_, false);
+  Release();  // Release the additional reference from constructor.
 }
 
 void DownloadThrottlingResourceHandler::ContinueDownload() {
   DCHECK(!download_handler_.get());
-  download_handler_ =
-      new DownloadResourceHandler(host_,
-                                  render_process_host_id_,
-                                  render_view_id_,
-                                  request_id_,
-                                  url_,
-                                  host_->download_file_manager(),
-                                  request_,
-                                  false,
-                                  DownloadSaveInfo());
-  if (response_.get())
-    download_handler_->OnResponseStarted(request_id_, response_.get());
-
-  if (tmp_buffer_length_)
-    CopyTmpBufferToDownloadHandler();
-
-  // And let the request continue.
-  host_->PauseRequest(render_process_host_id_, request_id_, false);
-}
+  if (!request_closed_) {
+    download_handler_ =
+        new DownloadResourceHandler(host_,
+                                    render_process_host_id_,
+                                    render_view_id_,
+                                    request_id_,
+                                    url_,
+                                    host_->download_file_manager(),
+                                    request_,
+                                    false,
+                                    DownloadSaveInfo());
+    if (response_.get())
+      download_handler_->OnResponseStarted(request_id_, response_.get());
+
+    if (tmp_buffer_length_)
+      CopyTmpBufferToDownloadHandler();
 
-int DownloadThrottlingResourceHandler::GetRequestId() {
-  return request_id_;
+    // And let the request continue.
+    host_->PauseRequest(render_process_host_id_, request_id_, false);
+  }
+  Release();  // Release the addtional reference from constructor.
 }
 
 void DownloadThrottlingResourceHandler::CopyTmpBufferToDownloadHandler() {
diff --git a/chrome/browser/renderer_host/download_throttling_resource_handler.h b/chrome/browser/renderer_host/download_throttling_resource_handler.h
index 4e10c0d..1fab3eaf0 100644
--- a/chrome/browser/renderer_host/download_throttling_resource_handler.h
+++ b/chrome/browser/renderer_host/download_throttling_resource_handler.h
@@ -58,7 +58,6 @@ class DownloadThrottlingResourceHandler
   // DownloadRequestLimiter::Callback implementation:
   virtual void CancelDownload();
   virtual void ContinueDownload();
-  virtual int GetRequestId();
 
  private:
   virtual ~DownloadThrottlingResourceHandler();
@@ -91,6 +90,10 @@ class DownloadThrottlingResourceHandler
   // we ignore one of them.
   bool ignore_on_read_complete_;
 
+  // Have we received OnRequestClosed()?  If so, we shouldn't act on
+  // CancelDownload()/ContinueDownload().
+  bool request_closed_;
+
   DISALLOW_COPY_AND_ASSIGN(DownloadThrottlingResourceHandler);
 };
 
diff --git a/content/browser/renderer_host/resource_dispatcher_host.cc b/content/browser/renderer_host/resource_dispatcher_host.cc
index bd9f1ec..936d1d0 100644
--- a/content/browser/renderer_host/resource_dispatcher_host.cc
+++ b/content/browser/renderer_host/resource_dispatcher_host.cc
@@ -1170,14 +1170,17 @@ void ResourceDispatcherHost::CancelRequest(int child_id,
   }
   net::URLRequest* request = i->second;
   const bool started_before_cancel = request->is_pending();
-  CancelRequestInternal(request, from_renderer);
-  // If the request isn't in flight, then we won't get asyncronous notification,
-  // so we have to signal ourselves to finish this request.
-  if (!started_before_cancel)
+
+  if (CancelRequestInternal(request, from_renderer) &&
+      !started_before_cancel) {
+    // If the request isn't in flight, then we won't get asyncronous
+    // notification, so we have to signal ourselves to finish this
+    // request.
     OnResponseCompleted(request);
+  }
 }
 
-void ResourceDispatcherHost::CancelRequestInternal(net::URLRequest* request,
+bool ResourceDispatcherHost::CancelRequestInternal(net::URLRequest* request,
                                                    bool from_renderer) {
   VLOG(1) << "CancelRequest: " << request->url().spec();
 
@@ -1196,11 +1199,13 @@ void ResourceDispatcherHost::CancelRequestInternal(net::URLRequest* request,
     request->Cancel();
     // Our callers assume |request| is valid after we return.
     DCHECK(IsValidRequest(request));
+    return true;
   }
 
   // Do not remove from the pending requests, as the request will still
   // call AllDataReceived, and may even have more data before it does
   // that.
+  return false;
 }
 
 int ResourceDispatcherHost::IncrementOutstandingRequestsMemoryCost(
diff --git a/content/browser/renderer_host/resource_dispatcher_host.h b/content/browser/renderer_host/resource_dispatcher_host.h
index a177719..33757df 100644
--- a/content/browser/renderer_host/resource_dispatcher_host.h
+++ b/content/browser/renderer_host/resource_dispatcher_host.h
@@ -317,8 +317,10 @@ class ResourceDispatcherHost : public net::URLRequest::Delegate {
   // Helper function for regular and download requests.
   void BeginRequestInternal(net::URLRequest* request);
 
-  // Helper function that cancels |request|.
-  void CancelRequestInternal(net::URLRequest* request, bool from_renderer);
+  // Helper function that cancels |request|.  Returns whether the
+  // request was actually cancelled.  If a renderer cancels a request
+  // for a download, we ignore the cancellation.
+  bool CancelRequestInternal(net::URLRequest* request, bool from_renderer);
 
   // Helper function that inserts |request| into the resource queue.
   void InsertIntoResourceQueue(