Revert 68298 - Make sure we don't keep tab id - handles mapping when a dread dies.

BUG= 65153
TEST=See hard to repro scenario in bug...

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

TBR=mad@google.com
Review URL: http://codereview.chromium.org/5595005

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

4 files changed, 3 insertions, 64 deletions

diff --git a/ceee/ie/broker/chrome_postman.cc b/ceee/ie/broker/chrome_postman.cc
index 9a9562d..bc44023 100644
--- a/ceee/ie/broker/chrome_postman.cc
+++ b/ceee/ie/broker/chrome_postman.cc
@@ -6,12 +6,12 @@
 #include "ceee/ie/broker/chrome_postman.h"
 
 #include "base/string_util.h"
-#include "ceee/common/com_utils.h"
 #include "ceee/ie/broker/api_dispatcher.h"
 #include "ceee/ie/common/api_registration.h"
 #include "ceee/ie/common/ceee_module_util.h"
 #include "chrome/browser/automation/extension_automation_constants.h"
 #include "chrome/common/url_constants.h"
+#include "ceee/common/com_utils.h"
 
 namespace ext = extension_automation_constants;
 
@@ -121,16 +121,6 @@ void ChromePostman::PostMessage(BSTR message, BSTR target) {
   }
 }
 
-void ChromePostman::QueueApiInvocationThreadTask(Task* task) {
-  MessageLoop* message_loop = api_worker_thread_.message_loop();
-  if (message_loop) {
-    message_loop->PostTask(FROM_HERE, task);
-  } else {
-    LOG(ERROR) << "Trying to queue a task before the API worker thread is"
-                  "completely initialized and ready.";
-  }
-}
-
 void ChromePostman::FireEvent(const char* event_name, const char* event_args) {
   MessageLoop* message_loop = api_worker_thread_.message_loop();
   if (message_loop) {
diff --git a/ceee/ie/broker/chrome_postman.h b/ceee/ie/broker/chrome_postman.h
index 33d9418..11c05b8 100644
--- a/ceee/ie/broker/chrome_postman.h
+++ b/ceee/ie/broker/chrome_postman.h
@@ -45,9 +45,6 @@ class ChromePostman
   // This posts a tasks to the Api Invocation thread to fire the given event.
   virtual void FireEvent(const char* event_name, const char* event_args);
 
-  // This queues a generic tasks to be executed in the Api Invocation thread.
-  virtual void QueueApiInvocationThreadTask(Task* task);
-
   // This creates both an STA and an MTA threads. We must make sure we only call
   // Chrome Frame from this STA. And since we can't block Chrome Frame we use
   // the MTA thread to executes API Invocation we receive from Chrome Frame.
diff --git a/ceee/ie/broker/executors_manager.cc b/ceee/ie/broker/executors_manager.cc
index a221eb6..84d3c37 100644
--- a/ceee/ie/broker/executors_manager.cc
+++ b/ceee/ie/broker/executors_manager.cc
@@ -8,7 +8,6 @@
 
 #include "base/logging.h"
 #include "ceee/ie/broker/broker_module_util.h"
-#include "ceee/ie/broker/chrome_postman.h"
 #include "ceee/ie/broker/common_api_module.h"
 #include "ceee/common/com_utils.h"
 
@@ -193,7 +192,6 @@ HRESULT ExecutorsManager::RegisterWindowExecutor(ThreadId thread_id,
 
 HRESULT ExecutorsManager::GetExecutor(ThreadId thread_id, HWND window,
                                       REFIID riid, void** executor) {
-  VLOG(1) << "Thread: " << thread_id << ", window: " << window;
   DCHECK(executor != NULL);
   // We may need to wait for either a currently pending
   // or own newly created registration of a new executor.
@@ -412,11 +410,9 @@ void ExecutorsManager::DeleteTabHandle(HWND handle) {
   AutoLock lock(lock_);
   {
     HandleMap::iterator handle_it = handle_map_.find(handle);
-    // Don't DCHECK, we may be called more than once, but it's fine,
-    // we just ignore subsequent calls.
+    DCHECK(handle_map_.end() != handle_it);
     if (handle_map_.end() != handle_it) {
       TabIdMap::iterator tab_id_it = tab_id_map_.find(handle_it->second);
-      // But if we have it in one map, we must have it in the reverse map.
       DCHECK(tab_id_map_.end() != tab_id_it);
       if (tab_id_map_.end() != tab_id_it) {
 #ifdef DEBUG
@@ -478,31 +474,6 @@ void ExecutorsManager::DeleteTabHandle(HWND handle) {
     windows_events_funnel().OnRemoved(frame_window);
 }
 
-// This is for using CleanupMapsForThread() into a runnable object without
-// the need to AddRef/Release the ExecutorsManager which is a Singleton.
-DISABLE_RUNNABLE_METHOD_REFCOUNT(ExecutorsManager);
-
-void ExecutorsManager::CleanupMapsForThread(DWORD thread_id) {
-  // We collect the tab handles to remove so that we can do it outside of lock.
-  // This is because 1) it's not trivial to do it while we loop through the map,
-  // since there are multiple maps to deal with and the DeleteHandle takes care
-  // of that for us, and 2) we can't call DeleteHandle from within a lock.
-  std::vector<HWND> tab_handles_to_remove;
-  {
-    AutoLock lock(lock_);
-    HandleMap::iterator handle_it = handle_map_.begin();
-    for (; handle_it != handle_map_.end(); ++ handle_it) {
-      if (::GetWindowThreadProcessId(handle_it->first, NULL) == thread_id)
-        tab_handles_to_remove.push_back(handle_it->first);
-    }
-  }  // AutoLock end
-
-  std::vector<HWND>::iterator tab_handle_it = tab_handles_to_remove.begin();
-  for (; tab_handle_it != tab_handles_to_remove.end(); ++tab_handle_it) {
-    DeleteTabHandle(*tab_handle_it);
-  }
-}
-
 bool ExecutorsManager::IsTabIdValid(int tab_id) {
   AutoLock lock(lock_);
   TabIdMap::const_iterator it = tab_id_map_.find(tab_id);
@@ -636,21 +607,7 @@ DWORD ExecutorsManager::ThreadProc(LPVOID parameter) {
     } else if (result >= WAIT_OBJECT_0 &&
                result < WAIT_OBJECT_0 + num_threads) {
       // One of our threads have died, cleanup time.
-      DWORD thread_id = thread_ids[result - WAIT_OBJECT_0];
-      VLOG(1) << "Thread: " << thread_id << " is dying on us!";
-      me->RemoveExecutor(thread_id);
-      // We must asynchronously post this change in case there are pending
-      // asynchronous notifications (e.g., tabs.onRemoved) that would still
-      // need the handle to tab id mapping.
-      // NOTE: No need to worry about the lifespan of the executors manager
-      // referred by the me variable since it's a singleton released at exit.
-      ChromePostman* single_instance = ChromePostman::GetInstance();
-      // This is not intialized in unittests yet.
-      if (single_instance) {
-        single_instance->QueueApiInvocationThreadTask(
-            NewRunnableMethod(me, &ExecutorsManager::CleanupMapsForThread,
-                              thread_id));
-      }
+      me->RemoveExecutor(thread_ids[result - WAIT_OBJECT_0]);
     } else if (result == WAIT_OBJECT_0 + num_threads +
                kTerminationHandleIndexOffset) {
       // we are being terminated, break the cycle.
diff --git a/ceee/ie/broker/executors_manager.h b/ceee/ie/broker/executors_manager.h
index 48a15f9..a51e2da 100644
--- a/ceee/ie/broker/executors_manager.h
+++ b/ceee/ie/broker/executors_manager.h
@@ -16,7 +16,6 @@
 
 #include "base/lock.h"
 #include "base/singleton.h"
-#include "base/task.h"
 #include "ceee/common/window_utils.h"
 #include "ceee/ie/broker/window_events_funnel.h"
 
@@ -121,10 +120,6 @@ class ExecutorsManager {
   // Unregister the HWND and its corresponding tab ID and tool band tab ID.
   virtual void DeleteTabHandle(HWND handle);
 
-  // Cleans up the maps from all handles that would be associated to the given
-  // thread id.
-  virtual void CleanupMapsForThread(DWORD thread_id);
-
   // Traits for Singleton<ExecutorsManager> so that we can pass an argument
   // to the constructor.
   struct SingletonTraits : public DefaultSingletonTraits<ExecutorsManager> {