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

ceee/ie/broker/executors_manager.cc

 // Copyright (c) 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.
 //
 // ExecutorsManager implementation.
 
 #include "ceee/ie/broker/executors_manager.h"
 
 #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"
 
 namespace {
 
 // The timeout we set before accepting a failure when we wait for events.
 const DWORD kTimeOut = 20000;
 
 // Utility class to ensure the setting of an event before we exit a method.
 class AutoSetEvent {
@@ skipping 164 matching lines @@
   }
 
   // Update the list of handles that our thread is waiting on.
   BOOL success = ::SetEvent(update_threads_list_gate_);
   DCHECK(success);
   return S_OK;
 }
 
 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.
   CHandle executor_registration_gate;
 
   // We need to remember if we must create a new one or not.
   // But we must create the executor creator outside of the lock.
   bool create_executor = false;
   {
     AutoLock lock(lock_);
@@ skipping 187 matching lines @@
   HWND frame_window = window_utils::GetTopLevelParent(handle);
   // There are cases where it's too late to get the parent,
   // so we will need to dig for it deeper later.
   if (!common_api::CommonApiResult::IsIeFrameClass(frame_window))
     frame_window = NULL;
 
   bool send_on_removed = false;
   AutoLock lock(lock_);
   {
     HandleMap::iterator handle_it = handle_map_.find(handle);
-    DCHECK(handle_map_.end() != handle_it);
+    // Don't DCHECK, we may be called more than once, but it's fine,
+    // we just ignore subsequent calls.
     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
         tab_id_map_[handle_it->second] =
             reinterpret_cast<HWND>(INVALID_HANDLE_VALUE);
         handle_map_[handle] = kInvalidChromeSessionId;
 #else
         tab_id_map_.erase(handle_it->second);
         handle_map_.erase(handle);
 #endif  // DEBUG
@@ skipping 41 matching lines @@
       frame_window_families_.erase(frame_window);
       send_on_removed = true;
     }
   }  // End of lock
 
   // Safer to send notifications outside of our lock.
   if (send_on_removed)
     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);
 #ifdef DEBUG
   return it != tab_id_map_.end() && it->second != INVALID_HANDLE_VALUE;
 #else
   return it != tab_id_map_.end();
 #endif
 }
 
@@ skipping 113 matching lines @@
     size_t num_handles = num_threads + kExtraHandles;
     DWORD result = me->WaitForMultipleObjects(num_handles, handles, FALSE,
                                               INFINITE);
     if (result == WAIT_OBJECT_0 + num_threads +
                   kUpdateHandleIndexOffset) {
       // We got a new thread added,
       // simply let the loop turn to add it to our watch list.
     } else if (result >= WAIT_OBJECT_0 &&
                result < WAIT_OBJECT_0 + num_threads) {
       // One of our threads have died, cleanup time.
-      me->RemoveExecutor(thread_ids[result - WAIT_OBJECT_0]);
+      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

[Sigurður Ásgeirsson, 2010/12/03 20:40:42]

Interesting. 
Doesn't this leave us with another race, where we enumerate and find the id of a
now-defunct tab? I guess that's another inherent race, though, as enumeration
may always return stuff that's now defunct.

+      // 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));
+      }
     } else if (result == WAIT_OBJECT_0 + num_threads +
                kTerminationHandleIndexOffset) {
       // we are being terminated, break the cycle.
       break;
     } else {
       DCHECK(result == WAIT_FAILED);
       LOG(ERROR) << "ExecutorsManager::ThreadProc " << com::LogWe();
       break;
     }
   }
   // Merci... Bonsoir...
   ::CoUninitialize();
   return 1;
 }