Fix a crash caused by a race condition between the time the UrlmonUrlRequest::EndRequestInternal task

executing and the OnStopBinding function getting called by Urlmon. If our task executes first we essentially
operate on an invalid object.

Fixes http://code.google.com/p/chromium/issues/detail?id=29033

Added a comment to the EndRequest function indicating that it is invalid to access any members after it
is called. Added a helper function to release the bindings and revoke the bind status callback.

Bug=29033

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

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

2 files changed, 17 insertions, 10 deletions

diff --git a/chrome_frame/urlmon_url_request.cc b/chrome_frame/urlmon_url_request.cc
index e738ae1..b2588ab 100644
--- a/chrome_frame/urlmon_url_request.cc
+++ b/chrome_frame/urlmon_url_request.cc
@@ -315,16 +315,7 @@ STDMETHODIMP UrlmonUrlRequest::OnStopBinding(HRESULT result, LPCWSTR error) {
   } else {
     status_.set_status(URLRequestStatus::SUCCESS);
     status_.set_os_error(0);
-  }
-
-  DLOG(INFO) << "OnStopBinding received for request id: " << id();
-
-  // Release these variables after reporting EndRequest since we might need to
-  // access their state.
-  binding_.Release();
-  if (bind_context_) {
-    ::RevokeBindStatusCallback(bind_context_, this);
-    bind_context_.Release();
+    ReleaseBindings();
   }
 
   return S_OK;
@@ -724,8 +715,11 @@ void UrlmonUrlRequest::EndRequest() {
     ignore_redirect_stop_binding_error_ = false;
   }
 
+  ReleaseBindings();
   // Remove the request mapping and release the outstanding reference to us in
   // the context of the UI thread.
+  // We should not access any members of the UrlmonUrlRequest object after this
+  // as the object would be deleted.
   PostTask(FROM_HERE,
            NewRunnableMethod(this, &UrlmonUrlRequest::EndRequestInternal));
 }
@@ -803,6 +797,14 @@ std::string UrlmonUrlRequest::GetHttpHeaders() const {
   return buffer.get();
 }
 
+void UrlmonUrlRequest::ReleaseBindings() {
+  binding_.Release();
+  if (bind_context_) {
+    ::RevokeBindStatusCallback(bind_context_, this);
+    bind_context_.Release();
+  }
+}
+
 //
 // UrlmonUrlRequest::Cache implementation.
 //
diff --git a/chrome_frame/urlmon_url_request.h b/chrome_frame/urlmon_url_request.h
index e52d08d..fc98e8f 100644
--- a/chrome_frame/urlmon_url_request.h
+++ b/chrome_frame/urlmon_url_request.h
@@ -119,6 +119,7 @@ END_MSG_MAP()
   void StartAsync();
   void StopAsync();
   void ReadAsync(int bytes_to_read);
+  void ReleaseBindings();
 
   static const size_t kCopyChunkSize = 32 * 1024;
   // URL requests are handled on this thread.
@@ -236,6 +237,10 @@ END_MSG_MAP()
   };
 
   HRESULT StartAsyncDownload();
+  // Sends over the response end notification to chrome, releases the bindings
+  // and releases the initial reference on the UrlmonUrlRequest object.
+  // After this function is called we should not attempt to access any members
+  // as the object could become invalid at any point.
   void EndRequest();
   // Executes in the context of the UI thread and releases the outstanding
   // reference to us. It also deletes the request mapping for this instance.