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

chrome/browser/guest_view/guest_view_manager.cc

 // Copyright 2014 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.
 
 #include "chrome/browser/guest_view/guest_view_manager.h"
 
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/guest_view/guest_view_base.h"
 #include "chrome/browser/guest_view/guest_view_constants.h"
@@ skipping 42 matching lines @@
 
   virtual void WebContentsDestroyed() OVERRIDE {
     delete this;
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(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() {}
 
 // static.
 GuestViewManager* GuestViewManager::FromBrowserContext(
     BrowserContext* context) {
   GuestViewManager* guest_manager =
       static_cast<GuestViewManager*>(context->GetUserData(
           guestview::kGuestViewManagerKeyName));
   if (!guest_manager) {
@@ skipping 106 matching lines @@
     GuestViewBase* guest_view = GuestViewBase::FromWebContents(guest);
     if (embedder_web_contents != guest_view->embedder_web_contents())
       continue;
 
     if (callback.Run(guest))
       return true;
   }
   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) {
   GuestInstanceMap::iterator it =
       guest_web_contents_by_instance_id_.find(guest_instance_id);
   DCHECK(it != guest_web_contents_by_instance_id_.end());
   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) {
   render_process_host_id_multiset_.insert(render_process_host_id);
 }
 
 content::WebContents* GuestViewManager::GetGuestByInstanceID(
     int guest_instance_id,
     int embedder_render_process_id) {
   GuestInstanceMap::const_iterator it =
@@ skipping 56 matching lines @@
       return false;
 
     return embedder_render_process_id ==
         guest->GetOpener()->embedder_web_contents()->GetRenderProcessHost()->
             GetID();
   }
 
   return embedder_render_process_id ==
       guest->embedder_web_contents()->GetRenderProcessHost()->GetID();
 }