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 d682cce24bcdeb28e9b52b352b6a65ea1b0ebc61..bca7dbb40ffe88d5571a1055fb7a4a5143d510e4 100644
--- a/webkit/glue/plugins/plugin_list.cc
+++ b/webkit/glue/plugins/plugin_list.cc
@@ -289,7 +289,7 @@ PluginList::PluginList()
 bool PluginList::ShouldDisableGroup(const string16& group_name) {
   AutoLock lock(lock_);
   if (PluginGroup::IsPluginNameDisabledByPolicy(group_name)) {
-    disabled_groups_.insert(group_name);
+    disabled_groups_.insert(DisabledGroupsListElement(group_name, POLICY));
     return true;
   }
   return disabled_groups_.count(group_name) > 0;
@@ -314,7 +314,7 @@ void PluginList::LoadPlugins(bool refresh) {
     internal_plugins = internal_plugins_;
   }
 
-  std::vector<WebPluginInfo> new_plugins;
+  std::vector<WebPluginInfo*> new_plugins;

[jam, 2010/12/15 18:25:50]

why is this change necessary?

[pastarmovj, 2010/12/15 21:56:04]

Before we got a copy of the plugins in PluginsList::plugins_ and in the
PluginGroup for that plugin. Therefore all code handling the state of plugin had
to make sure the state of these was kept consistent as both are used in
different contexts. However this tends to be not so trivial given the different
contexts these happen to occur. In order to resolve this problem it is better to
have only one instance of a WebPluginInfo per plugin. I haven't found any better
way than this though to do it yet.  Maybe using some sort of smart pointers can
be a better solution but still doesn't solve the problem who owns that element.
In general I see a long term solution in completely scraping the plugins_ member
in favor of using directly the groups. However this is a major refactoring that
is a bit outside the scope of that bugfix. 

I am open for other solutions here though.

   std::set<FilePath> visited_plugins;
 
   std::vector<FilePath> directories_to_scan;
@@ -346,7 +346,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.
@@ -371,19 +371,18 @@ void PluginList::LoadPlugins(bool refresh) {
     }
     if (!group->Enabled()) {
       AutoLock lock(lock_);
-      disabled_groups_.insert(group_name);
+      if (disabled_groups_.count(group_name) == 0)
+        disabled_groups_.insert(DisabledGroupsListElement(group_name, USER));
     }
   }
 
-  // Only update the data now since loading plugins can take a while.
   AutoLock lock(lock_);
-
   plugins_ = new_plugins;

[Bernhard Bauer, 2010/12/15 17:23:45]

What happens to the old pointers in |plugins_|? Do we leak them? What's the
reason we're now using pointers, BTW?

[pastarmovj, 2010/12/15 21:56:04]

About the reason for pointers please look at the response to John's question
above. 

As of the pointers being leaked - no, we don't leak then because the groups own
the objects behind these pointers. And because the groups are being built and
owned by the PluginList we can make sure the pointers never outlive the objects.
All functions that give information about plugins in the system only make copies
of these objects so they can't pass pointer that might get 0xDEADBEEF-ed while
in use.

   plugins_loaded_ = true;
 }
 
 void PluginList::LoadPlugin(const FilePath& path,
-                            std::vector<WebPluginInfo>* plugins) {
+                            std::vector<WebPluginInfo*>* plugins) {
   LOG_IF(ERROR, PluginList::DebugPluginLoading())
       << "Loading plugin " << path.value();
 
@@ -420,8 +419,7 @@ void PluginList::LoadPlugin(const FilePath& path,
   }
 
   AutoLock lock(lock_);
-  plugins->push_back(plugin_info);
-  AddToPluginGroups(plugin_info);
+  AddToPluginGroups(plugin_info, plugins);
 }
 
 bool PluginList::SupportsType(const WebPluginInfo& info,
@@ -465,7 +463,11 @@ void PluginList::GetPlugins(bool refresh, std::vector<WebPluginInfo>* plugins) {
   LoadPlugins(refresh);
 
   AutoLock lock(lock_);
-  *plugins = plugins_;
+  for (std::vector<WebPluginInfo*>::const_iterator it = plugins_.begin();
+       it != plugins_.end();
+       ++it) {
+    plugins->push_back(**it);
+  }
 }
 
 void PluginList::GetEnabledPlugins(bool refresh,
@@ -474,11 +476,11 @@ void PluginList::GetEnabledPlugins(bool refresh,
 
   plugins->clear();
   AutoLock lock(lock_);
-  for (std::vector<WebPluginInfo>::const_iterator it = plugins_.begin();
+  for (std::vector<WebPluginInfo*>::const_iterator it = plugins_.begin();
        it != plugins_.end();
        ++it) {
-    if (it->enabled)
-      plugins->push_back(*it);
+    if ((*it)->enabled)
+      plugins->push_back(**it);
   }
 }
 
@@ -502,12 +504,12 @@ 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 (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]);
+        info->push_back(*plugins_[i]);
         if (actual_mime_types)
           actual_mime_types->push_back(mime_type);
       }
@@ -521,12 +523,12 @@ void PluginList::GetPluginInfoArray(
     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 (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]);
+          info->push_back(*plugins_[i]);
           if (actual_mime_types)
             actual_mime_types->push_back(actual_mime_type);
         }
@@ -536,12 +538,12 @@ void PluginList::GetPluginInfoArray(
 
   // 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 (!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]);
+        info->push_back(*plugins_[i]);
         if (actual_mime_types)
           actual_mime_types->push_back(mime_type);
       }
@@ -551,7 +553,7 @@ void PluginList::GetPluginInfoArray(
   // 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();
+    const WebPluginInfo& default_info = *plugins_.back();
     if (SupportsType(default_info, mime_type, allow_wildcard)) {
       info->push_back(default_info);
       if (actual_mime_types)
@@ -594,8 +596,8 @@ bool PluginList::GetPluginInfoByPath(const FilePath& plugin_path,
   LoadPlugins(false);
   AutoLock lock(lock_);
   for (size_t i = 0; i < plugins_.size(); ++i) {
-    if (plugins_[i].path == plugin_path) {
-      *info = plugins_[i];
+    if (plugins_[i]->path == plugin_path) {
+      *info = *plugins_[i];
       return true;
     }
   }
@@ -608,6 +610,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) {
@@ -618,7 +621,7 @@ void PluginList::GetPluginGroups(
 const PluginGroup* PluginList::GetPluginGroup(
     const WebPluginInfo& web_plugin_info) {
   AutoLock lock(lock_);
-  return AddToPluginGroups(web_plugin_info);
+  return AddToPluginGroups(web_plugin_info, NULL);
 }
 
 string16 PluginList::GetPluginGroupName(std::string identifier) {
@@ -632,7 +635,7 @@ string16 PluginList::GetPluginGroupName(std::string identifier) {
 std::string PluginList::GetPluginGroupIdentifier(
     const WebPluginInfo& web_plugin_info) {
   AutoLock lock(lock_);
-  PluginGroup* group = AddToPluginGroups(web_plugin_info);
+  PluginGroup* group = AddToPluginGroups(web_plugin_info, NULL);
   return group->identifier();
 }
 
@@ -649,7 +652,8 @@ void PluginList::AddHardcodedPluginGroups() {
 }
 
 PluginGroup* PluginList::AddToPluginGroups(
-    const WebPluginInfo& web_plugin_info) {
+    const WebPluginInfo& web_plugin_info,
+    std::vector<WebPluginInfo*>* plugins) {
   PluginGroup* group = NULL;
   for (PluginGroup::PluginMap::iterator it = plugin_groups_.begin();
        it != plugin_groups_.end(); ++it) {
@@ -670,7 +674,25 @@ 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_++);
+  WebPluginInfo* group_plugin_copy;
+  bool is_new_addition = group->AddPlugin(
+      web_plugin_info, next_priority_, &group_plugin_copy);
+  if (plugins)
+    plugins->push_back(group_plugin_copy);
+  if (is_new_addition) {
+    next_priority_++;
+    string16 group_name = group->GetGroupName();
+    if (!group->Enabled() && !disabled_groups_.count(group_name)) {
+      disabled_groups_.insert(DisabledGroupsListElement(
+          group_name,
+          PluginGroup::IsPluginNameDisabledByPolicy(group_name) ?
+              POLICY : USER));
+    } else if (group->Enabled() && disabled_groups_.count(group_name)) {
+      disabled_groups_.erase(group_name);
+    }
+    // We don't need to protect the flag here because it is protected from the
+    // callers of |AddToPluginGroups|.
+  }
   return group;
 }
 
@@ -679,19 +701,29 @@ bool PluginList::EnablePlugin(const FilePath& filename) {
 
   bool did_enable = false;
 
-  std::set<FilePath>::iterator entry = disabled_plugins_.find(filename);
-  if (entry == disabled_plugins_.end())
+  DisabledPlugins::iterator entry =
+      disabled_plugins_.find(filename);
+  if (entry == disabled_plugins_.end() || entry->second == POLICY)
     return did_enable;  // Early exit if plugin not in disabled list.
 
   disabled_plugins_.erase(entry);  // Remove from disabled list.
 
   // Set enabled flags if necessary.
-  for (std::vector<WebPluginInfo>::iterator it = plugins_.begin();
+  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;
+    if ((*it)->path == filename) {
+      DCHECK(!(*it)->enabled);  // Should have been disabled.
+      (*it)->enabled = true;
+      PluginGroup* group = AddToPluginGroups(**it, NULL);
+      bool group_was_enabled = group->Enabled();
+      group->RefreshEnabledState();
+      if (group_was_enabled && group->Enabled()) {
+        DisabledGroups::iterator entry =
+            disabled_groups_.find(group->GetGroupName());
+        if (entry != disabled_groups_.end())
+          disabled_groups_.erase(entry);
+      }
       did_enable = true;
     }
   }
@@ -699,23 +731,40 @@ bool PluginList::EnablePlugin(const FilePath& filename) {
   return did_enable;
 }
 
-bool PluginList::DisablePlugin(const FilePath& filename) {
+bool PluginList::DisablePlugin(const FilePath& filename, bool policy_disabled) {
   AutoLock lock(lock_);
 
   bool did_disable = false;
 
-  if (disabled_plugins_.find(filename) != disabled_plugins_.end())
+  DisabledPlugins::iterator entry =
+      disabled_plugins_.find(filename);
+  if (entry != disabled_plugins_.end()) {
+    if ((entry->second == POLICY && !policy_disabled) ||
+        (entry->second == USER && policy_disabled))
+      entry->second = POLICY_AND_USER;
     return did_disable;  // Early exit if plugin already in disabled list.
+  }
 
-  disabled_plugins_.insert(filename);  // Add to disabled list.
+  // Add to disabled list.
+  disabled_plugins_.insert(
+      DisabledPluginsListElement(filename, policy_disabled ? POLICY : USER));
 
   // Unset enabled flags if necessary.
-  for (std::vector<WebPluginInfo>::iterator it = plugins_.begin();
+  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;
+    if ((*it)->path == filename) {
+      DCHECK((*it)->enabled);  // Should have been enabled.
+      (*it)->enabled = false;
+      PluginGroup* group = AddToPluginGroups(**it, NULL);
+      group->RefreshEnabledState();
+      if (!group->Enabled()) {
+        if (!disabled_groups_.count(group->GetGroupName())) {
+          disabled_groups_.insert(
+              DisabledGroupsListElement(group->GetGroupName(),
+                                        policy_disabled ? POLICY : USER));
+        }
+      }
       did_disable = true;
     }
   }
@@ -728,15 +777,20 @@ bool PluginList::EnableGroup(bool enable, const string16& group_name) {
   {
     AutoLock lock(lock_);
 
-    std::set<string16>::iterator entry = disabled_groups_.find(group_name);
+    DisabledGroups::iterator entry =
+        disabled_groups_.find(group_name);
     if (enable) {
-      if (entry == disabled_groups_.end())
+      if (entry == disabled_groups_.end() || entry->second == POLICY)
         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);
+      disabled_groups_.insert(
+          DisabledGroupsListElement(
+              group_name,
+              PluginGroup::IsPluginNameDisabledByPolicy(group_name) ?
+                  POLICY : USER));
     }
   }
 
@@ -758,6 +812,16 @@ void PluginList::DisableOutdatedPluginGroups() {
   disable_outdated_plugins_ = true;
 }
 
+const PluginList::DisabledPlugins&  PluginList::GetDisabledPlugins() const {
+  AutoLock lock(lock_);
+  return disabled_plugins_;
+}
+
+const PluginList::DisabledGroups& PluginList::GetDisabledGroups() const {
+  AutoLock lock(lock_);
+  return disabled_groups_;
+}
+
 PluginList::~PluginList() {
   Shutdown();
 }