Refactor IDMap to support safe removing of elements during iteration.

TEST=Covered by unit_tests and other automated tests.
http://crbug.com/19202

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

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

13 files changed, 80 insertions, 90 deletions

diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index ad61077..905bf11 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -128,11 +128,12 @@ class BrowserIdleTimer : public base::IdleTimer {
     process.ReduceWorkingSet();
 
     // Handle the Renderer(s).
-    RenderProcessHost::iterator renderer_iter;
-    for (renderer_iter = RenderProcessHost::begin(); renderer_iter !=
-         RenderProcessHost::end(); renderer_iter++) {
-       base::Process process = renderer_iter->second->process();
-       process.ReduceWorkingSet();
+    RenderProcessHost::iterator renderer_iter(
+        RenderProcessHost::AllHostsIterator());
+    while (!renderer_iter.IsAtEnd()) {
+      base::Process process = renderer_iter.GetCurrentValue()->process();
+      process.ReduceWorkingSet();
+      renderer_iter.Advance();
     }
 
     // Handle the child processe.  We need to iterate through them on the IO
diff --git a/chrome/browser/browser_shutdown.cc b/chrome/browser/browser_shutdown.cc
index b67542d..c762cc2 100644
--- a/chrome/browser/browser_shutdown.cc
+++ b/chrome/browser/browser_shutdown.cc
@@ -66,18 +66,19 @@ void OnShutdownStarting(ShutdownType type) {
     // shutdown path for the ones that didn't exit here.
     shutdown_num_processes_slow_ = 0;
     size_t start_rph_size = RenderProcessHost::size();
-    for (RenderProcessHost::iterator hosts = RenderProcessHost::begin();
-         hosts != RenderProcessHost::end();
-         ++hosts) {
-      RenderProcessHost* rph = hosts->second;
-      if (!rph->FastShutdownIfPossible())
+    RenderProcessHost::iterator hosts(RenderProcessHost::AllHostsIterator());
+    while (!hosts.IsAtEnd()) {
+      if (!hosts.GetCurrentValue()->FastShutdownIfPossible()) {
         // TODO(ojan): I think now that we deal with beforeunload/unload
         // higher up, it's not possible to get here. Confirm this and change
         // FastShutdownIfPossible to just be FastShutdown.
         shutdown_num_processes_slow_++;
+      }
       // The number of RPHs should not have changed as the result of invoking
       // FastShutdownIfPossible.
       CHECK(start_rph_size == RenderProcessHost::size());
+
+      hosts.Advance();
     }
   }
 }
diff --git a/chrome/browser/chrome_plugin_browsing_context.cc b/chrome/browser/chrome_plugin_browsing_context.cc
index 71dc80d..504c5c0 100644
--- a/chrome/browser/chrome_plugin_browsing_context.cc
+++ b/chrome/browser/chrome_plugin_browsing_context.cc
@@ -58,16 +58,9 @@ void CPBrowsingContextManager::Observe(NotificationType type,
   URLRequestContext* context = Source<URLRequestContext>(source).ptr();
 
   // Multiple CPBrowsingContexts may refer to the same URLRequestContext.
-  // Remove after collecting all entries, since removing may invalidate
-  // iterators.
-  std::vector<int32> ids_to_remove;
-  for (Map::const_iterator it = map_.begin(); it != map_.end(); ++it) {
-    if (it->second == context)
-      ids_to_remove.push_back(it->first);
-  }
-
-  for (size_t i = 0; i < ids_to_remove.size(); ++i) {
-    map_.Remove(ids_to_remove[i]);
+  for (Map::iterator it(&map_); !it.IsAtEnd(); it.Advance()) {
+    if (it.GetCurrentValue() == context)
+      map_.Remove(it.GetCurrentKey());
   }
 
   reverse_map_.erase(context);
diff --git a/chrome/browser/memory_details.cc b/chrome/browser/memory_details.cc
index ccf279e..df81bc3 100644
--- a/chrome/browser/memory_details.cc
+++ b/chrome/browser/memory_details.cc
@@ -196,13 +196,13 @@ void MemoryDetails::CollectChildInfoOnUIThread() {
     // check if it's a diagnostics-related process.  We skip all diagnostics
     // pages (e.g. "about:xxx" URLs).  Iterate the RenderProcessHosts to find
     // the tab contents.
-    RenderProcessHost::iterator renderer_iter;
-    for (renderer_iter = RenderProcessHost::begin(); renderer_iter !=
-         RenderProcessHost::end(); ++renderer_iter) {
-      DCHECK(renderer_iter->second);
+    RenderProcessHost::iterator renderer_iter(
+        RenderProcessHost::AllHostsIterator());
+    for (; !renderer_iter.IsAtEnd(); renderer_iter.Advance()) {
+      DCHECK(renderer_iter.GetCurrentValue());
       ProcessMemoryInformation& process =
           process_data_[CHROME_BROWSER].processes[index];
-      if (process.pid != renderer_iter->second->process().pid())
+      if (process.pid != renderer_iter.GetCurrentValue()->process().pid())
         continue;
       process.type = ChildProcessInfo::RENDER_PROCESS;
       // The RenderProcessHost may host multiple TabContents.  Any
@@ -212,16 +212,16 @@ void MemoryDetails::CollectChildInfoOnUIThread() {
       // NOTE: This is a bit dangerous.  We know that for now, listeners
       //       are always RenderWidgetHosts.  But in theory, they don't
       //       have to be.
-      RenderProcessHost::listeners_iterator iter;
-      for (iter = renderer_iter->second->listeners_begin();
-           iter != renderer_iter->second->listeners_end(); ++iter) {
-        RenderWidgetHost* widget =
-            static_cast<RenderWidgetHost*>(iter->second);
+      RenderProcessHost::listeners_iterator iter(
+          renderer_iter.GetCurrentValue()->ListenersIterator());
+      for (; !iter.IsAtEnd(); iter.Advance()) {
+        const RenderWidgetHost* widget =
+            static_cast<const RenderWidgetHost*>(iter.GetCurrentValue());
         DCHECK(widget);
         if (!widget || !widget->IsRenderView())
           continue;
 
-        RenderViewHost* host = static_cast<RenderViewHost*>(widget);
+        const RenderViewHost* host = static_cast<const RenderViewHost*>(widget);
         TabContents* contents = NULL;
         if (host->delegate())
           contents = host->delegate()->GetAsTabContents();
diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc
index 71b162c..bd45fb8 100644
--- a/chrome/browser/metrics/metrics_service.cc
+++ b/chrome/browser/metrics/metrics_service.cc
@@ -957,9 +957,10 @@ void MetricsService::LogTransmissionTimerDone() {
   details->StartFetch();
 
   // Collect WebCore cache information to put into a histogram.
-  for (RenderProcessHost::iterator it = RenderProcessHost::begin();
-       it != RenderProcessHost::end(); ++it) {
-    it->second->Send(new ViewMsg_GetCacheResourceStats());
+  RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());
+  while (!it.IsAtEnd()) {
+    it.GetCurrentValue()->Send(new ViewMsg_GetCacheResourceStats());
+    it.Advance();
   }
 }
 
diff --git a/chrome/browser/plugin_service.cc b/chrome/browser/plugin_service.cc
index fe6dfa6..c3e1b14 100644
--- a/chrome/browser/plugin_service.cc
+++ b/chrome/browser/plugin_service.cc
@@ -196,11 +196,11 @@ void PluginService::OnWaitableEventSignaled(base::WaitableEvent* waitable_event)
 
   NPAPI::PluginList::Singleton()->ResetPluginsLoaded();
 
-  for (RenderProcessHost::iterator it = RenderProcessHost::begin();
-       it != RenderProcessHost::end(); ++it) {
-    it->second->Send(new ViewMsg_PurgePluginListCache());
+  for (RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());
+       !it.IsAtEnd(); it.Advance()) {
+    it.GetCurrentValue()->Send(new ViewMsg_PurgePluginListCache());
   }
-#endif
+#endif  // defined(OS_WIN)
 }
 
 void PluginService::Observe(NotificationType type,
diff --git a/chrome/browser/renderer_host/browser_render_process_host.cc b/chrome/browser/renderer_host/browser_render_process_host.cc
index 0a7c12b..a0b64c1 100644
--- a/chrome/browser/renderer_host/browser_render_process_host.cc
+++ b/chrome/browser/renderer_host/browser_render_process_host.cc
@@ -649,17 +649,20 @@ bool BrowserRenderProcessHost::FastShutdownIfPossible() {
 
   // Check for any external tab containers, since they may still be running even
   // though this window closed.
-  BrowserRenderProcessHost::listeners_iterator iter;
-  // NOTE: This is a bit dangerous.  We know that for now, listeners are
-  // always RenderWidgetHosts.  But in theory, they don't have to be.
-  for (iter = listeners_begin(); iter != listeners_end(); ++iter) {
-    RenderWidgetHost* widget = static_cast<RenderWidgetHost*>(iter->second);
+  BrowserRenderProcessHost::listeners_iterator iter(ListenersIterator());
+  while (!iter.IsAtEnd()) {
+    // NOTE: This is a bit dangerous.  We know that for now, listeners are
+    // always RenderWidgetHosts.  But in theory, they don't have to be.
+    const RenderWidgetHost* widget =
+        static_cast<const RenderWidgetHost*>(iter.GetCurrentValue());
     DCHECK(widget);
-    if (!widget || !widget->IsRenderView())
-      continue;
-    RenderViewHost* rvh = static_cast<RenderViewHost*>(widget);
-    if (rvh->delegate()->IsExternalTabContainer())
-      return false;
+    if (widget && widget->IsRenderView()) {
+      const RenderViewHost* rvh = static_cast<const RenderViewHost*>(widget);
+      if (rvh->delegate()->IsExternalTabContainer())
+        return false;
+    }
+
+    iter.Advance();
   }
 
   // Otherwise, we're allowed to just terminate the process. Using exit code 0
@@ -876,13 +879,11 @@ void BrowserRenderProcessHost::OnChannelError() {
 
   channel_.reset();
 
-  // This process should detach all the listeners, causing the object to be
-  // deleted. We therefore need a stack copy of the web view list to avoid
-  // crashing when checking for the termination condition the last time.
-  IDMap<IPC::Channel::Listener> local_listeners(listeners_);
-  for (listeners_iterator i = local_listeners.begin();
-       i != local_listeners.end(); ++i) {
-    i->second->OnMessageReceived(ViewHostMsg_RenderViewGone(i->first));
+  IDMap<IPC::Channel::Listener>::iterator iter(&listeners_);
+  while (!iter.IsAtEnd()) {
+    iter.GetCurrentValue()->OnMessageReceived(
+        ViewHostMsg_RenderViewGone(iter.GetCurrentKey()));
+    iter.Advance();
   }
 
   ClearTransportDIBCache();
diff --git a/chrome/browser/renderer_host/render_process_host.cc b/chrome/browser/renderer_host/render_process_host.cc
index 090ed91..4f0cbc0 100644
--- a/chrome/browser/renderer_host/render_process_host.cc
+++ b/chrome/browser/renderer_host/render_process_host.cc
@@ -125,13 +125,8 @@ void RenderProcessHost::UpdateMaxPageID(int32 page_id) {
 }
 
 // static
-RenderProcessHost::iterator RenderProcessHost::begin() {
-  return all_hosts.begin();
-}
-
-// static
-RenderProcessHost::iterator RenderProcessHost::end() {
-  return all_hosts.end();
+RenderProcessHost::iterator RenderProcessHost::AllHostsIterator() {
+  return iterator(&all_hosts);
 }
 
 // static
@@ -165,10 +160,13 @@ RenderProcessHost* RenderProcessHost::GetExistingProcessHost(Profile* profile,
   std::vector<RenderProcessHost*> suitable_renderers;
   suitable_renderers.reserve(size());
 
-  for (iterator iter = begin(); iter != end(); ++iter) {
+  iterator iter(AllHostsIterator());
+  while (!iter.IsAtEnd()) {
     if (run_renderer_in_process() ||
-        IsSuitableHost(iter->second, profile, type))
-      suitable_renderers.push_back(iter->second);
+        IsSuitableHost(iter.GetCurrentValue(), profile, type))
+      suitable_renderers.push_back(iter.GetCurrentValue());
+
+    iter.Advance();
   }
 
   // Now pick a random suitable renderer, if we have any.
diff --git a/chrome/browser/renderer_host/render_process_host.h b/chrome/browser/renderer_host/render_process_host.h
index aee189c..c906235 100644
--- a/chrome/browser/renderer_host/render_process_host.h
+++ b/chrome/browser/renderer_host/render_process_host.h
@@ -28,7 +28,7 @@ struct ViewMsg_ClosePage_Params;
 class RenderProcessHost : public IPC::Channel::Sender,
                           public IPC::Channel::Listener {
  public:
-  typedef IDMap<RenderProcessHost>::const_iterator iterator;
+  typedef IDMap<RenderProcessHost>::iterator iterator;
 
   // We classify renderers according to their highest privilege, and try
   // to group pages into renderers with similar privileges.
@@ -83,11 +83,9 @@ class RenderProcessHost : public IPC::Channel::Sender,
   // Allows iteration over this RenderProcessHost's RenderViewHost listeners.
   // Use from UI thread only.
   typedef IDMap<IPC::Channel::Listener>::const_iterator listeners_iterator;
-  listeners_iterator listeners_begin() {
-    return listeners_.begin();
-  }
-  listeners_iterator listeners_end() {
-    return listeners_.end();
+
+  listeners_iterator ListenersIterator() {
+    return listeners_iterator(&listeners_);
   }
 
   IPC::Channel::Listener* GetListenerByID(int routing_id) {
@@ -195,10 +193,7 @@ class RenderProcessHost : public IPC::Channel::Sender,
 
   // Allows iteration over all the RenderProcessHosts in the browser. Note
   // that each host may not be active, and therefore may have NULL channels.
-  // This is just a standard STL iterator, so it is not valid if the list
-  // of RenderProcessHosts changes between iterations.
-  static iterator begin();
-  static iterator end();
+  static iterator AllHostsIterator();
   static size_t size();  // TODO(brettw) rename this, it's very unclear.
 
   // Returns the RenderProcessHost given its ID.  Returns NULL if the ID does
diff --git a/chrome/browser/renderer_host/render_view_host.cc b/chrome/browser/renderer_host/render_view_host.cc
index 49c4821..8acc403 100644
--- a/chrome/browser/renderer_host/render_view_host.cc
+++ b/chrome/browser/renderer_host/render_view_host.cc
@@ -148,7 +148,7 @@ void RenderViewHost::Observe(NotificationType type,
   if (rph == process()) {
     // Try to get some debugging information on the stack.
     size_t num_hosts = RenderProcessHost::size();
-    bool no_listeners = rph->listeners_begin() == rph->listeners_end();
+    bool no_listeners = rph->ListenersIterator().IsAtEnd();
     bool live_instance = site_instance() != NULL;
     CHECK(live_instance);
     bool live_process = site_instance()->GetProcess() != NULL;
@@ -164,7 +164,7 @@ void RenderViewHost::Observe(NotificationType type,
 bool RenderViewHost::CreateRenderView() {
   DCHECK(!IsRenderViewLive()) << "Creating view twice";
   CHECK(process());
-  CHECK(process()->listeners_begin() != process()->listeners_end()) <<
+  CHECK(!process()->ListenersIterator().IsAtEnd()) <<
       "Our process should have us as a listener.";
 
   // The process may (if we're sharing a process with another host that already
diff --git a/chrome/browser/renderer_host/render_view_host.h b/chrome/browser/renderer_host/render_view_host.h
index 71a15fe..6381ef1 100644
--- a/chrome/browser/renderer_host/render_view_host.h
+++ b/chrome/browser/renderer_host/render_view_host.h
@@ -403,7 +403,7 @@ class RenderViewHost : public RenderWidgetHost,
 
   // RenderWidgetHost public overrides.
   virtual void Shutdown();
-  virtual bool IsRenderView() { return true; }
+  virtual bool IsRenderView() const { return true; }
   virtual void OnMessageReceived(const IPC::Message& msg);
   virtual void GotFocus();
   virtual bool CanBlur() const;
diff --git a/chrome/browser/renderer_host/render_widget_host.h b/chrome/browser/renderer_host/render_widget_host.h
index edace90..3b52e4a 100644
--- a/chrome/browser/renderer_host/render_widget_host.h
+++ b/chrome/browser/renderer_host/render_widget_host.h
@@ -171,7 +171,7 @@ class RenderWidgetHost : public IPC::Channel::Listener,
   virtual void Shutdown();
 
   // Manual RTTI FTW. We are not hosting a web page.
-  virtual bool IsRenderView() { return false; }
+  virtual bool IsRenderView() const { return false; }
 
   // IPC::Channel::Listener
   virtual void OnMessageReceived(const IPC::Message& msg);
diff --git a/chrome/browser/visitedlink_event_listener.cc b/chrome/browser/visitedlink_event_listener.cc
index 6b27929..86fe7d5 100644
--- a/chrome/browser/visitedlink_event_listener.cc
+++ b/chrome/browser/visitedlink_event_listener.cc
@@ -19,20 +19,20 @@ void VisitedLinkEventListener::NewTable(base::SharedMemory* table_memory) {
     return;
 
   // Send to all RenderProcessHosts.
-  for (RenderProcessHost::iterator i = RenderProcessHost::begin();
-       i != RenderProcessHost::end(); ++i) {
-    if (!i->second->HasConnection())
+  for (RenderProcessHost::iterator i(RenderProcessHost::AllHostsIterator());
+       !i.IsAtEnd(); i.Advance()) {
+    if (!i.GetCurrentValue()->HasConnection())
       continue;
 
     base::SharedMemoryHandle new_table;
-    base::ProcessHandle process = i->second->process().handle();
+    base::ProcessHandle process = i.GetCurrentValue()->process().handle();
     if (!process) {
       // process can be null if it's started with the --single-process flag.
       process = base::Process::Current().handle();
     }
 
     table_memory->ShareToProcess(process, &new_table);
-    i->second->Send(new ViewMsg_VisitedLink_NewTable(new_table));
+    i.GetCurrentValue()->Send(new ViewMsg_VisitedLink_NewTable(new_table));
   }
 }
 
@@ -51,17 +51,17 @@ void VisitedLinkEventListener::Reset() {
   coalesce_timer_.Stop();
 
   // Send to all RenderProcessHosts.
-  for (RenderProcessHost::iterator i = RenderProcessHost::begin();
-       i != RenderProcessHost::end(); ++i) {
-    i->second->ResetVisitedLinks();
+  for (RenderProcessHost::iterator i(RenderProcessHost::AllHostsIterator());
+       !i.IsAtEnd(); i.Advance()) {
+    i.GetCurrentValue()->ResetVisitedLinks();
   }
 }
 
 void VisitedLinkEventListener::CommitVisitedLinks() {
   // Send to all RenderProcessHosts.
-  for (RenderProcessHost::iterator i = RenderProcessHost::begin();
-       i != RenderProcessHost::end(); ++i) {
-    i->second->AddVisitedLinks(pending_visited_links_);
+  for (RenderProcessHost::iterator i(RenderProcessHost::AllHostsIterator());
+       !i.IsAtEnd(); i.Advance()) {
+    i.GetCurrentValue()->AddVisitedLinks(pending_visited_links_);
   }
 
   pending_visited_links_.clear();