The ChromeFrameAutomationClient class needs to be refcounted as it implements the PluginRequestHandler

interface which is maintained by individual requests which can outlive the active document/activex instances.
I ran into a crash where UrlmonUrlRequest was calling into an invalid PluginRequestHandler pointer which
had been destroyed just before.

We also need to ensure that UrlmonUrlRequest and ChromeFrameActiveXBase select the multi threaded model as
AddRef/Release can be invoked from multiple threads.

I also removed the CleanupAsyncRequests function in ChromeFrameAutomationClient and moved all the code to CleanupRequests, which ensures that we treat synchronous
and asynchronous requests similarly. 

There are instances where an automation client instance is created and destroyed without being initialized which causes a spurious assert to fire in the Uninitialize function. I added a check in the Uninitialize function to return if the state is uninitialized.

Bug=none

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

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

12 files changed, 62 insertions, 78 deletions

diff --git a/chrome_frame/chrome_active_document.cc b/chrome_frame/chrome_active_document.cc
index bbc1887..4e862f9 100644
--- a/chrome_frame/chrome_active_document.cc
+++ b/chrome_frame/chrome_active_document.cc
@@ -58,7 +58,7 @@ HRESULT ChromeActiveDocument::FinalConstruct() {
   ChromeActiveDocument* cached_document = g_active_doc_cache.Get();
   if (cached_document) {
     DCHECK(automation_client_.get() == NULL);
-    automation_client_.reset(cached_document->automation_client_.release());
+    automation_client_ = cached_document->automation_client_.release();
     DLOG(INFO) << "Reusing automation client instance from "
                << cached_document;
     DCHECK(automation_client_.get() != NULL);
diff --git a/chrome_frame/chrome_frame_activex_base.h b/chrome_frame/chrome_frame_activex_base.h
index 11dd24e..4a4fe95 100644
--- a/chrome_frame/chrome_frame_activex_base.h
+++ b/chrome_frame/chrome_frame_activex_base.h
@@ -134,7 +134,7 @@ extern bool g_first_launch_by_process_;
 // Common implementation for ActiveX and Active Document
 template <class T, const CLSID& class_id>
 class ATL_NO_VTABLE ChromeFrameActivexBase :
-  public CComObjectRootEx<CComSingleThreadModel>,
+  public CComObjectRootEx<CComMultiThreadModel>,
   public IOleControlImpl<T>,
   public IOleObjectImpl<T>,
   public IOleInPlaceActiveObjectImpl<T>,
@@ -508,7 +508,7 @@ END_MSG_MAP()
       worker_thread_.message_loop()->PostTask(
           FROM_HERE, NewRunnableMethod(this, &Base::OnWorkerStop));
       if (automation_client_.get())
-        automation_client_->CleanupAsyncRequests();
+        automation_client_->CleanupRequests();
       worker_thread_.Stop();
     }
     return 0;
diff --git a/chrome_frame/chrome_frame_automation.cc b/chrome_frame/chrome_frame_automation.cc
index 1a4b763..8dbf795 100644
--- a/chrome_frame/chrome_frame_automation.cc
+++ b/chrome_frame/chrome_frame_automation.cc
@@ -436,7 +436,6 @@ bool ChromeFrameAutomationClient::Initialize(
   chrome_frame_delegate_ = chrome_frame_delegate;
   incognito_ = incognito;
   ui_thread_id_ = PlatformThread::CurrentId();
-
 #ifndef NDEBUG
   // In debug mode give more time to work with a debugger.
   if (IsDebuggerPresent()) {
@@ -476,6 +475,11 @@ bool ChromeFrameAutomationClient::Initialize(
 void ChromeFrameAutomationClient::Uninitialize() {
   DLOG(INFO) << __FUNCTION__;
 
+  if (init_state_ == UNINITIALIZED) {
+    DLOG(WARNING) << __FUNCTION__ << ": Automation client not initialized";
+    return;
+  }
+
   init_state_ = UNINITIALIZING;
 
   // Called from client's FinalRelease() / destructor
@@ -633,14 +637,14 @@ class InstallExtensionContext {
   }
 
   void InstallExtensionComplete(AutomationMsg_ExtensionResponseValues res) {
-    client_->PostTask(FROM_HERE, NewRunnableMethod(client_,
+    client_->PostTask(FROM_HERE, NewRunnableMethod(client_.get(),
         &ChromeFrameAutomationClient::InstallExtensionComplete, crx_path_,
         user_data_, res));
     delete this;
   }
 
  private:
-  ChromeFrameAutomationClient* client_;
+  scoped_refptr<ChromeFrameAutomationClient> client_;
   FilePath crx_path_;
   void* user_data_;
 };
@@ -1072,20 +1076,6 @@ bool ChromeFrameAutomationClient::IsValidRequest(
 
 void ChromeFrameAutomationClient::CleanupRequests() {
   DCHECK_EQ(PlatformThread::CurrentId(), ui_thread_id_);
-  while (request_map_.size()) {
-    PluginUrlRequest* request = request_map_.begin()->second;
-    if (request) {
-      int request_id = request->id();
-      request->Stop();
-    }
-  }
-
-  DCHECK(request_map_.empty());
-  request_map_.clear();
-}
-
-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
@@ -1119,7 +1109,7 @@ bool ChromeFrameAutomationClient::Reinitialize(
     return false;
   }
 
-  CleanupAsyncRequests();
+  CleanupRequests();
   chrome_frame_delegate_ = delegate;
   SetParentWindow(NULL);
   return true;
diff --git a/chrome_frame/chrome_frame_automation.h b/chrome_frame/chrome_frame_automation.h
index 8def0fd..90c1951 100644
--- a/chrome_frame/chrome_frame_automation.h
+++ b/chrome_frame/chrome_frame_automation.h
@@ -233,9 +233,6 @@ class ChromeFrameAutomationClient
   PluginUrlRequest* LookupRequest(int request_id) const;
   bool IsValidRequest(PluginUrlRequest* request) const;
   void CleanupRequests();
-  // For IE the host network stack requests are issued on a separate thread,
-  // which requires the requests to be cleaned up asynchronously.
-  void CleanupAsyncRequests();
 
   void set_use_chrome_network(bool use_chrome_network) {
     use_chrome_network_ = use_chrome_network;
@@ -263,13 +260,6 @@ class ChromeFrameAutomationClient
 
   void SetPageFontSize(enum AutomationPageFontSize);
 
-  // Dummy reference counting functions to enable us to use the
-  // TaskMarshallerThroughWindowsMessages functionality. At this point we don't
-  // need to ensure that any tasks executed on us grab a reference to ensure
-  // that the instance remains valid.
-  void AddRef() {}
-  void Release() {}
-
  protected:
   // ChromeFrameAutomationProxy::LaunchDelegate implementation.
   virtual void LaunchComplete(ChromeFrameAutomationProxy* proxy,
diff --git a/chrome_frame/chrome_frame_plugin.h b/chrome_frame/chrome_frame_plugin.h
index 6d29948..01ee2e5 100644
--- a/chrome_frame/chrome_frame_plugin.h
+++ b/chrome_frame/chrome_frame_plugin.h
@@ -5,6 +5,7 @@
 #ifndef CHROME_FRAME_CHROME_FRAME_PLUGIN_H_
 #define CHROME_FRAME_CHROME_FRAME_PLUGIN_H_
 
+#include "base/ref_counted.h"
 #include "base/win_util.h"
 #include "chrome_frame/chrome_frame_automation.h"
 #include "chrome_frame/utils.h"
@@ -32,7 +33,7 @@ END_MSG_MAP()
 
   bool Initialize() {
     DCHECK(!automation_client_.get());
-    automation_client_.reset(CreateAutomationClient());
+    automation_client_ = CreateAutomationClient();
     if (!automation_client_.get()) {
       NOTREACHED() << "new ChromeFrameAutomationClient";
       return false;
@@ -44,7 +45,7 @@ END_MSG_MAP()
   void Uninitialize() {
     if (automation_client_.get()) {
       automation_client_->Uninitialize();
-      automation_client_.reset();
+      automation_client_ = NULL;
     }
   }
 
@@ -191,7 +192,7 @@ END_MSG_MAP()
 
  protected:
   // Our gateway to chrome land
-  scoped_ptr<ChromeFrameAutomationClient> automation_client_;
+  scoped_refptr<ChromeFrameAutomationClient> automation_client_;
 
   // Url of the containing document.
   std::string document_url_;
diff --git a/chrome_frame/find_dialog.cc b/chrome_frame/find_dialog.cc
index 15aaff7..eddfc81 100644
--- a/chrome_frame/find_dialog.cc
+++ b/chrome_frame/find_dialog.cc
@@ -50,7 +50,7 @@ LRESULT CFFindDialog::OnCancel(WORD wNotifyCode, WORD wID, HWND hWndCtl,
 LRESULT CFFindDialog::OnInitDialog(UINT msg, WPARAM wparam, LPARAM lparam,
                                    BOOL& handled) {
   // Init() must be called before Create() or DoModal()!
-  DCHECK(automation_client_);
+  DCHECK(automation_client_.get());
 
   InstallMessageHook();
   SendDlgItemMessage(IDC_FIND_TEXT, EM_EXLIMITTEXT, 0, kMaxFindChars);
@@ -67,32 +67,32 @@ LRESULT CALLBACK CFFindDialog::GetMsgProc(int code, WPARAM wparam,
                                           LPARAM lparam) {
   // Mostly borrowed from http://support.microsoft.com/kb/q187988/
   // and http://www.codeproject.com/KB/atl/cdialogmessagehook.aspx.
-  LPMSG msg = reinterpret_cast<LPMSG>(lparam);

-  if (code >= 0 && wparam == PM_REMOVE &&

-      msg->message >= WM_KEYFIRST && msg->message <= WM_KEYLAST) {

-    HWND hwnd = GetActiveWindow();

-    if (::IsWindow(hwnd) && ::IsDialogMessage(hwnd, msg)) {

-      // The value returned from this hookproc is ignored, and it cannot

-      // be used to tell Windows the message has been handled. To avoid

-      // further processing, convert the message to WM_NULL before

-      // returning.

-      msg->hwnd = NULL;

-      msg->message = WM_NULL;

-      msg->lParam = 0L;

-      msg->wParam = 0;

-    }

-  }

-

-  // Passes the hook information to the next hook procedure in

-  // the current hook chain.

+  LPMSG msg = reinterpret_cast<LPMSG>(lparam);
+  if (code >= 0 && wparam == PM_REMOVE &&
+      msg->message >= WM_KEYFIRST && msg->message <= WM_KEYLAST) {
+    HWND hwnd = GetActiveWindow();
+    if (::IsWindow(hwnd) && ::IsDialogMessage(hwnd, msg)) {
+      // The value returned from this hookproc is ignored, and it cannot
+      // be used to tell Windows the message has been handled. To avoid
+      // further processing, convert the message to WM_NULL before
+      // returning.
+      msg->hwnd = NULL;
+      msg->message = WM_NULL;
+      msg->lParam = 0L;
+      msg->wParam = 0;
+    }
+  }
+
+  // Passes the hook information to the next hook procedure in
+  // the current hook chain.
   return ::CallNextHookEx(msg_hook_, code, wparam, lparam);
 }
 
 bool CFFindDialog::InstallMessageHook() {
   // Make sure we only call this once.
   DCHECK(msg_hook_ == NULL);
-  msg_hook_ = ::SetWindowsHookEx(WH_GETMESSAGE, &CFFindDialog::GetMsgProc,

-                                 _AtlBaseModule.m_hInst, GetCurrentThreadId());

+  msg_hook_ = ::SetWindowsHookEx(WH_GETMESSAGE, &CFFindDialog::GetMsgProc,
+                                 _AtlBaseModule.m_hInst, GetCurrentThreadId());
   DCHECK(msg_hook_ != NULL);
   return msg_hook_ != NULL;
 }
diff --git a/chrome_frame/find_dialog.h b/chrome_frame/find_dialog.h
index 9e7cafe..f2089b9 100644
--- a/chrome_frame/find_dialog.h
+++ b/chrome_frame/find_dialog.h
@@ -48,7 +48,7 @@ class CFFindDialog : public CDialogImpl<CFFindDialog> {
   static HHOOK msg_hook_;
 
   // We don't own these, and they must exist at least as long as we do.
-  ChromeFrameAutomationClient* automation_client_;
+  scoped_refptr<ChromeFrameAutomationClient> automation_client_;
 };
 
 #endif  // CHROME_FRAME_FIND_DIALOG_H_
diff --git a/chrome_frame/np_proxy_service.h b/chrome_frame/np_proxy_service.h
index c0a85c8..e41bda1 100644
--- a/chrome_frame/np_proxy_service.h
+++ b/chrome_frame/np_proxy_service.h
@@ -102,7 +102,7 @@ class NpProxyService : public NsISupportsImplBase<NpProxyService>,
   void Reset();
   DictionaryValue* BuildProxyValueSet();
 
-  ChromeFrameAutomationClient* automation_client_;
+  scoped_refptr<ChromeFrameAutomationClient> automation_client_;
 
   ScopedNsPtr<nsIServiceManager> service_manager_;
   ScopedNsPtr<nsIPrefService> pref_service_;
diff --git a/chrome_frame/plugin_url_request.h b/chrome_frame/plugin_url_request.h
index 554855d..4993770 100644
--- a/chrome_frame/plugin_url_request.h
+++ b/chrome_frame/plugin_url_request.h
@@ -21,7 +21,9 @@ class PluginUrlRequest;
 
 // Interface for a class that keeps a collection of outstanding
 // reqeusts and offers an outgoing channel.
-class PluginRequestHandler : public IPC::Message::Sender {
+class PluginRequestHandler
+    : public IPC::Message::Sender,
+      public base::RefCountedThreadSafe<PluginRequestHandler> {
  public:
   virtual bool AddRequest(PluginUrlRequest* request) = 0;
   virtual void RemoveRequest(PluginUrlRequest* request) = 0;
@@ -127,7 +129,7 @@ class PluginUrlRequest : public UrlRequestReference {
   bool frame_busting_enabled_;
 
  private:
-  PluginRequestHandler* request_handler_;
+  scoped_refptr<PluginRequestHandler> request_handler_;
   int tab_;
   int remote_request_id_;
   uint64 post_data_len_;
diff --git a/chrome_frame/test/chrome_frame_automation_mock.h b/chrome_frame/test/chrome_frame_automation_mock.h
index c77f41f..a0644d3 100644
--- a/chrome_frame/test/chrome_frame_automation_mock.h
+++ b/chrome_frame/test/chrome_frame_automation_mock.h
@@ -23,14 +23,14 @@ class AutomationMockDelegate
       const std::wstring& extra_chrome_arguments, bool incognito)
       : caller_message_loop_(caller_message_loop), is_connected_(false) {
     test_server_.SetUp();
-    automation_client_.reset(new ChromeFrameAutomationClient);
+    automation_client_ = new ChromeFrameAutomationClient;
     automation_client_->Initialize(this, launch_timeout, perform_version_check,
         profile_name, extra_chrome_arguments, incognito);
   }
   ~AutomationMockDelegate() {
     if (automation_client_.get()) {
       automation_client_->Uninitialize();
-      automation_client_.reset();
+      automation_client_ = NULL;
     }
     if (IsWindow())
       DestroyWindow();
diff --git a/chrome_frame/test/chrome_frame_unittests.cc b/chrome_frame/test/chrome_frame_unittests.cc
index 6472310..79b8d13 100644
--- a/chrome_frame/test/chrome_frame_unittests.cc
+++ b/chrome_frame/test/chrome_frame_unittests.cc
@@ -901,8 +901,8 @@ TEST(CFACWithChrome, CreateTooFast) {
   int timeout = 0;  // Chrome cannot send Hello message so fast.
   const std::wstring profile = L"Adam.N.Epilinter";
 
-  scoped_ptr<ChromeFrameAutomationClient> client;
-  client.reset(new ChromeFrameAutomationClient());
+  scoped_refptr<ChromeFrameAutomationClient> client;
+  client = new ChromeFrameAutomationClient();
 
   EXPECT_CALL(cfd, OnAutomationServerLaunchFailed(AUTOMATION_TIMEOUT,
                                                   testing::_))
@@ -924,8 +924,8 @@ TEST(CFACWithChrome, CreateNotSoFast) {
   const std::wstring profile = L"Adam.N.Epilinter";
   int timeout = 10000;
 
-  scoped_ptr<ChromeFrameAutomationClient> client;
-  client.reset(new ChromeFrameAutomationClient);
+  scoped_refptr<ChromeFrameAutomationClient> client;
+  client = new ChromeFrameAutomationClient;
 
   EXPECT_CALL(cfd, OnAutomationServerReady())
       .Times(1)
@@ -938,7 +938,7 @@ TEST(CFACWithChrome, CreateNotSoFast) {
 
   loop.RunFor(11);
   client->Uninitialize();
-  client.reset(NULL);
+  client = NULL;
 }
 
 MATCHER_P(MsgType, msg_type, "IPC::Message::type()") {
@@ -960,8 +960,8 @@ TEST(CFACWithChrome, NavigateOk) {
   const std::string url = "about:version";
   int timeout = 10000;
 
-  scoped_ptr<ChromeFrameAutomationClient> client;
-  client.reset(new ChromeFrameAutomationClient);
+  scoped_refptr<ChromeFrameAutomationClient> client;
+  client = new ChromeFrameAutomationClient;
 
   EXPECT_CALL(cfd, OnAutomationServerReady())
       .WillOnce(testing::IgnoreResult(testing::InvokeWithoutArgs(CreateFunctor(
@@ -989,7 +989,7 @@ TEST(CFACWithChrome, NavigateOk) {
   EXPECT_TRUE(client->Initialize(&cfd, timeout, false, profile, L"", false));
   loop.RunFor(10);
   client->Uninitialize();
-  client.reset(NULL);
+  client = NULL;
 }
 
 // Bug: http://b/issue?id=2033644
@@ -1000,8 +1000,8 @@ TEST(CFACWithChrome, DISABLED_NavigateFailed) {
   const std::string url = "http://127.0.0.3:65412/";
   int timeout = 10000;
 
-  scoped_ptr<ChromeFrameAutomationClient> client;
-  client.reset(new ChromeFrameAutomationClient);
+  scoped_refptr<ChromeFrameAutomationClient> client;
+  client = new ChromeFrameAutomationClient;
 
   EXPECT_CALL(cfd, OnAutomationServerReady())
       .WillOnce(testing::IgnoreResult(testing::InvokeWithoutArgs(CreateFunctor(
@@ -1025,7 +1025,7 @@ TEST(CFACWithChrome, DISABLED_NavigateFailed) {
 
   loop.RunFor(10);
   client->Uninitialize();
-  client.reset(NULL);
+  client = NULL;
 }
 
 MATCHER_P(EqURLRequest, x, "IPC::AutomationURLRequest matcher") {
@@ -1056,8 +1056,8 @@ TEST(CFACWithChrome, UseHostNetworkStack) {
   const std::string url = "http://bongo.com";
   int timeout = 10000;
 
-  scoped_ptr<ChromeFrameAutomationClient> client;
-  client.reset(new ChromeFrameAutomationClient);
+  scoped_refptr<ChromeFrameAutomationClient> client;
+  client = new ChromeFrameAutomationClient;
   client->set_use_chrome_network(false);
   cfd.SetAutomationSender(client.get());
 
@@ -1131,7 +1131,7 @@ TEST(CFACWithChrome, UseHostNetworkStack) {
 
   loop.RunFor(10);
   client->Uninitialize();
-  client.reset(NULL);
+  client = NULL;
 }
 
 
@@ -1158,7 +1158,8 @@ class CFACMockTest : public testing::Test {
   scoped_ptr<AutomationHandleTracker> tracker_;
   MockAutomationMessageSender dummy_sender_;
   scoped_refptr<TabProxy> tab_;
-  scoped_ptr<ChromeFrameAutomationClient> client_;  // the victim of all tests
+  // the victim of all tests
+  scoped_refptr<ChromeFrameAutomationClient> client_;
 
   std::wstring profile_;
   int timeout_;
@@ -1210,7 +1211,7 @@ class CFACMockTest : public testing::Test {
     dummy_sender_.ForwardTo(&proxy_);
     tracker_.reset(new AutomationHandleTracker(&dummy_sender_));
 
-    client_.reset(new ChromeFrameAutomationClient);
+    client_ = new ChromeFrameAutomationClient;
     client_->set_proxy_factory(&factory_);
   }
 };
diff --git a/chrome_frame/urlmon_url_request.h b/chrome_frame/urlmon_url_request.h
index eb13d17..c95f47d 100644
--- a/chrome_frame/urlmon_url_request.h
+++ b/chrome_frame/urlmon_url_request.h
@@ -23,7 +23,7 @@
 #include "net/base/upload_data.h"
 
 class UrlmonUrlRequest
-    : public CComObjectRootEx<CComSingleThreadModel>,
+    : public CComObjectRootEx<CComMultiThreadModel>,
       public PluginUrlRequest,
       public IServiceProviderImpl<UrlmonUrlRequest>,
       public IBindStatusCallback,