Fix crash. Happens in LaunchDelegate::LaunchComplete if delegate (ChromeFrameAutomationClient) is destroyed before launch callback is invoked. Happens when underlying automation proxy is going to be shared to new instance of ChromeFrameAutomationClient and we destroy that instance before receiving LaunchComplete callback.

Still have a race (and possible crash) for automation_server_ in ReleaseAutomationServer and LaunchComplete,
TEST=test added.
Review URL: http://codereview.chromium.org/582018

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

3 files changed, 71 insertions, 13 deletions

diff --git a/chrome_frame/chrome_frame_automation.cc b/chrome_frame/chrome_frame_automation.cc
index dce4b78..f505ffe 100644
--- a/chrome_frame/chrome_frame_automation.cc
+++ b/chrome_frame/chrome_frame_automation.cc
@@ -14,6 +14,7 @@
 #include "base/singleton.h"
 #include "base/string_util.h"
 #include "base/sys_info.h"
+#include "base/waitable_event.h"
 #include "chrome/app/client_util.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
@@ -329,37 +330,54 @@ bool ProxyFactory::ReleaseAutomationServer(void* server_id) {
 
   ProxyCacheEntry* entry = reinterpret_cast<ProxyCacheEntry*>(server_id);
 
+#ifndef NDEBUG
   lock_.Acquire();
   Vector::ContainerType::iterator it = std::find(proxies_.container().begin(),
                                                  proxies_.container().end(),
                                                  entry);
   DCHECK(it != proxies_.container().end());
   DCHECK(entry->thread->thread_id() != PlatformThread::CurrentId());
-  if (--entry->ref_count == 0) {
-    proxies_.container().erase(it);
-  }
+  DCHECK_GT(entry->ref_count, 0);
 
   lock_.Release();
+#endif
 
-  // Destroy it.
+  base::WaitableEvent done(true, false);
+  entry->thread->message_loop()->PostTask(FROM_HERE, NewRunnableMethod(this,
+      &ProxyFactory::ReleaseProxy, entry, &done));
+  done.Wait();
+
+  // Stop the thread and destroy the entry if there is no more clients.
   if (entry->ref_count == 0) {
-    entry->thread->message_loop()->PostTask(FROM_HERE, NewRunnableMethod(this,
-        &ProxyFactory::DestroyProxy, entry));
-    // Wait until thread exits
-    entry->thread.reset();
     DCHECK(entry->proxy == NULL);
+    entry->thread.reset();
     delete entry;
   }
 
   return true;
 }
 
-void ProxyFactory::DestroyProxy(ProxyCacheEntry* entry) {
+void ProxyFactory::ReleaseProxy(ProxyCacheEntry* entry,
+                                base::WaitableEvent* done) {
   DCHECK(entry->thread->thread_id() == PlatformThread::CurrentId());
+
+  lock_.Acquire();
+  if (!--entry->ref_count) {
+    Vector::ContainerType::iterator it = std::find(proxies_.container().begin(),
+                                                   proxies_.container().end(),
+                                                   entry);
+    proxies_->erase(it);
+  }
+  lock_.Release();
+
   // Send pending UMA data if any.
-  SendUMAData(entry);
-  delete entry->proxy;
-  entry->proxy = NULL;
+  if (!entry->ref_count) {
+    SendUMAData(entry);
+    delete entry->proxy;
+    entry->proxy = NULL;
+  }
+
+  done->Signal();
 }
 
 Singleton<ProxyFactory> g_proxy_factory;
diff --git a/chrome_frame/chrome_frame_automation.h b/chrome_frame/chrome_frame_automation.h
index 4cfa187..8b2f801 100644
--- a/chrome_frame/chrome_frame_automation.h
+++ b/chrome_frame/chrome_frame_automation.h
@@ -122,7 +122,7 @@ class ProxyFactory {
   void CreateProxy(ProxyCacheEntry* entry,
                    const ChromeFrameLaunchParams& params,
                    LaunchDelegate* delegate);
-  void DestroyProxy(ProxyCacheEntry* entry);
+  void ReleaseProxy(ProxyCacheEntry* entry, base::WaitableEvent* done);
 
   void SendUMAData(ProxyCacheEntry* proxy_entry);
 
diff --git a/chrome_frame/test/proxy_factory_mock.cc b/chrome_frame/test/proxy_factory_mock.cc
index a50dc33..2a587f4 100644
--- a/chrome_frame/test/proxy_factory_mock.cc
+++ b/chrome_frame/test/proxy_factory_mock.cc
@@ -1,6 +1,7 @@
 // Copyright (c) 2006-2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
+#include "base/waitable_event.h"
 #include "chrome_frame/test/proxy_factory_mock.h"
 
 #define GMOCK_MUTANT_INCLUDE_LATE_OBJECT_BINDING
@@ -84,3 +85,42 @@ TEST(ProxyFactoryTest, CreateDifferentProfiles) {
   f.ReleaseAutomationServer(i1);
 }
 
+TEST(ProxyFactoryTest, FastCreateDestroy) {
+  ProxyFactory f;
+  LaunchDelegateMock* d1 = new LaunchDelegateMock();
+  LaunchDelegateMock* d2 = new LaunchDelegateMock();
+
+  ChromeFrameLaunchParams params;
+  params.automation_server_launch_timeout = 10000;
+  params.profile_name = L"Dr. Gratiano Forbeson";
+  params.extra_chrome_arguments = L"";
+  params.perform_version_check = false;
+  params.incognito_mode = false;
+
+  void* i1 = NULL;
+  base::WaitableEvent launched(true, false);
+  EXPECT_CALL(*d1, LaunchComplete(testing::NotNull(), AUTOMATION_SUCCESS))
+      .Times(1)
+      .WillOnce(testing::InvokeWithoutArgs(&launched,
+                                           &base::WaitableEvent::Signal));
+  f.GetAutomationServer(d1, params, &i1);
+  // Wait for launch
+  ASSERT_TRUE(launched.TimedWait(base::TimeDelta::FromSeconds(10)));
+
+  // Expect second launch to succeed too
+  EXPECT_CALL(*d2, LaunchComplete(testing::NotNull(), AUTOMATION_SUCCESS))
+      .Times(1);
+
+  // Boost thread priority so we call ReleaseAutomationServer before
+  // LaunchComplete callback have a chance to be executed.
+  ::SetThreadPriority(::GetCurrentThread(), THREAD_PRIORITY_HIGHEST);
+  void* i2 = NULL;
+  f.GetAutomationServer(d2, params, &i2);
+  EXPECT_EQ(i1, i2);
+  f.ReleaseAutomationServer(i2);
+  delete d2;
+
+  ::SetThreadPriority(::GetCurrentThread(), THREAD_PRIORITY_NORMAL);
+  f.ReleaseAutomationServer(i1);
+  delete d1;
+}