Policy: Re-enabled plugin still disabled

webkit/glue/plugins/plugin_list.cc

Index: webkit/glue/plugins/plugin_list.cc
diff --git a/webkit/glue/plugins/plugin_list.cc b/webkit/glue/plugins/plugin_list.cc
index a2b4cf5cb73f6c545a7ff0e35116056d79974d2f..7cfbe2996f539fcd1c9a717f6fb10dc7f336472c 100644
--- a/webkit/glue/plugins/plugin_list.cc
+++ b/webkit/glue/plugins/plugin_list.cc
@@ -14,7 +14,6 @@
 #include "base/sys_string_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "googleurl/src/gurl.h"
-#include "net/base/mime_util.h"
 #include "webkit/glue/plugins/plugin_constants_win.h"
 #include "webkit/glue/plugins/plugin_lib.h"
 #include "webkit/glue/webkit_glue.h"
@@ -250,6 +249,8 @@ bool PluginList::CreateWebPluginInfo(const PluginVersionInfo& pvi,
   info->version = WideToUTF16(pvi.file_version);
   info->path = pvi.path;
   info->enabled = true;
+  info->priority = 0;
+  info->reason = WebPluginInfo::USER;
 
   for (size_t i = 0; i < mime_types.size(); ++i) {
     WebPluginMimeType mime_type;
@@ -287,15 +288,6 @@ PluginList::PluginList()
   AddHardcodedPluginGroups();
 }
 
-bool PluginList::ShouldDisableGroup(const string16& group_name) {
-  AutoLock lock(lock_);
-  if (PluginGroup::IsPluginNameDisabledByPolicy(group_name)) {
-    disabled_groups_.insert(group_name);
-    return true;
-  }
-  return disabled_groups_.count(group_name) > 0;
-}
-
 void PluginList::LoadPlugins(bool refresh) {
   // 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.
@@ -347,7 +339,7 @@ void PluginList::LoadPlugins(bool refresh) {
   }
 
 #if defined(OS_WIN)
-  LoadPluginsFromRegistry(&new_plugins, &visited_plugins);
+  LoadPluginsFromRegistry(&visited_plugins, &new_plugins);
 #endif
 
   // Load the default plugin last.
@@ -355,31 +347,18 @@ void PluginList::LoadPlugins(bool refresh) {
     LoadPlugin(FilePath(kDefaultPluginLibraryName), &new_plugins);
 
   // Disable all of the plugins and plugin groups that are disabled by policy.
-  // There's currenly a bug that makes it impossible to correctly re-enable
-  // plugins or plugin-groups to their original, "pre-policy" state, so
-  // plugins and groups are only changed to a more "safe" state after a policy
-  // change, i.e. from enabled to disabled. See bug 54681.
   for (PluginGroup::PluginMap::iterator it = plugin_groups_.begin();
        it != plugin_groups_.end(); ++it) {
     PluginGroup* group = it->second;
     string16 group_name = group->GetGroupName();
-    if (ShouldDisableGroup(group_name)) {
+    if (PluginGroup::IsPluginNameDisabledByPolicy(group_name))
       group->Enable(false);
-    }
 
-    if (disable_outdated_plugins_) {
+    if (disable_outdated_plugins_)
       group->DisableOutdatedPlugins();
-    }
-    if (!group->Enabled()) {
-      AutoLock lock(lock_);
-      disabled_groups_.insert(group_name);
-    }
   }
 
-  // Only update the data now since loading plugins can take a while.
   AutoLock lock(lock_);
-
-  plugins_ = new_plugins;
   plugins_loaded_ = true;
 }
 
@@ -412,61 +391,20 @@ void PluginList::LoadPlugin(const FilePath& path,
     }
   }
 
-  // Mark disabled plugins as such. (This has to happen before calling
-  // |AddToPluginGroups(plugin_info)|.)
-  if (disabled_plugins_.count(plugin_info.path)) {
-    plugin_info.enabled = false;
-  } else {
-    plugin_info.enabled = true;
-  }
-
   AutoLock lock(lock_);
-  plugins->push_back(plugin_info);
   AddToPluginGroups(plugin_info);
+  plugins->push_back(plugin_info);
 }
 
-bool PluginList::SupportsType(const WebPluginInfo& info,
-                              const std::string &mime_type,
-                              bool allow_wildcard) {
-  // Webkit will ask for a plugin to handle empty mime types.
-  if (mime_type.empty())
-    return false;
-
-  for (size_t i = 0; i < info.mime_types.size(); ++i) {
-    const WebPluginMimeType& mime_info = info.mime_types[i];
-    if (net::MatchesMimeType(mime_info.mime_type, mime_type)) {
-      if (!allow_wildcard && mime_info.mime_type == "*") {
-        continue;
-      }
-      return true;
-    }
-  }
-  return false;
-}
-
-bool PluginList::SupportsExtension(const WebPluginInfo& info,
-                                   const std::string &extension,
-                                   std::string* actual_mime_type) {
-  for (size_t i = 0; i < info.mime_types.size(); ++i) {
-    const WebPluginMimeType& mime_type = info.mime_types[i];
-    for (size_t j = 0; j < mime_type.file_extensions.size(); ++j) {
-      if (mime_type.file_extensions[j] == extension) {
-        if (actual_mime_type)
-          *actual_mime_type = mime_type.mime_type;
-        return true;
-      }
-    }
-  }
-
-  return false;
-}
-
-
 void PluginList::GetPlugins(bool refresh, std::vector<WebPluginInfo>* plugins) {
   LoadPlugins(refresh);
 
   AutoLock lock(lock_);
-  *plugins = plugins_;
+  for (PluginGroup::PluginMap::const_iterator group = plugin_groups_.begin();
+       group != plugin_groups_.end(); ++group) {
+    const std::vector<WebPluginInfo>& gr_plugins = group->second->GetPlugins();
+    plugins->insert(plugins->end(), gr_plugins.begin(), gr_plugins.end());
+  }
 }
 
 void PluginList::GetEnabledPlugins(bool refresh,
@@ -475,11 +413,14 @@ void PluginList::GetEnabledPlugins(bool refresh,
 
   plugins->clear();
   AutoLock lock(lock_);
-  for (std::vector<WebPluginInfo>::const_iterator it = plugins_.begin();
-       it != plugins_.end();
-       ++it) {
-    if (it->enabled)
-      plugins->push_back(*it);
+  for (PluginGroup::PluginMap::const_iterator group = plugin_groups_.begin();
+       group != plugin_groups_.end(); ++group) {
+    const std::vector<WebPluginInfo>& gr_plugins = group->second->GetPlugins();
+    for (std::vector<WebPluginInfo>::const_iterator it = gr_plugins.begin();

[jam, 2010/12/17 19:14:45]

size_t

[pastarmovj, 2010/12/20 19:57:37]

Done.

+         it != gr_plugins.end(); ++it) {
+      if (it->IsEnabled())
+        plugins->push_back(*it);
+    }
   }
 }
 
@@ -502,15 +443,19 @@ void PluginList::GetPluginInfoArray(
 
   // Add in enabled plugins by mime type.
   WebPluginInfo default_plugin;
-  for (size_t i = 0; i < plugins_.size(); ++i) {
-    if (plugins_[i].enabled &&
-        SupportsType(plugins_[i], mime_type, allow_wildcard)) {
-      FilePath path = plugins_[i].path;
-      if (path.value() != kDefaultPluginLibraryName &&
-          visited_plugins.insert(path).second) {
-        info->push_back(plugins_[i]);
-        if (actual_mime_types)
-          actual_mime_types->push_back(mime_type);
+  for (PluginGroup::PluginMap::const_iterator group = plugin_groups_.begin();
+       group != plugin_groups_.end(); ++group) {
+    const std::vector<WebPluginInfo>& plugins = group->second->GetPlugins();
+    for (std::vector<WebPluginInfo>::const_iterator it = plugins.begin();

[jam, 2010/12/17 19:14:45]

size_t

[pastarmovj, 2010/12/20 19:57:37]

Done.

+         it != plugins.end(); ++it) {
+      if (it->IsEnabled() && it->SupportsType(mime_type, allow_wildcard)) {
+        FilePath path = it->path;
+        if (path.value() != kDefaultPluginLibraryName &&
+            visited_plugins.insert(path).second) {
+          info->push_back(*it);
+          if (actual_mime_types)
+            actual_mime_types->push_back(mime_type);
+        }
       }
     }
   }
@@ -521,42 +466,55 @@ void PluginList::GetPluginInfoArray(
   if (last_dot != std::string::npos) {
     std::string extension = StringToLowerASCII(std::string(path, last_dot+1));
     std::string actual_mime_type;
-    for (size_t i = 0; i < plugins_.size(); ++i) {
-      if (plugins_[i].enabled &&
-          SupportsExtension(plugins_[i], extension, &actual_mime_type)) {
-        FilePath path = plugins_[i].path;
-        if (path.value() != kDefaultPluginLibraryName &&
-            visited_plugins.insert(path).second) {
-          info->push_back(plugins_[i]);
-          if (actual_mime_types)
-            actual_mime_types->push_back(actual_mime_type);
+    for (PluginGroup::PluginMap::const_iterator group = plugin_groups_.begin();
+         group != plugin_groups_.end(); ++group) {
+      const std::vector<WebPluginInfo>& plugins = group->second->GetPlugins();
+      for (std::vector<WebPluginInfo>::const_iterator it = plugins.begin();

[jam, 2010/12/17 19:14:45]

size_t

[pastarmovj, 2010/12/20 19:57:37]

Done.

+           it != plugins.end(); ++it) {
+        if (it->IsEnabled() &&
+            it->SupportsExtension(extension, &actual_mime_type)) {
+          FilePath path = it->path;
+          if (path.value() != kDefaultPluginLibraryName &&
+              visited_plugins.insert(path).second) {
+            info->push_back(*it);
+            if (actual_mime_types)
+              actual_mime_types->push_back(actual_mime_type);
+          }
         }
       }
     }
   }
 
   // Add in disabled plugins by mime type.
-  for (size_t i = 0; i < plugins_.size(); ++i) {
-    if (!plugins_[i].enabled &&
-        SupportsType(plugins_[i], mime_type, allow_wildcard)) {
-      FilePath path = plugins_[i].path;
-      if (path.value() != kDefaultPluginLibraryName &&
-          visited_plugins.insert(path).second) {
-        info->push_back(plugins_[i]);
-        if (actual_mime_types)
-          actual_mime_types->push_back(mime_type);
+  for (PluginGroup::PluginMap::const_iterator group = plugin_groups_.begin();
+       group != plugin_groups_.end(); ++group) {
+    const std::vector<WebPluginInfo>& plugins = group->second->GetPlugins();
+    for (std::vector<WebPluginInfo>::const_iterator it = plugins.begin();

[jam, 2010/12/17 19:14:45]

size_t

[pastarmovj, 2010/12/20 19:57:37]

Done.

+         it != plugins.end(); ++it) {
+      if (!it->IsEnabled() && it->SupportsType(mime_type, allow_wildcard)) {
+        FilePath path = it->path;
+        if (path.value() != kDefaultPluginLibraryName &&
+            visited_plugins.insert(path).second) {
+          info->push_back(*it);
+          if (actual_mime_types)
+            actual_mime_types->push_back(mime_type);
+        }
       }
     }
   }
 
   // Add the default plugin at the end if it supports the mime type given,
   // and the default plugin is enabled.
-  if (!plugins_.empty() && webkit_glue::IsDefaultPluginEnabled()) {
-    const WebPluginInfo& default_info = plugins_.back();
-    if (SupportsType(default_info, mime_type, allow_wildcard)) {
-      info->push_back(default_info);
-      if (actual_mime_types)
-        actual_mime_types->push_back(mime_type);
+  for (PluginGroup::PluginMap::const_iterator group = plugin_groups_.begin();
+       group != plugin_groups_.end(); ++group) {
+    if (group->first.compare(kDefaultPluginLibraryName) == 0) {
+      DCHECK_NE(0U, group->second->GetPlugins().size());
+      const WebPluginInfo& default_info = group->second->GetPlugins().front();
+      if (default_info.SupportsType(mime_type, allow_wildcard)) {
+        info->push_back(default_info);
+        if (actual_mime_types)
+          actual_mime_types->push_back(mime_type);
+      }
     }
   }
 }
@@ -594,10 +552,15 @@ bool PluginList::GetPluginInfoByPath(const FilePath& plugin_path,
                                      WebPluginInfo* info) {
   LoadPlugins(false);
   AutoLock lock(lock_);
-  for (size_t i = 0; i < plugins_.size(); ++i) {
-    if (plugins_[i].path == plugin_path) {
-      *info = plugins_[i];
-      return true;
+  for (PluginGroup::PluginMap::const_iterator group = plugin_groups_.begin();
+       group != plugin_groups_.end(); ++group) {
+    const std::vector<WebPluginInfo>& plugins = group->second->GetPlugins();
+    for (std::vector<WebPluginInfo>::const_iterator it = plugins.begin();

[jam, 2010/12/17 19:14:45]

size_t

[pastarmovj, 2010/12/20 19:57:37]

Done.

+         it != plugins.end(); ++it) {
+      if (it->path == plugin_path) {
+        *info = *it;
+        return true;
+      }
     }
   }
 
@@ -609,6 +572,7 @@ void PluginList::GetPluginGroups(
     std::vector<PluginGroup>* plugin_groups) {
   if (load_if_necessary)
     LoadPlugins(false);
+  AutoLock lock(lock_);
   plugin_groups->clear();
   for (PluginGroup::PluginMap::const_iterator it = plugin_groups_.begin();
        it != plugin_groups_.end(); ++it) {
@@ -654,8 +618,10 @@ PluginGroup* PluginList::AddToPluginGroups(
   PluginGroup* group = NULL;
   for (PluginGroup::PluginMap::iterator it = plugin_groups_.begin();
        it != plugin_groups_.end(); ++it) {
-    if (it->second->Match(web_plugin_info))
+    if (it->second->Match(web_plugin_info)) {
       group = it->second;
+      break;
+    }
   }
   if (!group) {
     group = PluginGroup::FromWebPluginInfo(web_plugin_info);
@@ -671,88 +637,60 @@ PluginGroup* PluginList::AddToPluginGroups(
     DCHECK(plugin_groups_.find(identifier) == plugin_groups_.end());
     plugin_groups_.insert(std::make_pair(identifier, group));
   }
-  group->AddPlugin(web_plugin_info, next_priority_++);
+  bool is_new_addition = group->AddPlugin(web_plugin_info, next_priority_);
+  if (is_new_addition)
+    next_priority_++;
   return group;
 }
 
-bool PluginList::EnablePlugin(const FilePath& filename) {
-  AutoLock lock(lock_);
-
-  bool did_enable = false;
-
-  std::set<FilePath>::iterator entry = disabled_plugins_.find(filename);
-  if (entry == disabled_plugins_.end())
-    return did_enable;  // Early exit if plugin not in disabled list.
-
-  disabled_plugins_.erase(entry);  // Remove from disabled list.
+PluginGroup* PluginList::AddToPluginGroups(

[Bernhard Bauer, 2010/12/17 18:50:59]

There is already a method AddToPluginGroups, and we don't allow overloading.

Also, are you sure you're not going to get inconsistencies if you create a new
WebPluginInfo with the other fields empty?

[pastarmovj, 2010/12/20 19:57:37]

Renamed to AddPlaceholderToPluginGroup. I explicitly check if I am trying to add
new plugin or update such a placeholder in the AddToPluginGroup function to
prevent inconsistencies. This is still better than juggling with the
disabld_plugins_ list we had before.

+    const FilePath& filename, const string16& name) {
+  WebPluginInfo plugin_info;
+  plugin_info.path = filename;
+  plugin_info.name = name;
+  return AddToPluginGroups(plugin_info);
+}
 
-  // Set enabled flags if necessary.
-  for (std::vector<WebPluginInfo>::iterator it = plugins_.begin();
-       it != plugins_.end();
-       ++it) {
-    if (it->path == filename) {
-      DCHECK(!it->enabled);  // Should have been disabled.
-      it->enabled = true;
-      did_enable = true;
-    }
+bool PluginList::EnablePlugin(const FilePath& filename, const string16& name) {

[jam, 2010/12/17 19:14:45]

I still don't understand why name is needed :)

[pastarmovj, 2010/12/20 19:57:37]

For us to be able to have a meaningful placeholder for a plugin we need a bit
more than a filename. Especially because group names are built from plugin
names.

+  AutoLock lock(lock_);
+  for (PluginGroup::PluginMap::iterator it = plugin_groups_.begin();
+       it != plugin_groups_.end(); ++it) {
+    if (it->second->HasPlugin(filename))
+      return it->second->EnablePlugin(filename);
   }
-
-  return did_enable;
+  // No such group yet add one as a placeholder.

[Bernhard Bauer, 2010/12/17 18:50:59]

Nit: Add punctuation please: "No such group yet; add one..."

[pastarmovj, 2010/12/20 19:57:37]

Done.

+  PluginGroup* group = AddToPluginGroups(filename, name);
+  return group->EnablePlugin(filename);
 }
 
-bool PluginList::DisablePlugin(const FilePath& filename) {
+bool PluginList::DisablePlugin(const FilePath& filename, const string16& name) {
   AutoLock lock(lock_);
-
-  bool did_disable = false;
-
-  if (disabled_plugins_.find(filename) != disabled_plugins_.end())
-    return did_disable;  // Early exit if plugin already in disabled list.
-
-  disabled_plugins_.insert(filename);  // Add to disabled list.
-
-  // Unset enabled flags if necessary.
-  for (std::vector<WebPluginInfo>::iterator it = plugins_.begin();
-       it != plugins_.end();
-       ++it) {
-    if (it->path == filename) {
-      DCHECK(it->enabled);  // Should have been enabled.
-      it->enabled = false;
-      did_disable = true;
-    }
+  for (PluginGroup::PluginMap::iterator it = plugin_groups_.begin();
+       it != plugin_groups_.end(); ++it) {
+    if (it->second->HasPlugin(filename))
+      return it->second->DisablePlugin(filename);
   }
-
-  return did_disable;
+  // No such group yet add one as a placeholder.

[Bernhard Bauer, 2010/12/17 18:50:59]

Same nit here.

[pastarmovj, 2010/12/20 19:57:37]

Done.

+  PluginGroup* group = AddToPluginGroups(filename, name);
+  return group->DisablePlugin(filename);
 }
 
 bool PluginList::EnableGroup(bool enable, const string16& group_name) {
-  bool did_change = false;
-  {
-    AutoLock lock(lock_);
-
-    std::set<string16>::iterator entry = disabled_groups_.find(group_name);
-    if (enable) {
-      if (entry == disabled_groups_.end())
-        return did_change;  // Early exit if group not in disabled list.
-      disabled_groups_.erase(entry);  // Remove from disabled list.
-    } else {
-      if (entry != disabled_groups_.end())
-        return did_change;  // Early exit if group already in disabled list.
-      disabled_groups_.insert(group_name);
-    }
-  }
-
+  AutoLock lock(lock_);
+  PluginGroup* group = NULL;
   for (PluginGroup::PluginMap::iterator it = plugin_groups_.begin();
        it != plugin_groups_.end(); ++it) {
-    if (it->second->GetGroupName() == group_name) {
-      if (it->second->Enabled() != enable) {
-        it->second->Enable(enable);
-        did_change = true;
-        break;
-      }
+    if (it->second->GetGroupName().find(group_name) != string16::npos) {
+      group = it->second;
+      break;
     }
   }
+  if (!group) {
+    group = PluginGroup::CreateEmptyGroup(group_name);
+    plugin_groups_.insert(std::make_pair(UTF16ToUTF8(group_name), group));
+  }
 
-  return did_change;
+  return group->Enable(enable);
 }
 
 void PluginList::DisableOutdatedPluginGroups() {