Fix a bug that disabled some plugin groups at startup.

An empty plugin group is disabled by definition (because it doesn't contain any enabled plugins), and we wrongly transferred the disabled state when reloading the list.

BUG=73717
TEST=PluginListTest.HardcodedGroups

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

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

5 files changed, 67 insertions, 81 deletions

diff --git a/webkit/plugins/npapi/plugin_group.cc b/webkit/plugins/npapi/plugin_group.cc
index bd24f3d..fc5a4c1 100644
--- a/webkit/plugins/npapi/plugin_group.cc
+++ b/webkit/plugins/npapi/plugin_group.cc
@@ -94,7 +94,7 @@ PluginGroup::PluginGroup(const string16& group_name,
       group_name_(group_name),
       name_matcher_(name_matcher),
       update_url_(update_url),
-      enabled_(true),
+      enabled_(false),
       version_(Version::GetVersionFromString("0")) {
 }
 
diff --git a/webkit/plugins/npapi/plugin_group_unittest.cc b/webkit/plugins/npapi/plugin_group_unittest.cc
index 28f8fe6..476655f 100644
--- a/webkit/plugins/npapi/plugin_group_unittest.cc
+++ b/webkit/plugins/npapi/plugin_group_unittest.cc
@@ -14,7 +14,6 @@
 #include "base/version.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "webkit/plugins/npapi/webplugininfo.h"
-#include "webkit/plugins/npapi/plugin_list.h"
 
 namespace webkit {
 namespace npapi {
@@ -48,6 +47,14 @@ static const PluginGroupDefinition kPluginDef34 = {
 static const PluginGroupDefinition kPluginDefNotVulnerable = {
     "myplugin-latest", "MyPlugin", "MyPlugin", NULL, 0, "http://latest" };
 
+const PluginGroupDefinition kPluginDefinitions[] = {
+  kPluginDef,
+  kPluginDef3,
+  kPluginDef4,
+  kPluginDef34,
+  kPluginDefNotVulnerable,
+};
+
 // name, path, version, desc.
 static const WebPluginInfo kPluginNoVersion = WebPluginInfo(
     ASCIIToUTF16("MyPlugin"), FilePath(FILE_PATH_LITERAL("myplugin.so.2.0.43")),
@@ -157,14 +164,10 @@ TEST(PluginGroupTest, PluginGroupDescription) {
 }
 
 TEST(PluginGroupTest, PluginGroupDefinition) {
-  PluginList* plugin_list = PluginList::Singleton();
-  const PluginGroupDefinition* definitions =
-      plugin_list->GetPluginGroupDefinitions();
-  for (size_t i = 0; i < plugin_list->GetPluginGroupDefinitionsSize(); ++i) {
+  for (size_t i = 0; i < arraysize(kPluginDefinitions); ++i) {
     scoped_ptr<PluginGroup> def_group(
-        PluginGroupTest::CreatePluginGroup(definitions[i]));
+        PluginGroupTest::CreatePluginGroup(kPluginDefinitions[i]));
     ASSERT_TRUE(def_group.get() != NULL);
-    EXPECT_FALSE(def_group->Match(kPlugin2043));
   }
 }
 
diff --git a/webkit/plugins/npapi/plugin_list.cc b/webkit/plugins/npapi/plugin_list.cc
index 5aa1a99..b1343c5 100644
--- a/webkit/plugins/npapi/plugin_list.cc
+++ b/webkit/plugins/npapi/plugin_list.cc
@@ -157,17 +157,6 @@ static const PluginGroupDefinition kGroupDefinitions[] = {
 };
 #endif
 
-// static
-const PluginGroupDefinition* PluginList::GetPluginGroupDefinitions() {
-  return kGroupDefinitions;
-}
-
-// static
-size_t PluginList::GetPluginGroupDefinitionsSize() {
-  // TODO(viettrungluu): |arraysize()| doesn't work with zero-size arrays.
-  return ARRAYSIZE_UNSAFE(kGroupDefinitions);
-}
-
 base::LazyInstance<PluginList> g_singleton(base::LINKER_INITIALIZED);
 
 // static
@@ -324,11 +313,24 @@ bool PluginList::ParseMimeTypes(
 PluginList::PluginList()
     : plugins_loaded_(false),
       plugins_need_refresh_(false),
-      disable_outdated_plugins_(false) {
+      disable_outdated_plugins_(false),
+      group_definitions_(kGroupDefinitions),
+      num_group_definitions_(ARRAYSIZE_UNSAFE(kGroupDefinitions)) {
   PlatformInit();
   AddHardcodedPluginGroups(&plugin_groups_);
 }
 
+PluginList::PluginList(const PluginGroupDefinition* definitions,
+                       size_t num_definitions)
+    : plugins_loaded_(false),
+      plugins_need_refresh_(false),
+      disable_outdated_plugins_(false),
+      group_definitions_(definitions),
+      num_group_definitions_(num_definitions) {
+  // Don't do platform-dependend initialization in unit tests.
+  AddHardcodedPluginGroups(&plugin_groups_);
+}
+
 void PluginList::LoadPluginsInternal(ScopedVector<PluginGroup>* plugin_groups) {
   // Don't want to hold the lock while loading new plugins, so we don't block
   // other methods if they're called on other threads.
@@ -429,17 +431,7 @@ void PluginList::LoadPlugins(bool refresh) {
       }
     }
 
-    // Check if the group was disabled previously by the user.
-    for (size_t j = 0; j < plugin_groups_.size(); ++j) {
-      if (plugin_groups_[j]->GetGroupName() == group_name &&
-          !plugin_groups_[j]->Enabled()) {
-        group->EnableGroup(false);
-      }
-    }
-
     if (group->IsEmpty()) {
-      if (!group->Enabled())
-        groups_to_disable_.insert(group->GetGroupName());
       new_plugin_groups.erase(new_plugin_groups.begin() + i);
       --i;
       continue;
@@ -702,11 +694,10 @@ std::string PluginList::GetPluginGroupIdentifier(
 }
 
 void PluginList::AddHardcodedPluginGroups(ScopedVector<PluginGroup>* groups) {
-  base::AutoLock lock(lock_);
-  const PluginGroupDefinition* definitions = GetPluginGroupDefinitions();
-  size_t num_definitions = GetPluginGroupDefinitionsSize();
-  for (size_t i = 0; i < num_definitions; ++i)
-    groups->push_back(PluginGroup::FromPluginGroupDefinition(definitions[i]));
+  for (size_t i = 0; i < num_group_definitions_; ++i) {
+    groups->push_back(
+        PluginGroup::FromPluginGroupDefinition(group_definitions_[i]));
+  }
 }
 
 PluginGroup* PluginList::AddToPluginGroups(
@@ -771,7 +762,8 @@ bool PluginList::EnableGroup(bool enable, const string16& group_name) {
   base::AutoLock lock(lock_);
   PluginGroup* group = NULL;
   for (size_t i = 0; i < plugin_groups_.size(); ++i) {
-    if (plugin_groups_[i]->GetGroupName().find(group_name) != string16::npos) {
+    if (!plugin_groups_[i]->IsEmpty() &&
+        plugin_groups_[i]->GetGroupName().find(group_name) != string16::npos) {
       group = plugin_groups_[i];
       break;
     }
diff --git a/webkit/plugins/npapi/plugin_list.h b/webkit/plugins/npapi/plugin_list.h
index 6d96589..62c74c5 100644
--- a/webkit/plugins/npapi/plugin_list.h
+++ b/webkit/plugins/npapi/plugin_list.h
@@ -62,9 +62,6 @@ class PluginList {
   // by a command line switch.
   static bool DebugPluginLoading();
 
-  virtual const PluginGroupDefinition* GetPluginGroupDefinitions();
-  virtual size_t GetPluginGroupDefinitionsSize();
-
   // Returns true iff the plugin list has been loaded already.
   bool PluginsLoaded();
 
@@ -213,8 +210,9 @@ class PluginList {
   virtual ~PluginList();
 
  protected:
-  // Constructors are private for singletons
-  PluginList();
+  // This constructor is used in unit tests to override the platform-dependent
+  // real-world plugin group definitions with custom ones.
+  PluginList(const PluginGroupDefinition* definitions, size_t num_definitions);
 
   // Adds the given WebPluginInfo to its corresponding group, creating it if
   // necessary, and returns the group.
@@ -222,13 +220,14 @@ class PluginList {
   PluginGroup* AddToPluginGroups(const WebPluginInfo& web_plugin_info,
                                  ScopedVector<PluginGroup>* plugin_groups);
 
-  // Holds the currently available plugin groups.
-  ScopedVector<PluginGroup> plugin_groups_;
-
  private:
   friend class PluginListTest;
+  friend struct base::DefaultLazyInstanceTraits<PluginList>;
   FRIEND_TEST_ALL_PREFIXES(PluginGroupTest, PluginGroupDefinition);
 
+  // Constructors are private for singletons
+  PluginList();
+
   // Creates PluginGroups for the static group definitions, and adds them to
   // the list of PluginGroups.
   void AddHardcodedPluginGroups(ScopedVector<PluginGroup>* groups);
@@ -321,6 +320,13 @@ class PluginList {
   // If set to true outdated plugins are disabled in the end of LoadPlugins.
   bool disable_outdated_plugins_;
 
+  // Hardcoded plugin group definitions.
+  const PluginGroupDefinition* const group_definitions_;
+  const size_t num_group_definitions_;
+
+  // Holds the currently available plugin groups.
+  ScopedVector<PluginGroup> plugin_groups_;
+
   // The set of plugins that have been scheduled for disabling once they get
   // loaded. This list is used in LoadPlugins and pruned after it. Contains
   // plugins that were either disabled by the user (prefs are loaded before
@@ -337,8 +343,6 @@ class PluginList {
   // accessed on multiple threads.
   base::Lock lock_;
 
-  friend struct base::DefaultLazyInstanceTraits<PluginList>;
-
   DISALLOW_COPY_AND_ASSIGN(PluginList);
 };
 
diff --git a/webkit/plugins/npapi/plugin_list_unittest.cc b/webkit/plugins/npapi/plugin_list_unittest.cc
index 2420120..d037e38 100644
--- a/webkit/plugins/npapi/plugin_list_unittest.cc
+++ b/webkit/plugins/npapi/plugin_list_unittest.cc
@@ -16,6 +16,11 @@ namespace plugin_test_internal {
 // to use |lock_| (but it doesn't hurt either).
 class PluginListWithoutFileIO : public PluginList {
  public:
+  PluginListWithoutFileIO(const PluginGroupDefinition* group_definitions,
+                          size_t num_group_definitions) :
+      PluginList(group_definitions, num_group_definitions) {}
+  virtual ~PluginListWithoutFileIO() {}
+
   void AddPluginToLoad(const WebPluginInfo& plugin) {
     plugins_to_load_.push_back(plugin);
   }
@@ -38,15 +43,6 @@ class PluginListWithoutFileIO : public PluginList {
     for (size_t i = 0; i < plugins_to_load_.size(); ++i)
       AddToPluginGroups(plugins_to_load_[i], plugin_groups);
   }
-
-  virtual const PluginGroupDefinition* GetPluginGroupDefinitions() OVERRIDE {
-    if (hardcoded_definitions_.empty())
-      return NULL;
-    return &hardcoded_definitions_.front();
-  }
-  virtual size_t GetPluginGroupDefinitionsSize() OVERRIDE {
-    return hardcoded_definitions_.size();
-  }
 };
 
 }  // namespace plugin_test_internal
@@ -76,15 +72,20 @@ bool Contains(const std::vector<WebPluginInfo>& list,
 FilePath::CharType kFooPath[] = FILE_PATH_LITERAL("/plugins/foo.plugin");
 FilePath::CharType kBarPath[] = FILE_PATH_LITERAL("/plugins/bar.plugin");
 const char* kFooIdentifier = "foo";
-const char* kFooName = "Foo";
-
+const char* kFooGroupName = "Foo";
+const char* kFooName = "Foo Plugin";
+const PluginGroupDefinition kPluginDefinitions[] = {
+  { kFooIdentifier, kFooGroupName, kFooName, NULL, 0,
+    "http://example.com/foo" },
+};
 
 }  // namespace
 
 class PluginListTest : public testing::Test {
  public:
   PluginListTest()
-      : foo_plugin_(ASCIIToUTF16("Foo Plugin"),
+      : plugin_list_(kPluginDefinitions, arraysize(kPluginDefinitions)),
+        foo_plugin_(ASCIIToUTF16(kFooName),
                     FilePath(kFooPath),
                     ASCIIToUTF16("1.2.3"),
                     ASCIIToUTF16("foo")),
@@ -99,11 +100,6 @@ class PluginListTest : public testing::Test {
     plugin_list_.DisablePlugin(bar_plugin_.path);
     plugin_list_.AddPluginToLoad(foo_plugin_);
     plugin_list_.AddPluginToLoad(bar_plugin_);
-    PluginGroupDefinition foo_definition = { kFooIdentifier, kFooName,
-                                             "Foo Plugin", NULL, 0,
-                                             "http://example.com/foo" };
-    plugin_list_.AddPluginGroupDefinition(foo_definition);
-    plugin_list_.LoadPlugins(true);
   }
 
  protected:
@@ -129,7 +125,7 @@ TEST_F(PluginListTest, GetEnabledPlugins) {
 
 TEST_F(PluginListTest, GetPluginGroup) {
   const PluginGroup* foo_group = plugin_list_.GetPluginGroup(foo_plugin_);
-  EXPECT_EQ(ASCIIToUTF16(kFooName), foo_group->GetGroupName());
+  EXPECT_EQ(ASCIIToUTF16(kFooGroupName), foo_group->GetGroupName());
   EXPECT_TRUE(foo_group->Enabled());
   // The second request should return a pointer to the same instance.
   const PluginGroup* foo_group2 = plugin_list_.GetPluginGroup(foo_plugin_);
@@ -255,34 +251,25 @@ TEST_F(PluginListTest, HardcodedGroups) {
   std::vector<PluginGroup> groups;
   plugin_list_.GetPluginGroups(true, &groups);
   ASSERT_EQ(2u, groups.size());
+  EXPECT_TRUE(groups[0].Enabled());
   EXPECT_EQ(1u, groups[0].web_plugins_info().size());
   EXPECT_TRUE(groups[0].ContainsPlugin(FilePath(kFooPath)));
   EXPECT_EQ(kFooIdentifier, groups[0].identifier());
+  EXPECT_FALSE(groups[1].Enabled());
   EXPECT_EQ(1u, groups[1].web_plugins_info().size());
   EXPECT_TRUE(groups[1].ContainsPlugin(FilePath(kBarPath)));
   EXPECT_EQ("bar.plugin", groups[1].identifier());
 }
 
-TEST_F(PluginListTest, UpdatedPlugin) {
-  // Test that a disabled plugin stays disabled when it's updated between
-  // browser restarts, i.e. its path and version change, but it stays in the
-  // same plugin group.
+TEST_F(PluginListTest, DisableBeforeLoad) {
+  // Test that a plugin group that was disabled before plugins are loaded stays
+  // disabled afterwards.
 
-  // Create a pristine PluginList.
-  plugin_test_internal::PluginListWithoutFileIO plugin_list;
+  EXPECT_TRUE(plugin_list_.EnableGroup(false, ASCIIToUTF16(kFooGroupName)));
 
-  // The preferences will mark the "MyPlugin" group as disabled.
-  string16 myPluginName(ASCIIToUTF16("MyPlugin"));
-  EXPECT_TRUE(plugin_list.EnableGroup(false, myPluginName));
-
-  WebPluginInfo plugin_3045(myPluginName,
-                            FilePath(FILE_PATH_LITERAL("/myplugin.3.0.45")),
-                            ASCIIToUTF16("3.0.45"),
-                            ASCIIToUTF16("MyPlugin version 3.0.45"));
-  plugin_list.AddPluginToLoad(plugin_3045);
   std::vector<WebPluginInfo> plugins;
-  plugin_list.GetPlugins(true, &plugins);
-  ASSERT_EQ(1u, plugins.size());
+  plugin_list_.GetPlugins(true, &plugins);
+  ASSERT_EQ(2u, plugins.size());
   ASSERT_EQ(WebPluginInfo::USER_DISABLED_POLICY_UNMANAGED, plugins[0].enabled);
 }