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

5 files changed, 44 insertions, 33 deletions

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.