Clean up PluginGroup and related code.

To avoid data races, do not pass pointers to PluginGroup around. Instead, create copies as plain objects.

BUG=61210
TEST=existing unit tests still work; TSan no longer reports the race (see bug)

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

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

12 files changed, 64 insertions, 69 deletions

diff --git a/chrome/browser/gtk/content_setting_bubble_gtk.cc b/chrome/browser/gtk/content_setting_bubble_gtk.cc
index 18118ac..3bf6145 100644
--- a/chrome/browser/gtk/content_setting_bubble_gtk.cc
+++ b/chrome/browser/gtk/content_setting_bubble_gtk.cc
@@ -4,6 +4,10 @@
 
 #include "chrome/browser/gtk/content_setting_bubble_gtk.h"
 
+#include <set>
+#include <string>
+#include <vector>
+
 #include "app/l10n_util.h"
 #include "app/text_elider.h"
 #include "base/i18n/rtl.h"
@@ -104,12 +108,9 @@ void ContentSettingBubbleGtk::BuildBubble() {
 
     for (std::set<std::string>::const_iterator it = plugins.begin();
         it != plugins.end(); ++it) {
-      std::string name;
-      NPAPI::PluginList::PluginMap groups;
-      NPAPI::PluginList::Singleton()->GetPluginGroups(false, &groups);
-      if (groups.find(*it) != groups.end())
-        name = UTF16ToUTF8(groups[*it]->GetGroupName());
-      else
+      std::string name = UTF16ToUTF8(
+          NPAPI::PluginList::Singleton()->GetPluginGroupName(*it));
+      if (name.empty())
         name = *it;
 
       GtkWidget* label = gtk_label_new(BuildElidedText(name).c_str());
diff --git a/chrome/browser/mock_plugin_exceptions_table_model.cc b/chrome/browser/mock_plugin_exceptions_table_model.cc
index e2d38a7..77b2a2f 100644
--- a/chrome/browser/mock_plugin_exceptions_table_model.cc
+++ b/chrome/browser/mock_plugin_exceptions_table_model.cc
@@ -5,11 +5,11 @@
 #include "chrome/browser/mock_plugin_exceptions_table_model.h"
 
 void MockPluginExceptionsTableModel::set_plugins(
-    const NPAPI::PluginList::PluginMap& plugins) {
+    std::vector<PluginGroup>& plugins) {
   plugins_ = plugins;
 }
 
 void MockPluginExceptionsTableModel::GetPlugins(
-    NPAPI::PluginList::PluginMap* plugins) {
-  *plugins = plugins_;
+    std::vector<PluginGroup>* plugin_groups) {
+  *plugin_groups = plugins_;
 }
diff --git a/chrome/browser/mock_plugin_exceptions_table_model.h b/chrome/browser/mock_plugin_exceptions_table_model.h
index d1f5da7..15a1c5b 100644
--- a/chrome/browser/mock_plugin_exceptions_table_model.h
+++ b/chrome/browser/mock_plugin_exceptions_table_model.h
@@ -17,13 +17,13 @@ class MockPluginExceptionsTableModel : public PluginExceptionsTableModel {
       : PluginExceptionsTableModel(map, otr_map) {}
   virtual ~MockPluginExceptionsTableModel() {}
 
-  void set_plugins(const NPAPI::PluginList::PluginMap& plugins);
+  void set_plugins(std::vector<PluginGroup>& plugins);
 
  protected:
-  virtual void GetPlugins(NPAPI::PluginList::PluginMap* plugins);
+  virtual void GetPlugins(std::vector<PluginGroup>* plugin_groups);
 
  private:
-  NPAPI::PluginList::PluginMap plugins_;
+  std::vector<PluginGroup> plugins_;
 };
 
 #endif  // CHROME_BROWSER_MOCK_PLUGIN_EXCEPTIONS_TABLE_MODEL_H_
diff --git a/chrome/browser/plugin_exceptions_table_model.cc b/chrome/browser/plugin_exceptions_table_model.cc
index 21ed547..c690908 100644
--- a/chrome/browser/plugin_exceptions_table_model.cc
+++ b/chrome/browser/plugin_exceptions_table_model.cc
@@ -130,17 +130,16 @@ void PluginExceptionsTableModel::ClearSettings() {
 }
 
 void PluginExceptionsTableModel::GetPlugins(
-    NPAPI::PluginList::PluginMap* plugins) {
-  NPAPI::PluginList::Singleton()->GetPluginGroups(false, plugins);
+    std::vector<PluginGroup>* plugin_groups) {
+  NPAPI::PluginList::Singleton()->GetPluginGroups(false, plugin_groups);
 }
 
 void PluginExceptionsTableModel::LoadSettings() {
   int group_id = 0;
-  NPAPI::PluginList::PluginMap plugins;
+  std::vector<PluginGroup> plugins;
   GetPlugins(&plugins);
-  for (NPAPI::PluginList::PluginMap::iterator it = plugins.begin();
-       it != plugins.end(); ++it) {
-    std::string plugin = it->first;
+  for (size_t i = 0; i < plugins.size(); ++i) {
+    std::string plugin = plugins[i].identifier();
     HostContentSettingsMap::SettingsForOneType settings;
     map_->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_PLUGINS,
                                 plugin,
@@ -151,7 +150,7 @@ void PluginExceptionsTableModel::LoadSettings() {
                                       plugin,
                                       &otr_settings);
     }
-    std::wstring title = UTF16ToWide(it->second->GetGroupName());
+    std::wstring title = UTF16ToWide(plugins[i].GetGroupName());
     for (HostContentSettingsMap::SettingsForOneType::iterator setting_it =
              settings.begin(); setting_it != settings.end(); ++setting_it) {
       SettingsEntry entry = {
diff --git a/chrome/browser/plugin_exceptions_table_model.h b/chrome/browser/plugin_exceptions_table_model.h
index 3a7c69a..6e8f00b 100644
--- a/chrome/browser/plugin_exceptions_table_model.h
+++ b/chrome/browser/plugin_exceptions_table_model.h
@@ -7,6 +7,8 @@
 #pragma once
 
 #include <deque>
+#include <string>
+#include <vector>
 
 #include "chrome/browser/content_settings/host_content_settings_map.h"
 #include "chrome/browser/remove_rows_table_model.h"
@@ -49,7 +51,7 @@ class PluginExceptionsTableModel : public RemoveRowsTableModel,
 
  protected:
   // Subclasses can override this method for testing.
-  virtual void GetPlugins(NPAPI::PluginList::PluginMap* plugins);
+  virtual void GetPlugins(std::vector<PluginGroup>* plugin_groups);
 
  private:
   friend class plugin_test_internal::PluginExceptionsTableModelTest;
diff --git a/chrome/browser/plugin_exceptions_table_model_unittest.cc b/chrome/browser/plugin_exceptions_table_model_unittest.cc
index 61ffdad..9540893 100644
--- a/chrome/browser/plugin_exceptions_table_model_unittest.cc
+++ b/chrome/browser/plugin_exceptions_table_model_unittest.cc
@@ -82,20 +82,22 @@ class PluginExceptionsTableModelTest : public testing::Test {
 
     table_model_.reset(new MockPluginExceptionsTableModel(map, NULL));
 
-    NPAPI::PluginList::PluginMap plugins;
+    std::vector<PluginGroup> plugins;
     WebPluginInfo foo_plugin;
     foo_plugin.path = FilePath(FILE_PATH_LITERAL("a-foo"));
     foo_plugin.name = ASCIIToUTF16("FooPlugin");
     foo_plugin.enabled = true;
-    PluginGroup* foo_group = PluginGroup::FromWebPluginInfo(foo_plugin);
-    plugins[foo_group->identifier()] = linked_ptr<PluginGroup>(foo_group);
+    scoped_ptr<PluginGroup> foo_group(
+        PluginGroup::FromWebPluginInfo(foo_plugin));
+    plugins.push_back(*foo_group);
 
     WebPluginInfo bar_plugin;
     bar_plugin.path = FilePath(FILE_PATH_LITERAL("b-bar"));
     bar_plugin.name = ASCIIToUTF16("BarPlugin");
     bar_plugin.enabled = true;
-    PluginGroup* bar_group = PluginGroup::FromWebPluginInfo(bar_plugin);
-    plugins[bar_group->identifier()] = linked_ptr<PluginGroup>(bar_group);
+    scoped_ptr<PluginGroup> bar_group(
+        PluginGroup::FromWebPluginInfo(bar_plugin));
+    plugins.push_back(*bar_group);
 
     table_model_->set_plugins(plugins);
     table_model_->ReloadSettings();
diff --git a/chrome/browser/plugin_updater.cc b/chrome/browser/plugin_updater.cc
index 80f0054..338e09c 100644
--- a/chrome/browser/plugin_updater.cc
+++ b/chrome/browser/plugin_updater.cc
@@ -4,8 +4,8 @@
 
 #include "chrome/browser/plugin_updater.h"
 
+#include <set>
 #include <string>
-#include <vector>
 
 #include "base/command_line.h"
 #include "base/message_loop.h"
@@ -45,16 +45,13 @@ DictionaryValue* PluginUpdater::CreatePluginFileSummary(
 
 // static
 ListValue* PluginUpdater::GetPluginGroupsData() {
-  NPAPI::PluginList::PluginMap plugin_groups;
+  std::vector<PluginGroup> plugin_groups;
   NPAPI::PluginList::Singleton()->GetPluginGroups(true, &plugin_groups);
 
   // Construct DictionaryValues to return to the UI
   ListValue* plugin_groups_data = new ListValue();
-  for (NPAPI::PluginList::PluginMap::const_iterator it =
-       plugin_groups.begin();
-       it != plugin_groups.end();
-       ++it) {
-    plugin_groups_data->Append(it->second->GetDataForUI());
+  for (size_t i = 0; i < plugin_groups.size(); ++i) {
+    plugin_groups_data->Append(plugin_groups[i].GetDataForUI());
   }
   return plugin_groups_data;
 }
@@ -244,7 +241,7 @@ void PluginUpdater::GetPreferencesDataOnFileThread(void* profile) {
   std::vector<WebPluginInfo> plugins;
   NPAPI::PluginList::Singleton()->GetPlugins(false, &plugins);
 
-  NPAPI::PluginList::PluginMap groups;
+  std::vector<PluginGroup> groups;
   NPAPI::PluginList::Singleton()->GetPluginGroups(false, &groups);
 
   BrowserThread::PostTask(
@@ -258,7 +255,7 @@ void PluginUpdater::GetPreferencesDataOnFileThread(void* profile) {
 void PluginUpdater::OnUpdatePreferences(
     Profile* profile,
     const std::vector<WebPluginInfo>& plugins,
-    const NPAPI::PluginList::PluginMap& groups) {
+    const std::vector<PluginGroup>& groups) {
   ListValue* plugins_list = profile->GetPrefs()->GetMutableList(
       prefs::kPluginsPluginsList);
   plugins_list->Clear();
@@ -276,13 +273,12 @@ void PluginUpdater::OnUpdatePreferences(
   }
 
   // Add the groups as well.
-  for (NPAPI::PluginList::PluginMap::const_iterator it = groups.begin();
-       it != groups.end(); ++it) {
+  for (size_t i = 0; i < groups.size(); ++i) {
     // Don't save preferences for vulnerable pugins.
     if (!CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisableOutdatedPlugins) ||
-        !it->second->IsVulnerable()) {
-      plugins_list->Append(it->second->GetSummary());
+        !groups[i].IsVulnerable()) {
+      plugins_list->Append(groups[i].GetSummary());
     }
   }
 }
diff --git a/chrome/browser/plugin_updater.h b/chrome/browser/plugin_updater.h
index 2b8cd106..78a3cd6 100644
--- a/chrome/browser/plugin_updater.h
+++ b/chrome/browser/plugin_updater.h
@@ -6,6 +6,8 @@
 #define CHROME_BROWSER_PLUGIN_UPDATER_H_
 #pragma once
 
+#include <vector>
+
 #include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/singleton.h"
@@ -54,9 +56,10 @@ class PluginUpdater : public NotificationObserver {
   static void GetPreferencesDataOnFileThread(void* profile);
 
   // Called on the UI thread with the plugin data to save the preferences.
-  static void OnUpdatePreferences(Profile* profile,
-                                  const std::vector<WebPluginInfo>& plugins,
-                                  const NPAPI::PluginList::PluginMap& groups);
+  static void OnUpdatePreferences(
+      Profile* profile,
+      const std::vector<WebPluginInfo>& plugins,
+      const std::vector<PluginGroup>& groups);
 
   // Queues sending the notification that plugin data has changed.  This is done
   // so that if a bunch of changes happen, we only send one notification.
diff --git a/chrome/browser/renderer_host/resource_message_filter.cc b/chrome/browser/renderer_host/resource_message_filter.cc
index c056f41..d356f78 100644
--- a/chrome/browser/renderer_host/resource_message_filter.cc
+++ b/chrome/browser/renderer_host/resource_message_filter.cc
@@ -859,21 +859,20 @@ void ResourceMessageFilter::OnGotPluginInfo(bool found,
                                             const GURL& policy_url,
                                             IPC::Message* reply_msg) {
   ContentSetting setting = CONTENT_SETTING_DEFAULT;
+  WebPluginInfo info_copy = info;
   if (found) {
-    WebPluginInfo info_copy = info;
     info_copy.enabled = info_copy.enabled &&
         plugin_service_->PrivatePluginAllowedForURL(info_copy.path, policy_url);
     HostContentSettingsMap* map = profile_->GetHostContentSettingsMap();
-    scoped_ptr<PluginGroup> group(
-        PluginGroup::CopyOrCreatePluginGroup(info_copy));
-    std::string resource = group->identifier();
+    std::string resource =
+        NPAPI::PluginList::Singleton()->GetPluginGroupIdentifier(info_copy);
     setting = map->GetContentSetting(policy_url,
                                      CONTENT_SETTINGS_TYPE_PLUGINS,
                                      resource);
   }
 
   ViewHostMsg_GetPluginInfo::WriteReplyParams(
-      reply_msg, found, info, setting, actual_mime_type);
+      reply_msg, found, info_copy, setting, actual_mime_type);
   Send(reply_msg);
 }
 
diff --git a/chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm b/chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm
index 26c3827..4ac68c0 100644
--- a/chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm
+++ b/chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm
@@ -242,12 +242,9 @@ NSTextField* LabelWithFrame(NSString* text, const NSRect& frame) {
   } else {
     for (std::set<std::string>::iterator it = plugins.begin();
          it != plugins.end(); ++it) {
-      NSString* name;
-      NPAPI::PluginList::PluginMap groups;
-      NPAPI::PluginList::Singleton()->GetPluginGroups(false, &groups);
-      if (groups.find(*it) != groups.end())
-        name = base::SysUTF16ToNSString(groups[*it]->GetGroupName());
-      else
+      NSString* name = SysUTF16ToNSString(
+          NPAPI::PluginList::Singleton()->GetPluginGroupName(*it));
+      if ([name length] == 0)
         name = base::SysUTF8ToNSString(*it);
       [pluginArray addObject:name];
     }
diff --git a/chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm b/chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm
index 65b901f..051d58b 100644
--- a/chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm
+++ b/chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm
@@ -18,8 +18,6 @@
 #include "webkit/glue/plugins/plugin_list.h"
 #include "webkit/glue/plugins/webplugininfo.h"
 
-namespace {
-
 class TableModelArrayControllerTest : public CocoaTest {
  public:
   TableModelArrayControllerTest()
@@ -52,25 +50,28 @@ class TableModelArrayControllerTest : public CocoaTest {
 
     model_.reset(new MockPluginExceptionsTableModel(map, NULL));
 
-    NPAPI::PluginList::PluginMap plugins;
+    std::vector<PluginGroup> plugins;
     WebPluginInfo foo_plugin;
     foo_plugin.path = FilePath(FILE_PATH_LITERAL("a-foo"));
     foo_plugin.name = ASCIIToUTF16("FooPlugin");
     foo_plugin.enabled = true;
-    PluginGroup* foo_group = PluginGroup::FromWebPluginInfo(foo_plugin);
-    plugins[foo_group->identifier()] = linked_ptr<PluginGroup>(foo_group);
+    scoped_ptr<PluginGroup> foo_group(
+        PluginGroup::FromWebPluginInfo(foo_plugin));
+    plugins.push_back(*foo_group);
     WebPluginInfo bar_plugin;
     bar_plugin.path = FilePath(FILE_PATH_LITERAL("b-bar"));
     bar_plugin.name = ASCIIToUTF16("BarPlugin");
     bar_plugin.enabled = true;
-    PluginGroup* bar_group = PluginGroup::FromWebPluginInfo(bar_plugin);
-    plugins[bar_group->identifier()] = linked_ptr<PluginGroup>(bar_group);
+    scoped_ptr<PluginGroup> bar_group(
+        PluginGroup::FromWebPluginInfo(bar_plugin));
+    plugins.push_back(*bar_group);
     WebPluginInfo blurp_plugin;
     blurp_plugin.path = FilePath(FILE_PATH_LITERAL("c-blurp"));
     blurp_plugin.name = ASCIIToUTF16("BlurpPlugin");
     blurp_plugin.enabled = true;
-    PluginGroup* blurp_group = PluginGroup::FromWebPluginInfo(blurp_plugin);
-    plugins[blurp_group->identifier()] = linked_ptr<PluginGroup>(blurp_group);
+    scoped_ptr<PluginGroup> blurp_group(
+        PluginGroup::FromWebPluginInfo(blurp_plugin));
+    plugins.push_back(*blurp_group);
 
     model_->set_plugins(plugins);
     model_->LoadSettings();
@@ -168,5 +169,3 @@ TEST_F(TableModelArrayControllerTest, AddException) {
               @")",
               [titles description]);
 }
-
-}  // namespace
diff --git a/chrome/browser/ui/views/content_setting_bubble_contents.cc b/chrome/browser/ui/views/content_setting_bubble_contents.cc
index 48e82aa..d2954eb 100644
--- a/chrome/browser/ui/views/content_setting_bubble_contents.cc
+++ b/chrome/browser/ui/views/content_setting_bubble_contents.cc
@@ -229,12 +229,9 @@ void ContentSettingBubbleContents::InitControlLayout() {
   if (!plugins.empty()) {
     for (std::set<std::string>::const_iterator it = plugins.begin();
         it != plugins.end(); ++it) {
-      std::wstring name;
-      NPAPI::PluginList::PluginMap groups;
-      NPAPI::PluginList::Singleton()->GetPluginGroups(false, &groups);
-      if (groups.find(*it) != groups.end())
-        name = UTF16ToWide(groups[*it]->GetGroupName());
-      else
+      std::wstring name = UTF16ToWide(
+          NPAPI::PluginList::Singleton()->GetPluginGroupName(*it));
+      if (name.empty())
         name = UTF8ToWide(*it);
       layout->StartRow(0, single_column_set_id);
       layout->AddView(new views::Label(name));