This CL fixes the following issues:-

1. http://code.google.com/p/chromium/issues/detail?id=27200
   This was a crash which would occur in the Stop function in the UrlmonUrlRequest object
   which would get invoked when the active document instance was being destroyed. The crash
   occured if the active document instance was reused in which case we end up reusing the
   automation client instance. The fix is to ensure that we clean up existing requests from
   the map before reusing the automation client instance.

2. http://code.google.com/p/chromium/issues/detail?id=27202
   This was a DCHECK which would occur when adding a new request to the request map, which
   indicated that an existing request with the same request id existed in the map. This would
   occur during a redirect where the request id is reused by Chrome. Fix is to remove the 
   request from the map when we handle the AutomationMsg_RequestEnd message in the UI thread
   itself. The UrlmonUrlRequest functions which attempt to remove the request from the map
   now check if the request is valid before doing this.

Bug=27200,27202

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

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

2 files changed, 33 insertions, 10 deletions

diff --git a/chrome_frame/chrome_frame_automation.cc b/chrome_frame/chrome_frame_automation.cc
index 54e114d..1a4b763 100644
--- a/chrome_frame/chrome_frame_automation.cc
+++ b/chrome_frame/chrome_frame_automation.cc
@@ -1018,8 +1018,13 @@ bool ChromeFrameAutomationClient::ReadRequest(
 
 void ChromeFrameAutomationClient::RemoveRequest(PluginUrlRequest* request) {
   DCHECK_EQ(PlatformThread::CurrentId(), ui_thread_id_);
-  DCHECK(request_map_.end() != request_map_.find(request->id()));
-  request_map_.erase(request->id());
+
+  // We check if the request pointer passed in is actually present in the map
+  // before going ahead and deleting it. This avoids the issue where we would
+  // incorrectly delete a different request with the same request id.
+  if (IsValidRequest(request)) {
+    request_map_.erase(request->id());
+  }
 }
 
 void ChromeFrameAutomationClient::RemoveRequest(int request_id, bool abort) {
@@ -1081,17 +1086,23 @@ void ChromeFrameAutomationClient::CleanupRequests() {
 
 void ChromeFrameAutomationClient::CleanupAsyncRequests() {
   DCHECK_EQ(PlatformThread::CurrentId(), ui_thread_id_);
+
+  std::vector<scoped_refptr<PluginUrlRequest> > request_list;
+  // We copy the pending requests into a temporary vector as the Stop
+  // function in the request could also try to delete the request from
+  // the request map and the iterator could end up being invalid.
   RequestMap::iterator index = request_map_.begin();
   while (index != request_map_.end()) {
     PluginUrlRequest* request = (*index).second;
-    if (request) {
-      int request_id = request->id();
-      request->Stop();
-    }
+    DCHECK(request != NULL);
+    request_list.push_back(request);
     index++;
   }
-
   request_map_.clear();
+
+  for (unsigned int index = 0; index < request_list.size(); ++index) {
+    request_list[index]->Stop();
+  }
 }
 
 bool ChromeFrameAutomationClient::Reinitialize(
@@ -1108,6 +1119,7 @@ bool ChromeFrameAutomationClient::Reinitialize(
     return false;
   }
 
+  CleanupAsyncRequests();
   chrome_frame_delegate_ = delegate;
   SetParentWindow(NULL);
   return true;
diff --git a/chrome_frame/urlmon_url_request.cc b/chrome_frame/urlmon_url_request.cc
index 1c71edb..560f4b8 100644
--- a/chrome_frame/urlmon_url_request.cc
+++ b/chrome_frame/urlmon_url_request.cc
@@ -46,13 +46,14 @@ bool UrlmonUrlRequest::Start() {
     return false;
   }
 
-  worker_thread_->message_loop()->PostTask(
-      FROM_HERE, NewRunnableMethod(this, &UrlmonUrlRequest::StartAsync));
-
   // Take a self reference to maintain COM lifetime. This will be released
   // in EndRequest
   AddRef();
   request_handler()->AddRequest(this);
+
+  worker_thread_->message_loop()->PostTask(
+      FROM_HERE, NewRunnableMethod(this, &UrlmonUrlRequest::StartAsync));
+
   return true;
 }
 
@@ -64,6 +65,11 @@ void UrlmonUrlRequest::Stop() {
     return;
   }
 
+  // We can remove the request from the map safely here if it is still valid.
+  // There is an additional reference on the UrlmonUrlRequest instance which
+  // is released when the task scheduled by the EndRequest function executes.
+  request_handler()->RemoveRequest(this);
+
   worker_thread_->message_loop()->PostTask(
       FROM_HERE, NewRunnableMethod(this, &UrlmonUrlRequest::StopAsync));
 }
@@ -205,6 +211,8 @@ STDMETHODIMP UrlmonUrlRequest::OnStopBinding(HRESULT result, LPCWSTR error) {
     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_ = NULL;
@@ -605,6 +613,9 @@ void UrlmonUrlRequest::EndRequest() {
 }
 
 void UrlmonUrlRequest::EndRequestInternal() {
+  // The request object could have been removed from the map in the
+  // OnRequestEnd callback which executes on receiving the
+  // AutomationMsg_RequestEnd IPC from Chrome.
   request_handler()->RemoveRequest(this);
   // Release the outstanding reference in the context of the UI thread to
   // ensure that our instance gets deleted in the same thread which created it.