Arrange so ExtensionMessageService::RendererReady is called on the IO thread.

This fixes a bug where it wouldn't notice when a renderer died because it was
observing the notification on the wrong thread.  This also simplifies some
logic in ExtensionMessageService.
Review URL: http://codereview.chromium.org/88002

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

3 files changed, 62 insertions, 65 deletions

diff --git a/chrome/browser/extensions/extension_message_service.cc b/chrome/browser/extensions/extension_message_service.cc
index a49cf47..ed6fab8 100755
--- a/chrome/browser/extensions/extension_message_service.cc
+++ b/chrome/browser/extensions/extension_message_service.cc
@@ -59,12 +59,12 @@ ExtensionMessageService* ExtensionMessageService::GetInstance(
 }
 
 ExtensionMessageService::ExtensionMessageService()
-    : next_port_id_(0) {
+    : next_port_id_(0), observing_renderer_shutdown_(false) {
 }
 
 void ExtensionMessageService::RegisterExtension(
     const std::string& extension_id, int render_process_id) {
-  AutoLock lock(renderers_lock_);
+  AutoLock lock(process_ids_lock_);
   DCHECK(process_ids_.find(extension_id) == process_ids_.end() ||
          process_ids_[extension_id] == render_process_id);
   process_ids_[extension_id] = render_process_id;
@@ -76,20 +76,22 @@ int ExtensionMessageService::OpenChannelToExtension(
          ChromeThread::GetMessageLoop(ChromeThread::IO));
 
   // Lookup the targeted extension process.
-  ResourceMessageFilter* dest = NULL;
+  int process_id;
   {
-    AutoLock lock(renderers_lock_);
-    ProcessIDMap::iterator process_id = process_ids_.find(
+    AutoLock lock(process_ids_lock_);
+    ProcessIDMap::iterator process_id_it = process_ids_.find(
         StringToLowerASCII(extension_id));
-    if (process_id == process_ids_.end())
-      return -1;
-
-    RendererMap::iterator renderer = renderers_.find(process_id->second);
-    if (renderer == renderers_.end())
+    if (process_id_it == process_ids_.end())
       return -1;
-    dest = renderer->second;
+    process_id = process_id_it->second;
   }
 
+  RendererMap::iterator renderer = renderers_.find(process_id);
+  if (renderer == renderers_.end())
+    return -1;
+
+  ResourceMessageFilter* dest = renderer->second;
+
   // Create a channel ID for both sides of the channel.
   int port1_id = next_port_id_++;
   int port2_id = next_port_id_++;
@@ -145,68 +147,50 @@ void ExtensionMessageService::DispatchEventToRenderers(
     return;
   }
 
-  // TODO(mpcomplete): this set should probably just be a member var.
-  std::set<ResourceMessageFilter*> renderer_set;
-  {
-    ProcessIDMap::iterator it;
-    AutoLock lock(renderers_lock_);
-
-    for (it = process_ids_.begin(); it != process_ids_.end(); it++) {
-      RendererMap::iterator renderer = renderers_.find(it->second);
-      if (renderer != renderers_.end())
-        renderer_set.insert(renderer->second);
-    }
-  }
-
+  // TODO(mpcomplete): we should only send messages to extension process
+  // renderers.
   std::set<ResourceMessageFilter*>::iterator renderer;
-  for (renderer = renderer_set.begin(); renderer != renderer_set.end();
-       ++renderer) {
+  for (renderer = renderers_unique_.begin();
+       renderer != renderers_unique_.end(); ++renderer) {
     (*renderer)->Send(new ViewMsg_ExtensionHandleEvent(event_name, event_args));
   }
 }
 
-void ExtensionMessageService::RendererReady(ResourceMessageFilter* filter) {
-  AutoLock lock(renderers_lock_);
-  DCHECK(renderers_.find(filter->GetProcessId()) == renderers_.end());
-  renderers_[filter->GetProcessId()] = filter;
+void ExtensionMessageService::RendererReady(ResourceMessageFilter* renderer) {
+  DCHECK(MessageLoop::current() ==
+       ChromeThread::GetMessageLoop(ChromeThread::IO));
+
+  DCHECK(renderers_.find(renderer->GetProcessId()) == renderers_.end());
+  renderers_[renderer->GetProcessId()] = renderer;
+  renderers_unique_.insert(renderer);
 
-  // Only observe this filter if we haven't seen it before.
-  if (filters_.find(filter) == filters_.end()) {
-    filters_.insert(filter);
+  if (!observing_renderer_shutdown_) {
+    observing_renderer_shutdown_ = true;
     NotificationService::current()->AddObserver(
         this,
         NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN,
-        Source<ResourceMessageFilter>(filter));
+        NotificationService::AllSources());
   }
 }
 
 void ExtensionMessageService::Observe(NotificationType type,
                                       const NotificationSource& source,
                                       const NotificationDetails& details) {
+  DCHECK(MessageLoop::current() ==
+       ChromeThread::GetMessageLoop(ChromeThread::IO));
+
   DCHECK(type.value == NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN);
-  ResourceMessageFilter* filter = Source<ResourceMessageFilter>(source).ptr();
+  ResourceMessageFilter* renderer = Source<ResourceMessageFilter>(source).ptr();
 
-  {
-    AutoLock lock(renderers_lock_);
-    DCHECK(renderers_.find(filter->GetProcessId()) != renderers_.end());
-    renderers_.erase(filter->GetProcessId());
-  }
+  renderers_.erase(renderer->GetProcessId());
+  renderers_unique_.erase(renderer);
 
-  // Close any channels that share this filter.
+  // Close any channels that share this renderer.
   // TODO(mpcomplete): should we notify the other side of the port?
   for (MessageChannelMap::iterator it = channels_.begin();
        it != channels_.end(); ) {
     MessageChannelMap::iterator current = it++;
-    if (current->second.port1 == filter || current->second.port2 == filter)
+    if (current->second.port1 == renderer || current->second.port2 == renderer)
       channels_.erase(current);
   }
-
-  std::set<ResourceMessageFilter*>::iterator fit = filters_.find(filter);
-  if (fit != filters_.end()) {
-    filters_.erase(fit);
-    NotificationService::current()->RemoveObserver(
-        this,
-        NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN,
-        source);
-  }
 }
diff --git a/chrome/browser/extensions/extension_message_service.h b/chrome/browser/extensions/extension_message_service.h
index d4eb856..0c8d63f 100755
--- a/chrome/browser/extensions/extension_message_service.h
+++ b/chrome/browser/extensions/extension_message_service.h
@@ -52,16 +52,16 @@ class ExtensionMessageService : public NotificationObserver {
   void PostMessageFromRenderer(int port_id, const std::string& message,
                                ResourceMessageFilter* source);
 
-  // --- UI or IO thread:
-
   // Called to let us know that a renderer has been started.
-  void RendererReady(ResourceMessageFilter* filter);
+  void RendererReady(ResourceMessageFilter* renderer);
 
   // NotificationObserver interface.
   void Observe(NotificationType type,
                const NotificationSource& source,
                const NotificationDetails& details);
 
+  // --- UI or IO thread:
+
   // Send an event to every registered extension renderer.
   void DispatchEventToRenderers(
       const std::string& event_name, const std::string& event_args);
@@ -71,15 +71,10 @@ class ExtensionMessageService : public NotificationObserver {
   typedef std::map<std::string, int> ProcessIDMap;
   ProcessIDMap process_ids_;
 
-  // A map of render_process_id to its corresponding message filter, which we
-  // use for sending messages.
-  typedef std::map<int, ResourceMessageFilter*> RendererMap;
-  RendererMap renderers_;
-
-  // Protects the two maps above, since each can be accessed on the IO thread
+  // Protects the process_ids map, since it can be accessed on the IO thread
   // or UI thread.  Be careful not to hold this lock when calling external
   // code (especially sending messages) to avoid deadlock.
-  Lock renderers_lock_;
+  Lock process_ids_lock_;
 
   // --- IO thread only:
 
@@ -96,8 +91,16 @@ class ExtensionMessageService : public NotificationObserver {
   // For generating unique channel IDs.
   int next_port_id_;
 
-  // For tracking the ResourceMessageFilters we are observing.
-  std::set<ResourceMessageFilter*> filters_;
+  // A map of render_process_id to its corresponding message filter, which we
+  // use for sending messages.
+  typedef std::map<int, ResourceMessageFilter*> RendererMap;
+  RendererMap renderers_;
+
+  // A unique list of renderers that we are aware of.
+  std::set<ResourceMessageFilter*> renderers_unique_;
+
+  // Set to true when we start observing this notification.
+  bool observing_renderer_shutdown_;
 };
 
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_MESSAGE_SERVICE_H_
diff --git a/chrome/browser/renderer_host/resource_message_filter.cc b/chrome/browser/renderer_host/resource_message_filter.cc
index 71ae0d8..b528819 100644
--- a/chrome/browser/renderer_host/resource_message_filter.cc
+++ b/chrome/browser/renderer_host/resource_message_filter.cc
@@ -161,8 +161,6 @@ void ResourceMessageFilter::Init(int render_process_id) {
   render_process_id_ = render_process_id;
   render_widget_helper_->Init(render_process_id, resource_dispatcher_host_);
   app_cache_dispatcher_host_->Initialize(this);
-  ExtensionMessageService::GetInstance(request_context_.get())->
-      RendererReady(this);
 }
 
 // Called on the IPC thread:
@@ -184,9 +182,21 @@ void ResourceMessageFilter::OnChannelConnected(int32 peer_pid) {
     NOTREACHED();
   }
   set_handle(peer_handle);
+
+  // Set the process ID if Init hasn't been called yet.  This doesn't work in
+  // single-process mode since peer_pid won't be the special fake PID we use
+  // for RenderProcessHost in that mode, so we just have to hope that Init
+  // is called first in that case.
+  if (render_process_id_ == -1)
+    render_process_id_ = peer_pid;
+
   // Hook AudioRendererHost to this object after channel is connected so it can
   // this object for sending messages.
   audio_renderer_host_->IPCChannelConnected(render_process_id_, handle(), this);
+
+  // Ditto for the ExtensionMessageService.
+  ExtensionMessageService::GetInstance(request_context_.get())->
+      RendererReady(this);
 }
 
 // Called on the IPC thread: