Do not allow GuestViewManager to (re)use an instance ID that was already removed.

chrome/browser/guest_view/guest_view_manager.cc

Index: chrome/browser/guest_view/guest_view_manager.cc
diff --git a/chrome/browser/guest_view/guest_view_manager.cc b/chrome/browser/guest_view/guest_view_manager.cc
index 833123f4be4954d9a877688aa32d7e7293d5daf3..c57f582d0af8b9cddd87291ee4954b5340109aca 100644
--- a/chrome/browser/guest_view/guest_view_manager.cc
+++ b/chrome/browser/guest_view/guest_view_manager.cc
@@ -60,8 +60,8 @@ class GuestWebContentsObserver
 };
 
 GuestViewManager::GuestViewManager(content::BrowserContext* context)
-    : current_instance_id_(0),
-      context_(context) {}
+    : current_instance_id_(0), last_instance_id_removed_(0), context_(context) {
+}
 
 GuestViewManager::~GuestViewManager() {}
 
@@ -188,13 +188,14 @@ bool GuestViewManager::ForEachGuest(WebContents* embedder_web_contents,
   return false;
 }
 
-void GuestViewManager::AddGuest(int guest_instance_id,
+bool GuestViewManager::AddGuest(int guest_instance_id,
                                 WebContents* guest_web_contents) {
   DCHECK(guest_web_contents_by_instance_id_.find(guest_instance_id) ==
          guest_web_contents_by_instance_id_.end());

[Fady Samuel, 2014/05/22 15:02:13]

Make this a CHECK instead of a DCHECK because it has security implications.

[lazyboy, 2014/05/22 15:59:31]

Done.

   guest_web_contents_by_instance_id_[guest_instance_id] = guest_web_contents;
   // This will add the RenderProcessHost ID when we get one.
   new GuestWebContentsObserver(guest_web_contents);
+  return true;
 }
 
 void GuestViewManager::RemoveGuest(int guest_instance_id) {
@@ -204,6 +205,21 @@ void GuestViewManager::RemoveGuest(int guest_instance_id) {
   render_process_host_id_multiset_.erase(
       it->second->GetRenderProcessHost()->GetID());
   guest_web_contents_by_instance_id_.erase(it);
+
+  if (guest_instance_id == last_instance_id_removed_ + 1) {

[Fady Samuel, 2014/05/22 15:02:13]

This code is hard to read. Could you please comment it. It's not clear to me
what last_instance_id_removed_ actually represents. I worry that this code might
have a bug in it if I don't understand the intent.

[lazyboy, 2014/05/22 15:59:31]

There's a note in last_instance_id_removed_ in .h file, I've added more comments
here.

I worry that this code might
Yes, I've added unit test for this purpose.

+    ++last_instance_id_removed_;
+    // Compact.
+    std::set<int>::iterator iter = removed_instance_ids_.begin();
+    while (iter != removed_instance_ids_.end()) {
+      int instance_id = *iter;
+      if (instance_id != last_instance_id_removed_ + 1)
+        break;
+      ++last_instance_id_removed_;
+      removed_instance_ids_.erase(iter++);
+    }
+  } else {
+    removed_instance_ids_.insert(guest_instance_id);
+  }
 }
 
 void GuestViewManager::AddRenderProcessHostID(int render_process_host_id) {