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

Guests can exhibit un-intended behavior otherwise.

BUG=None
Test=Internal

Review URL: https://codereview.chromium.org/298913003

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

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() {}
 
@@ -190,8 +190,8 @@ bool GuestViewManager::ForEachGuest(WebContents* embedder_web_contents,
 
 void 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());
+  CHECK(!ContainsKey(guest_web_contents_by_instance_id_, guest_instance_id));
+  CHECK(CanUseGuestInstanceID(guest_instance_id));
   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);
@@ -204,6 +204,29 @@ 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);
+
+  // All the instance IDs that lie within [0, last_instance_id_removed_]
+  // are invalid.
+  // The remaining sparse invalid IDs are kept in |removed_instance_ids_| set.
+  // The following code compacts the set by incrementing
+  // |last_instance_id_removed_|.
+  if (guest_instance_id == last_instance_id_removed_ + 1) {
+    ++last_instance_id_removed_;
+    // Compact.
+    std::set<int>::iterator iter = removed_instance_ids_.begin();
+    while (iter != removed_instance_ids_.end()) {
+      int instance_id = *iter;
+      // The sparse invalid IDs must not lie within
+      // [0, last_instance_id_removed_]
+      DCHECK(instance_id > last_instance_id_removed_);
+      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) {
@@ -237,6 +260,12 @@ bool GuestViewManager::CanEmbedderAccessInstanceIDMaybeKill(
   return true;
 }
 
+bool GuestViewManager::CanUseGuestInstanceID(int guest_instance_id) {
+  if (guest_instance_id <= last_instance_id_removed_)
+    return false;
+  return !ContainsKey(removed_instance_ids_, guest_instance_id);
+}
+
 bool GuestViewManager::CanEmbedderAccessInstanceID(
     int embedder_render_process_id,
     int guest_instance_id) {

chrome/browser/guest_view/guest_view_manager.h

@@ -7,6 +7,7 @@
 
 #include <map>
 
+#include "base/gtest_prod_util.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "content/public/browser/browser_plugin_guest_manager.h"
@@ -56,6 +57,8 @@ class GuestViewManager : public content::BrowserPluginGuestManager,
  private:
   friend class GuestViewBase;
   friend class GuestWebContentsObserver;
+  friend class TestGuestViewManager;
+  FRIEND_TEST_ALL_PREFIXES(GuestViewManagerTest, AddRemove);
 
   void AddGuest(int guest_instance_id,
                 content::WebContents* guest_web_contents);
@@ -78,6 +81,12 @@ class GuestViewManager : public content::BrowserPluginGuestManager,
   bool CanEmbedderAccessInstanceID(int embedder_render_process_id,
                                    int guest_instance_id);
 
+  // Returns true if |guest_instance_id| can be used to add a new guest to this
+  // manager.
+  // We disallow adding new guest with instance IDs that were previously removed
+  // from this manager using RemoveGuest.
+  bool CanUseGuestInstanceID(int guest_instance_id);
+
   static bool CanEmbedderAccessGuest(int embedder_render_process_id,
                                      GuestViewBase* guest);
 
@@ -89,6 +98,16 @@ class GuestViewManager : public content::BrowserPluginGuestManager,
   GuestInstanceMap guest_web_contents_by_instance_id_;
 
   int current_instance_id_;
+
+  // Any instance ID whose number not greater than this was removed via
+  // RemoveGuest.
+  // This is used so that we don't have store all removed instance IDs in
+  // |removed_instance_ids_|.
+  int last_instance_id_removed_;
+  // The remaining instance IDs that are greater than
+  // |last_instance_id_removed_| are kept here.
+  std::set<int> removed_instance_ids_;
+
   content::BrowserContext* context_;
 
   DISALLOW_COPY_AND_ASSIGN(GuestViewManager);

chrome/browser/guest_view/guest_view_manager_unittest.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 "chrome/test/base/testing_profile.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/web_contents_tester.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using content::WebContents;
+using content::WebContentsTester;
+
+class GuestViewManagerTest : public testing::Test {
+ public:
+  GuestViewManagerTest() {}
+  virtual ~GuestViewManagerTest() {}
+
+  scoped_ptr<WebContents> CreateWebContents() {
+    return scoped_ptr<WebContents>(
+        WebContentsTester::CreateTestWebContents(&profile_, NULL));
+  }
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;
+  TestingProfile profile_;
+
+  DISALLOW_COPY_AND_ASSIGN(GuestViewManagerTest);
+};
+
+// This class allows us to access some private variables in
+// GuestViewManager.
+class TestGuestViewManager : public GuestViewManager {
+ public:
+  explicit TestGuestViewManager(content::BrowserContext* context)
+      : GuestViewManager(context) {}
+
+  int last_instance_id_removed_for_testing() {
+    return last_instance_id_removed_;
+  }
+
+  size_t GetRemovedInstanceIdSize() { return removed_instance_ids_.size(); }
+
+ private:
+  using GuestViewManager::last_instance_id_removed_;
+  using GuestViewManager::removed_instance_ids_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestGuestViewManager);
+};
+
+TEST_F(GuestViewManagerTest, AddRemove) {
+  TestingProfile profile;
+  scoped_ptr<TestGuestViewManager> manager(new TestGuestViewManager(&profile));
+
+  scoped_ptr<WebContents> web_contents1(CreateWebContents());
+  scoped_ptr<WebContents> web_contents2(CreateWebContents());
+  scoped_ptr<WebContents> web_contents3(CreateWebContents());
+
+  EXPECT_EQ(0, manager->last_instance_id_removed_for_testing());
+
+  EXPECT_TRUE(manager->CanUseGuestInstanceID(1));
+  EXPECT_TRUE(manager->CanUseGuestInstanceID(2));
+  EXPECT_TRUE(manager->CanUseGuestInstanceID(3));
+
+  manager->AddGuest(1, web_contents1.get());
+  manager->AddGuest(2, web_contents2.get());
+  manager->RemoveGuest(2);
+
+  // Since we removed 2, it would be an invalid ID.
+  EXPECT_TRUE(manager->CanUseGuestInstanceID(1));
+  EXPECT_FALSE(manager->CanUseGuestInstanceID(2));
+  EXPECT_TRUE(manager->CanUseGuestInstanceID(3));
+
+  EXPECT_EQ(0, manager->last_instance_id_removed_for_testing());
+
+  EXPECT_TRUE(manager->CanUseGuestInstanceID(3));
+
+  manager->AddGuest(3, web_contents3.get());
+  manager->RemoveGuest(1);
+  EXPECT_FALSE(manager->CanUseGuestInstanceID(1));
+  EXPECT_FALSE(manager->CanUseGuestInstanceID(2));
+
+  EXPECT_EQ(2, manager->last_instance_id_removed_for_testing());
+  manager->RemoveGuest(3);
+  EXPECT_EQ(3, manager->last_instance_id_removed_for_testing());
+
+  EXPECT_FALSE(manager->CanUseGuestInstanceID(1));
+  EXPECT_FALSE(manager->CanUseGuestInstanceID(2));
+  EXPECT_FALSE(manager->CanUseGuestInstanceID(3));
+
+  EXPECT_EQ(0u, manager->GetRemovedInstanceIdSize());
+}

chrome/chrome_tests_unit.gypi

@@ -599,7 +599,7 @@
         'browser/autocomplete/search_provider_unittest.cc',
         'browser/autocomplete/shortcuts_backend_unittest.cc',
         'browser/autocomplete/shortcuts_provider_unittest.cc',
-	'browser/autocomplete/zero_suggest_provider_unittest.cc',
+        'browser/autocomplete/zero_suggest_provider_unittest.cc',
         'browser/autofill/autofill_cc_infobar_delegate_unittest.cc',
         'browser/background/background_application_list_model_unittest.cc',
         'browser/background/background_contents_service_unittest.cc',
@@ -988,6 +988,7 @@
         'browser/google/google_update_settings_unittest.cc',
         'browser/google/google_url_tracker_unittest.cc',
         'browser/google/google_util_unittest.cc',
+        'browser/guest_view/guest_view_manager_unittest.cc',
         'browser/history/android/android_cache_database_unittest.cc',
         'browser/history/android/android_history_provider_service_unittest.cc',
         'browser/history/android/android_history_types_unittest.cc',