Add site_instance_id to ProcessMap::Item.

This is needed because there can be two site instances for the
same extension/process pair. Boo.

The linear scan for Contains() is a bummer. I thought about
more exotic data structures, but it didn't seem worth it.

BUG=105328

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

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

chrome/browser/chrome_content_browser_client.cc

@@ -457,14 +457,16 @@ void ChromeContentBrowserClient::SiteInstanceGotProcess(
   if (!extension)
     return;
 
-  service->process_map()->Insert(
-      extension->id(), site_instance->GetProcess()->GetID());
+  service->process_map()->Insert(extension->id(),
+                                 site_instance->GetProcess()->GetID(),
+                                 site_instance->id());
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&ExtensionInfoMap::RegisterExtensionProcess,
                  profile->GetExtensionInfoMap(),
                  extension->id(),
-                 site_instance->GetProcess()->GetID()));
+                 site_instance->GetProcess()->GetID(),
+                 site_instance->id()));
 }
 
 void ChromeContentBrowserClient::SiteInstanceDeleting(
@@ -485,14 +487,16 @@ void ChromeContentBrowserClient::SiteInstanceDeleting(
   if (!extension)
     return;
 
-  service->process_map()->Remove(
-      extension->id(), site_instance->GetProcess()->GetID());
+  service->process_map()->Remove(extension->id(),
+                                 site_instance->GetProcess()->GetID(),
+                                 site_instance->id());
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&ExtensionInfoMap::UnregisterExtensionProcess,
                  profile->GetExtensionInfoMap(),
                  extension->id(),
-                 site_instance->GetProcess()->GetID()));
+                 site_instance->GetProcess()->GetID(),
+                 site_instance->id()));
 }
 
 bool ChromeContentBrowserClient::ShouldSwapProcessesForNavigation(

chrome/browser/extensions/extension_info_map.cc

@@ -107,8 +107,9 @@ bool ExtensionInfoMap::CanCrossIncognito(const Extension* extension) {
 }
 
 void ExtensionInfoMap::RegisterExtensionProcess(const std::string& extension_id,
-                                                int process_id) {
-  if (!process_map_.Insert(extension_id, process_id)) {
+                                                int process_id,
+                                                int site_instance_id) {
+  if (!process_map_.Insert(extension_id, process_id, site_instance_id)) {
     NOTREACHED() << "Duplicate extension process registration for: "
                  << extension_id << "," << process_id << ".";
   }
@@ -116,15 +117,16 @@ void ExtensionInfoMap::RegisterExtensionProcess(const std::string& extension_id,
 
 void ExtensionInfoMap::UnregisterExtensionProcess(
     const std::string& extension_id,
-    int process_id) {
-  if (!process_map_.Remove(extension_id, process_id)) {
+    int process_id,
+    int site_instance_id) {
+  if (!process_map_.Remove(extension_id, process_id, site_instance_id)) {
     NOTREACHED() << "Unknown extension process registration for: "
                  << extension_id << "," << process_id << ".";
   }
 }
 
 void ExtensionInfoMap::UnregisterAllExtensionsInProcess(int process_id) {
-  process_map_.Remove(process_id);
+  process_map_.RemoveAllFromProcess(process_id);
 }
 
 bool ExtensionInfoMap::SecurityOriginHasAPIPermission(

chrome/browser/extensions/extension_info_map.h

@@ -55,11 +55,13 @@ class ExtensionInfoMap : public base::RefCountedThreadSafe<ExtensionInfoMap> {
 
   // Adds an entry to process_map_.
   void RegisterExtensionProcess(const std::string& extension_id,
-                                int process_id);
+                                int process_id,
+                                int site_instance_id);
 
   // Removes an entry from process_map_.
   void UnregisterExtensionProcess(const std::string& extension_id,
-                                  int process_id);
+                                  int process_id,
+                                  int site_instance_id);
   void UnregisterAllExtensionsInProcess(int process_id);
 
   // Returns true if there is exists an extension with the same origin as

chrome/browser/extensions/extension_service.cc

@@ -2373,7 +2373,7 @@ void ExtensionService::Observe(int type,
 
       installed_app_hosts_.erase(process->GetID());
 
-      process_map_.Remove(process->GetID());
+      process_map_.RemoveAllFromProcess(process->GetID());
       BrowserThread::PostTask(
           BrowserThread::IO, FROM_HERE,
           base::Bind(&ExtensionInfoMap::UnregisterAllExtensionsInProcess,

chrome/browser/extensions/process_map.cc

@@ -7,21 +7,27 @@
 namespace extensions {
 
 // Item
-ProcessMap::Item::Item() {
-}
+struct ProcessMap::Item {
+  Item() {
+  }
 
-ProcessMap::Item::Item(const ProcessMap::Item& other)
-    : extension_id(other.extension_id), process_id(other.process_id) {
-}
+  explicit Item(const ProcessMap::Item& other)
+      : extension_id(other.extension_id),
+        process_id(other.process_id),
+        site_instance_id(other.site_instance_id) {
+  }
 
-ProcessMap::Item::Item(const std::string& extension_id, int process_id)
-    : extension_id(extension_id), process_id(process_id) {
-}
+  Item(const std::string& extension_id, int process_id,
+       int site_instance_id)
+      : extension_id(extension_id),
+        process_id(process_id),
+        site_instance_id(site_instance_id) {
+  }
 
-ProcessMap::Item::~Item() {
-}
+  ~Item() {
+  }
 
-bool ProcessMap::Item::operator<(const ProcessMap::Item& other) const {
+  bool operator<(const ProcessMap::Item& other) const {
     if (extension_id < other.extension_id)
       return true;
 
@@ -30,8 +36,19 @@ bool ProcessMap::Item::operator<(const ProcessMap::Item& other) const {
       return true;
     }
 
+    if (extension_id == other.extension_id &&
+        process_id == other.process_id &&
+        site_instance_id < other.site_instance_id) {
+      return true;
+    }
+
     return false;
-}
+  }
+
+  std::string extension_id;
+  int process_id;
+  int site_instance_id;
+};
 
 
 // ProcessMap
@@ -41,15 +58,17 @@ ProcessMap::ProcessMap() {
 ProcessMap::~ProcessMap() {
 }
 
-bool ProcessMap::Insert(const std::string& extension_id, int process_id) {
-  return items_.insert(Item(extension_id, process_id)).second;
+bool ProcessMap::Insert(const std::string& extension_id, int process_id,
+                        int site_instance_id) {
+  return items_.insert(Item(extension_id, process_id, site_instance_id)).second;
 }
 
-bool ProcessMap::Remove(const std::string& extension_id, int process_id) {
-  return items_.erase(Item(extension_id, process_id)) > 0;
+bool ProcessMap::Remove(const std::string& extension_id, int process_id,
+                        int site_instance_id) {
+  return items_.erase(Item(extension_id, process_id, site_instance_id)) > 0;
 }
 
-int ProcessMap::Remove(int process_id) {
+int ProcessMap::RemoveAllFromProcess(int process_id) {
   int result = 0;
   for (ItemSet::const_iterator iter = items_.begin(); iter != items_.end(); ) {
     if (iter->process_id == process_id) {
@@ -64,7 +83,12 @@ int ProcessMap::Remove(int process_id) {
 
 bool ProcessMap::Contains(const std::string& extension_id,
                           int process_id) const {
-  return items_.find(Item(extension_id, process_id)) != items_.end();
+  for (ItemSet::const_iterator iter = items_.begin(); iter != items_.end();
+       ++iter) {
+    if (iter->process_id == process_id && iter->extension_id == extension_id)
+      return true;
+  }
+  return false;
 }
 
 bool ProcessMap::Contains(int process_id) const {

chrome/browser/extensions/process_map.h

@@ -16,6 +16,7 @@ namespace extensions {
 // Contains information about which extensions are assigned to which processes.
 //
 // The relationship between extensions and processes is complex:
+//
 // - Extensions can be either "split" mode or "spanning" mode.
 // - In spanning mode, extensions share a single process between all incognito
 //   and normal windows. This was the original mode for extensions.
@@ -23,6 +24,8 @@ namespace extensions {
 // - There are also hosted apps, which are a kind of extensions, and those
 //   usually have a process model similar to normal web sites: multiple
 //   processes per-profile.
+// - A single hosted app can have more than one SiteInstance in the same process
+//   if we're over the process limit and force them to share a process.
 //
 // In general, we seem to play with the process model of extensions a lot, so
 // it is safest to assume it is many-to-many in most places in the codebase.
@@ -43,12 +46,14 @@ namespace extensions {
 //    it is an "extension process" (e.g., for UI purposes). It may contain only
 //    hosted apps. See crbug.com/102533.
 //
-// 2. An extension can show be in multiple processes. That is why there is no
+// 2. An extension can show up in multiple processes. That is why there is no
 //    GetExtensionProcess() method here. There are two cases: a) The extension
 //    is actually a hosted app, in which case this is normal, or b) there is an
 //    incognito window open and the extension is "split mode". It is *not safe*
 //    to assume that there is one process per extension. If you only care about
-//    extensions (not hosted apps), and you are on the UI thread, then use
+//    extensions (not hosted apps), and you are on the UI thread, and you don't
+//    care about incognito version of this extension (or vice versa if you're in
+//    an incognito profile) then use
 //    ExtensionProcessManager::GetSiteInstanceForURL()->[Has|Get]Process().
 //
 // 3. The process ids contained in this class are *not limited* to the Profile
@@ -64,30 +69,23 @@ class ProcessMap {
 
   size_t size() const { return items_.size(); }
 
-  bool Insert(const std::string& extension_id, int process_id);
-  bool Remove(const std::string& extension_id, int process_id);
-  int Remove(int process_id);
+  bool Insert(const std::string& extension_id, int process_id,
+              int site_instance_id);
+
+  bool Remove(const std::string& extension_id, int process_id,
+              int site_instance_id);
+  int RemoveAllFromProcess(int process_id);
+
   bool Contains(const std::string& extension_id, int process_id) const;
   bool Contains(int process_id) const;
 
   std::set<std::string> GetExtensionsInProcess(int process_id) const;
 
  private:
-  struct Item {
-    Item();
-    Item(const Item& other);
-    Item(const std::string& extension_id, int process_id);
-    ~Item();
-
-    // Required for set membership.
-    bool operator<(const Item& other) const;
-
-    std::string extension_id;
-    int process_id;
-  };
+  struct Item;
 
   typedef std::set<Item> ItemSet;
-  std::set<Item> items_;
+  ItemSet items_;
 
   DISALLOW_COPY_AND_ASSIGN(ProcessMap);
 };

chrome/browser/extensions/process_map_unittest.cc

@@ -13,25 +13,25 @@ TEST(ExtensionProcessMapTest, Test) {
 
   // Test behavior when empty.
   EXPECT_FALSE(map.Contains("a", 1));
-  EXPECT_FALSE(map.Remove("a", 1));
+  EXPECT_FALSE(map.Remove("a", 1, 1));
   EXPECT_EQ(0u, map.size());
 
   // Test insertion and behavior with one item.
-  EXPECT_TRUE(map.Insert("a", 1));
+  EXPECT_TRUE(map.Insert("a", 1, 1));
   EXPECT_TRUE(map.Contains("a", 1));
   EXPECT_FALSE(map.Contains("a", 2));
   EXPECT_FALSE(map.Contains("b", 1));
   EXPECT_EQ(1u, map.size());
 
   // Test inserting a duplicate item.
-  EXPECT_FALSE(map.Insert("a", 1));
+  EXPECT_FALSE(map.Insert("a", 1, 1));
   EXPECT_TRUE(map.Contains("a", 1));
   EXPECT_EQ(1u, map.size());
 
   // Insert some more items.
-  EXPECT_TRUE(map.Insert("a", 2));
-  EXPECT_TRUE(map.Insert("b", 1));
-  EXPECT_TRUE(map.Insert("b", 2));
+  EXPECT_TRUE(map.Insert("a", 2, 2));
+  EXPECT_TRUE(map.Insert("b", 1, 3));
+  EXPECT_TRUE(map.Insert("b", 2, 4));
   EXPECT_EQ(4u, map.size());
 
   EXPECT_TRUE(map.Contains("a", 1));
@@ -40,13 +40,26 @@ TEST(ExtensionProcessMapTest, Test) {
   EXPECT_TRUE(map.Contains("b", 2));
   EXPECT_FALSE(map.Contains("a", 3));
 
+  // Note that this only differs from an existing item because of the site
+  // instance id.
+  EXPECT_TRUE(map.Insert("a", 1, 5));
+  EXPECT_TRUE(map.Contains("a", 1));
+
   // Test removal.
-  EXPECT_TRUE(map.Remove("a", 1));
-  EXPECT_FALSE(map.Remove("a", 1));
+  EXPECT_TRUE(map.Remove("a", 1, 1));
+  EXPECT_FALSE(map.Remove("a", 1, 1));
+  EXPECT_EQ(4u, map.size());
+
+  // Should still return true because there were two site instances for this
+  // extension/process pair.
+  EXPECT_TRUE(map.Contains("a", 1));
+
+  EXPECT_TRUE(map.Remove("a", 1, 5));
   EXPECT_EQ(3u, map.size());
+  EXPECT_FALSE(map.Contains("a", 1));
 
-  EXPECT_EQ(2, map.Remove(2));
+  EXPECT_EQ(2, map.RemoveAllFromProcess(2));
   EXPECT_EQ(1u, map.size());
-  EXPECT_EQ(0, map.Remove(2));
+  EXPECT_EQ(0, map.RemoveAllFromProcess(2));
   EXPECT_EQ(1u, map.size());
 }