ChromeFrame HTTP requests would randomly fail if we navigated to multiple HTTP sites. This was because

the automation resource message filter tracked HTTP requests based on the request ids which are generated
by the renderer process. As a result a new request would get created say with id 0, while an existing request
would end in ChromeFrame causing the new request to incorrectly shutdown.

Fix is to revert back to the original way of tracking requests with an auto incrementing id. The automation url
job maintains both ids now, i.e. the automation request id and the chrome request id. The download notification
receives the automation id and basically looks up the associated automation request id and sends the notification
back to ChromeFrame.

This fixes bug http://code.google.com/p/chromium/issues/detail?id=27401

Other fixes in this CL include the following:-
1. The active document instance would never get destroyed. This was because we call ShowUI on the doc host 
   which maintains a reference. We need to call HideUI in Setsite of NULL, which releases the reference.

2. When the active x instance is shutting down we try to shutdown all running requests in the OnDestroy handler.
   To ensure that the request is deleted from the request map and released in the same thread which created it
   we post a task back to the ui thread which never runs as the window is being destroyed. Fix is to create
   a message only window with every urlmonrequest instance which supports task marshaling.

Tests in a future CL.

Bug=27401

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

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

10 files changed, 114 insertions, 41 deletions

diff --git a/chrome/browser/automation/automation_resource_message_filter.cc b/chrome/browser/automation/automation_resource_message_filter.cc
index e0512ca..6709ba2 100644
--- a/chrome/browser/automation/automation_resource_message_filter.cc
+++ b/chrome/browser/automation/automation_resource_message_filter.cc
@@ -21,6 +21,8 @@
 AutomationResourceMessageFilter::RenderViewMap
     AutomationResourceMessageFilter::filtered_render_views_;
 
+int AutomationResourceMessageFilter::unique_request_id_ = 1;
+
 AutomationResourceMessageFilter::AutomationResourceMessageFilter()
     : channel_(NULL) {
   ChromeThread::PostTask(
@@ -193,6 +195,37 @@ bool AutomationResourceMessageFilter::LookupRegisteredRenderView(
   return found;
 }
 
+bool AutomationResourceMessageFilter::GetAutomationRequestId(
+    int request_id, int* automation_request_id) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
+
+  RequestMap::iterator it = request_map_.begin();
+  while (it != request_map_.end()) {
+    URLRequestAutomationJob* job = it->second;
+    DCHECK(job);
+    if (job && job->request_id() == request_id) {
+      *automation_request_id = job->id();
+      return true;
+    }
+    it++;
+  }
+
+  return false;
+}
+
+bool AutomationResourceMessageFilter::SendDownloadRequestToHost(
+    int routing_id, int tab_handle, int request_id) {
+  int automation_request_id = 0;
+  bool valid_id = GetAutomationRequestId(request_id, &automation_request_id);
+  if (!valid_id) {
+    NOTREACHED() << "Invalid request id: " << request_id;
+    return false;
+  }
+
+  return Send(new AutomationMsg_DownloadRequestInHost(0, tab_handle,
+                                                      automation_request_id));
+}
+
 void AutomationResourceMessageFilter::OnSetFilteredInet(bool enable) {
   chrome_browser_net::SetUrlRequestMocksEnabled(enable);
 }
diff --git a/chrome/browser/automation/automation_resource_message_filter.h b/chrome/browser/automation/automation_resource_message_filter.h
index ceb6115..7bb03d6 100644
--- a/chrome/browser/automation/automation_resource_message_filter.h
+++ b/chrome/browser/automation/automation_resource_message_filter.h
@@ -39,6 +39,12 @@ class AutomationResourceMessageFilter
   AutomationResourceMessageFilter();
   virtual ~AutomationResourceMessageFilter();
 
+  // Returns a new automation request id. This is unique across all instances
+  // of AutomationResourceMessageFilter.
+  int NewAutomationRequestId() {
+    return base::subtle::Barrier_AtomicIncrement(&unique_request_id_, 1);
+  }
+
   // IPC::ChannelProxy::MessageFilter methods:
   virtual void OnFilterAdded(IPC::Channel* channel);
   virtual void OnChannelConnected(int32 peer_pid);
@@ -63,7 +69,16 @@ class AutomationResourceMessageFilter
   static bool LookupRegisteredRenderView(
       int renderer_pid, int renderer_id, AutomationDetails* details);
 
+  // Sends the download request to the automation host.
+  bool SendDownloadRequestToHost(int routing_id, int tab_handle,
+                                 int request_id);
+
  protected:
+  // Retrieves the automation request id for the passed in chrome request
+  // id and returns it in the automation_request_id parameter.
+  // Returns true on success.
+  bool GetAutomationRequestId(int request_id, int* automation_request_id);
+
   static void RegisterRenderViewInIOThread(int renderer_pid, int renderer_id,
       int tab_handle, AutomationResourceMessageFilter* filter);
   static void UnRegisterRenderViewInIOThread(int renderer_pid, int renderer_id);
@@ -94,6 +109,9 @@ class AutomationResourceMessageFilter
   // owned by this class.
   IPC::Channel* channel_;
 
+  // A unique request id per process.
+  static int unique_request_id_;
+
   // Map of outstanding requests.
   RequestMap request_map_;
 
diff --git a/chrome/browser/automation/url_request_automation_job.cc b/chrome/browser/automation/url_request_automation_job.cc
index fdd680f..f6d4ecd 100644
--- a/chrome/browser/automation/url_request_automation_job.cc
+++ b/chrome/browser/automation/url_request_automation_job.cc
@@ -52,13 +52,18 @@ URLRequest::ProtocolFactory* URLRequestAutomationJob::old_https_factory_
 URLRequestAutomationJob::URLRequestAutomationJob(URLRequest* request, int tab,
     int request_id, AutomationResourceMessageFilter* filter)
     : URLRequestJob(request),
-      id_(request_id),
       tab_(tab),
       message_filter_(filter),
       pending_buf_size_(0),
-      redirect_status_(0) {
+      redirect_status_(0),
+      request_id_(request_id) {
   DLOG(INFO) << "URLRequestAutomationJob create. Count: " << ++instance_count_;
-  DCHECK_NE(id_, -1);
+  DCHECK(message_filter_ != NULL);
+
+  if (message_filter_) {
+    id_ = message_filter_->NewAutomationRequestId();
+    DCHECK_NE(id_, 0);
+  }
 }
 
 URLRequestAutomationJob::~URLRequestAutomationJob() {
@@ -217,7 +222,8 @@ bool URLRequestAutomationJob::MayFilterMessage(const IPC::Message& message,
     case AutomationMsg_RequestEnd::ID: {
       void* iter = NULL;
       int tab = 0;
-      if (message.ReadInt(&iter, &tab) && message.ReadInt(&iter, request_id)) {
+      if (message.ReadInt(&iter, &tab) &&
+          message.ReadInt(&iter, request_id)) {
         return true;
       }
       break;
diff --git a/chrome/browser/automation/url_request_automation_job.h b/chrome/browser/automation/url_request_automation_job.h
index 0edd876..5865fb3 100644
--- a/chrome/browser/automation/url_request_automation_job.h
+++ b/chrome/browser/automation/url_request_automation_job.h
@@ -47,6 +47,10 @@ class URLRequestAutomationJob : public URLRequestJob {
     return id_;
   }
 
+  int request_id() const {
+    return request_id_;
+  }
+
  protected:
   // Protected URLRequestJob override.
   virtual bool ReadRawData(net::IOBuffer* buf, int buf_size, int* bytes_read);
@@ -75,6 +79,7 @@ class URLRequestAutomationJob : public URLRequestJob {
   scoped_refptr<net::HttpResponseHeaders> headers_;
   std::string redirect_url_;
   int redirect_status_;
+  int request_id_;
 
   static int instance_count_;
 
diff --git a/chrome/browser/external_tab_container.cc b/chrome/browser/external_tab_container.cc
index 4440294..806c4c6 100644
--- a/chrome/browser/external_tab_container.cc
+++ b/chrome/browser/external_tab_container.cc
@@ -404,10 +404,10 @@ bool ExternalTabContainer::TakeFocus(bool reverse) {
 bool ExternalTabContainer::CanDownload(int request_id) {
   if (load_requests_via_automation_) {
     if (automation_) {
-      // NOTE: The request_id must be the same id as used by corresponding
-      // URLRequestAutomationJob instance to communicate with the host.
-      automation_->Send(new AutomationMsg_DownloadRequestInHost(0, tab_handle_,
-                                                                request_id));
+      ChromeThread::PostTask(ChromeThread::IO, FROM_HERE,
+          NewRunnableMethod(automation_resource_message_filter_,
+              &AutomationResourceMessageFilter::SendDownloadRequestToHost,
+              0, tab_handle_, request_id));
     }
   } else {
     DLOG(WARNING) << "Downloads are only supported with host browser network "
diff --git a/chrome_frame/chrome_active_document.cc b/chrome_frame/chrome_active_document.cc
index d0de8dd..bbc1887 100644
--- a/chrome_frame/chrome_active_document.cc
+++ b/chrome_frame/chrome_active_document.cc
@@ -139,14 +139,6 @@ STDMETHODIMP ChromeActiveDocument::DoVerb(LONG verb,
                                                       pos);
 }
 
-STDMETHODIMP ChromeActiveDocument::InPlaceDeactivate(void) {
-  // Release the pointers we have no need for now.
-  doc_site_.Release();
-  in_place_frame_.Release();
-  return IOleInPlaceObjectWindowlessImpl<ChromeActiveDocument>::
-             InPlaceDeactivate();
-}
-
 // Override IOleInPlaceActiveObjectImpl::OnDocWindowActivate
 STDMETHODIMP ChromeActiveDocument::OnDocWindowActivate(BOOL activate) {
   DLOG(INFO) << __FUNCTION__;
@@ -298,6 +290,16 @@ HRESULT ChromeActiveDocument::IOleObject_SetClientSite(
       g_active_doc_cache.Set(NULL);
       cached_document->Release();
     }
+
+    ScopedComPtr<IDocHostUIHandler> doc_host_handler;
+    doc_host_handler.QueryFrom(doc_site_);
+
+    if (doc_host_handler.get()) {
+      doc_host_handler->HideUI();
+    }
+
+    doc_site_.Release();
+    in_place_frame_.Release();
   }
   return Base::IOleObject_SetClientSite(client_site);
 }
diff --git a/chrome_frame/chrome_active_document.h b/chrome_frame/chrome_active_document.h
index fa6a271..ee2cf6e 100644
--- a/chrome_frame/chrome_active_document.h
+++ b/chrome_frame/chrome_active_document.h
@@ -166,7 +166,6 @@ END_EXEC_COMMAND_MAP()
                     LONG index,
                     HWND parent_window,
                     LPCRECT pos);
-  STDMETHOD(InPlaceDeactivate)(void);
 
   // Override IOleInPlaceActiveObjectImpl::OnDocWindowActivate
   STDMETHOD(OnDocWindowActivate)(BOOL activate);
diff --git a/chrome_frame/chrome_frame_activex_base.h b/chrome_frame/chrome_frame_activex_base.h
index 4941bba..11dd24e 100644
--- a/chrome_frame/chrome_frame_activex_base.h
+++ b/chrome_frame/chrome_frame_activex_base.h
@@ -150,13 +150,9 @@ class ATL_NO_VTABLE ChromeFrameActivexBase :
   public IPropertyNotifySinkCP<T>,
   public CComCoClass<T, &class_id>,
   public CComControl<T>,
-  public ChromeFramePlugin<T>,
-  public TaskMarshallerThroughWindowsMessages<
-            ChromeFrameActivexBase<T, class_id> > {
+  public ChromeFramePlugin<T> {
  protected:
   typedef std::set<ScopedComPtr<IDispatch> > EventHandlers;
-  typedef TaskMarshallerThroughWindowsMessages<
-      ChromeFrameActivexBase<T, class_id> > TaskMarshaller;
   typedef ChromeFrameActivexBase<T, class_id> Base;
 
  public:
@@ -204,7 +200,6 @@ BEGIN_MSG_MAP(ChromeFrameActivexBase)
   MESSAGE_HANDLER(WM_DESTROY, OnDestroy)
   CHAIN_MSG_MAP(ChromeFramePlugin<T>)
   CHAIN_MSG_MAP(CComControl<T>)
-  CHAIN_MSG_MAP(TaskMarshaller)
   DEFAULT_REFLECTION_HANDLER()
 END_MSG_MAP()
 
@@ -448,7 +443,6 @@ END_MSG_MAP()
         request_info.extra_request_headers, request_info.upload_data.get(),
         static_cast<T*>(this)->is_frame_busting_enabled())) {
       request->set_worker_thread(&worker_thread_);
-      request->set_task_marshaller(this);
       // If Start is successful, it will add a self reference.
       request->Start();
       request->set_parent_window(m_hWnd);
diff --git a/chrome_frame/urlmon_url_request.cc b/chrome_frame/urlmon_url_request.cc
index 560f4b8..6ca2401 100644
--- a/chrome_frame/urlmon_url_request.cc
+++ b/chrome_frame/urlmon_url_request.cc
@@ -27,8 +27,7 @@ UrlmonUrlRequest::UrlmonUrlRequest()
       thread_(PlatformThread::CurrentId()),
       redirect_status_(0),
       parent_window_(NULL),
-      worker_thread_(NULL),
-      task_marshaller_(NULL) {
+      worker_thread_(NULL) {
   DLOG(INFO) << StringPrintf("Created request. Obj: %X", this)
       << " Count: " << ++instance_count_;
 }
@@ -46,8 +45,15 @@ bool UrlmonUrlRequest::Start() {
     return false;
   }
 
+  Create(HWND_MESSAGE);
+  if (!IsWindow()) {
+    NOTREACHED() << "Failed to create urlmon message window: "
+                 << GetLastError();
+    return false;
+  }
+
   // Take a self reference to maintain COM lifetime. This will be released
-  // in EndRequest
+  // in OnFinalMessage
   AddRef();
   request_handler()->AddRequest(this);
 
@@ -102,6 +108,13 @@ void UrlmonUrlRequest::StopAsync() {
   }
 }
 
+void UrlmonUrlRequest::OnFinalMessage(HWND window) {
+  m_hWnd = NULL;
+  // 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.
+  Release();
+}
+
 bool UrlmonUrlRequest::Read(int bytes_to_read) {
   DCHECK_EQ(PlatformThread::CurrentId(), thread_);
 
@@ -603,13 +616,10 @@ void UrlmonUrlRequest::EndRequest() {
 
   OnResponseEnd(status_);
 
-  DCHECK(task_marshaller_ != NULL);
-
   // Remove the request mapping and release the outstanding reference to us in
   // the context of the UI thread.
-  task_marshaller_->PostTask(
-      FROM_HERE, NewRunnableMethod(this,
-                                   &UrlmonUrlRequest::EndRequestInternal));
+  PostTask(FROM_HERE,
+           NewRunnableMethod(this, &UrlmonUrlRequest::EndRequestInternal));
 }
 
 void UrlmonUrlRequest::EndRequestInternal() {
@@ -617,9 +627,9 @@ void UrlmonUrlRequest::EndRequestInternal() {
   // 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.
-  Release();
+  // The current instance could get destroyed in the context of DestroyWindow.
+  // We should not access the object after this.
+  DestroyWindow();
 }
 
 int UrlmonUrlRequest::GetHttpResponseStatus() const {
diff --git a/chrome_frame/urlmon_url_request.h b/chrome_frame/urlmon_url_request.h
index 25c54ae..eb13d17 100644
--- a/chrome_frame/urlmon_url_request.h
+++ b/chrome_frame/urlmon_url_request.h
@@ -8,6 +8,7 @@
 #include <urlmon.h>
 #include <atlbase.h>
 #include <atlcom.h>
+#include <atlwin.h>
 
 #include <algorithm>
 
@@ -28,8 +29,13 @@ class UrlmonUrlRequest
       public IBindStatusCallback,
       public IHttpNegotiate,
       public IAuthenticate,
-      public IHttpSecurity {
+      public IHttpSecurity,
+      public CWindowImpl<UrlmonUrlRequest>,
+      public TaskMarshallerThroughWindowsMessages<UrlmonUrlRequest> {
  public:
+  typedef TaskMarshallerThroughWindowsMessages<UrlmonUrlRequest>
+      TaskMarshaller;
+
   UrlmonUrlRequest();
   ~UrlmonUrlRequest();
 
@@ -46,6 +52,10 @@ BEGIN_SERVICE_MAP(UrlmonUrlRequest)
   SERVICE_ENTRY(IID_IHttpNegotiate);
 END_SERVICE_MAP()
 
+BEGIN_MSG_MAP(UrlmonUrlRequest)
+  CHAIN_MSG_MAP(TaskMarshaller)
+END_MSG_MAP()
+
   // PluginUrlRequest implementation
   virtual bool Start();
   virtual void Stop();
@@ -99,9 +109,7 @@ END_SERVICE_MAP()
     worker_thread_ = worker_thread;
   }
 
-  void set_task_marshaller(TaskMarshaller* task_marshaller) {
-    task_marshaller_ = task_marshaller;
-  }
+  virtual void OnFinalMessage(HWND window);
 
  protected:
   // The following functions issue and handle Urlmon requests on the dedicated
@@ -114,8 +122,6 @@ END_SERVICE_MAP()
   // URL requests are handled on this thread.
   base::Thread* worker_thread_;
 
-  TaskMarshaller* task_marshaller_;
-
   // A fake stream class to make it easier to copy received data using
   // IStream::CopyTo instead of allocating temporary buffers and keeping
   // track of data copied so far.