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..78e74e7e66cc2f0b352308fa2a08d621b75e085b 100644
--- a/webkit/glue/plugins/plugin_list.cc
+++ b/webkit/glue/plugins/plugin_list.cc
@@ -166,6 +166,7 @@ bool PluginList::PluginsLoaded() {
 void PluginList::RefreshPlugins() {
   AutoLock lock(lock_);
   plugins_need_refresh_ = true;
+  plugins_vector_is_dirty_ = true;
 }
 
 void PluginList::AddExtraPluginPath(const FilePath& plugin_path) {
@@ -280,6 +281,7 @@ bool PluginList::CreateWebPluginInfo(const PluginVersionInfo& pvi,
 PluginList::PluginList()
     : plugins_loaded_(false),
       plugins_need_refresh_(false),
+      plugins_vector_is_dirty_(false),
       disable_outdated_plugins_(false),
       next_priority_(0) {
   PlatformInit();
@@ -289,7 +291,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 +316,6 @@ void PluginList::LoadPlugins(bool refresh) {
     internal_plugins = internal_plugins_;
   }
 
-  std::vector<WebPluginInfo> new_plugins;
   std::set<FilePath> visited_plugins;
 
   std::vector<FilePath> directories_to_scan;
@@ -326,23 +327,23 @@ void PluginList::LoadPlugins(bool refresh) {
   for (size_t i = 0; i < internal_plugins.size(); ++i) {
     if (internal_plugins[i].path.value() == kDefaultPluginLibraryName)
       continue;
-    LoadPlugin(internal_plugins[i].path, &new_plugins);
+    LoadPlugin(internal_plugins[i].path);
   }
 
   for (size_t i = 0; i < extra_plugin_paths.size(); ++i) {
     const FilePath& path = extra_plugin_paths[i];
     if (visited_plugins.find(path) != visited_plugins.end())
       continue;
-    LoadPlugin(path, &new_plugins);
+    LoadPlugin(path);
     visited_plugins.insert(path);
   }
 
   for (size_t i = 0; i < extra_plugin_dirs.size(); ++i) {
-    LoadPluginsFromDir(extra_plugin_dirs[i], &new_plugins, &visited_plugins);
+    LoadPluginsFromDir(extra_plugin_dirs[i], &visited_plugins);
   }
 
   for (size_t i = 0; i < directories_to_scan.size(); ++i) {
-    LoadPluginsFromDir(directories_to_scan[i], &new_plugins, &visited_plugins);
+    LoadPluginsFromDir(directories_to_scan[i], &visited_plugins);
   }
 
 #if defined(OS_WIN)
@@ -351,7 +352,7 @@ void PluginList::LoadPlugins(bool refresh) {
 
   // Load the default plugin last.
   if (webkit_glue::IsDefaultPluginEnabled())
-    LoadPlugin(FilePath(kDefaultPluginLibraryName), &new_plugins);
+    LoadPlugin(FilePath(kDefaultPluginLibraryName));
 
   // 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
@@ -371,19 +372,40 @@ void PluginList::LoadPlugins(bool refresh) {
     }
     if (!group->Enabled()) {
       AutoLock lock(lock_);
-      disabled_groups_.insert(group_name);
+      if(0 == disabled_groups_.count(group_name))

[danno, 2010/12/14 09:43:39]

use == 0 formatting (reverse order). compiler gives warning/error on inadvertent
assignment. 

[pastarmovj, 2010/12/15 14:44:51]

Done.

+        disabled_groups_.insert(DisabledGroupsListElement(group_name, USER));
     }
   }
 
   // Only update the data now since loading plugins can take a while.
+  RebuildPluginsList();

[danno, 2010/12/14 09:43:39]

You also acquire the lock inside of RebuildPluginList. Do you really need to
re-acquire it below?

[pastarmovj, 2010/12/15 14:44:51]

I have to for the plugins_loaded_ flag. However this code has/will change anyway
and RebuildPluginsList won't get called.

+  //plugins_ = new_plugins;
   AutoLock lock(lock_);
+  plugins_loaded_ = true;
+}
+
+void PluginList::RebuildPluginsList() {
+  AutoLock lock(lock_);
+  if(!plugins_vector_is_dirty_) return;
+  plugins_vector_is_dirty_ = false;
 
+  std::vector<WebPluginInfo*> new_plugins;
+  new_plugins.resize(this->next_priority_);
+  for (PluginGroup::PluginMap::iterator it = plugin_groups_.begin();
+       it != plugin_groups_.end(); ++it) {
+    std::vector<WebPluginInfo>& group_plugins = it->second->GetPlugins();
+    std::vector<int>::iterator itprio =
+        it->second->GetPluginPositions().begin();
+    for (std::vector<WebPluginInfo>::iterator itp = group_plugins.begin();
+         itp != group_plugins.end();
+         ++itp,++itprio) {
+      new_plugins[*itprio] = &(*itp);

[danno, 2010/12/14 09:43:39]

Whoa. Taking the address of a dereferenced iterator makes my eyeballs itch, and
has an odd "smell". Are you sure that the lifecycles of the WebPluginInfos in
group_plugins outlive plugns_ always? If so, it needs to be clearly documented
in the the header for the plugin_ member. I suspect there was a good reason for
copying them in the old version.

[pastarmovj, 2010/12/15 14:44:51]

Cool right ;)

+    }
+  }
   plugins_ = new_plugins;
-  plugins_loaded_ = true;
 }
 
-void PluginList::LoadPlugin(const FilePath& path,
-                            std::vector<WebPluginInfo>* plugins) {
+void PluginList::LoadPlugin(const FilePath& path) {

[Bernhard Bauer, 2010/12/14 04:21:49]

Drive-by: Removing the |plugins| parameter may seem like a good idea, but it's
not. 

As your CL stands right now, if multiple threads call  LoadPlugins at the same
time, we will possibly end up with duplicates in the plugin list. We don't use a
lock around the whole LoadPlugins method for performance reasons, so we
construct the new plugins array separately, and then only swap it out at the end
(hence the extra |plugins| parameter).

Jakob, for AddToPluginGroups we should probably add an additional parameter as
well. Sorry I didn't catch that in the first CL.

[pastarmovj, 2010/12/15 14:44:51]

Done.

   LOG_IF(ERROR, PluginList::DebugPluginLoading())
       << "Loading plugin " << path.value();
 
@@ -393,7 +415,7 @@ void PluginList::LoadPlugin(const FilePath& path,
   if (!ReadPluginInfo(path, &plugin_info, &entry_points))
     return;
 
-  if (!ShouldLoadPlugin(plugin_info, plugins))
+  if (!ShouldLoadPlugin(plugin_info))
     return;
 
   if (path.value() != kDefaultPluginLibraryName
@@ -420,7 +442,6 @@ void PluginList::LoadPlugin(const FilePath& path,
   }
 
   AutoLock lock(lock_);
-  plugins->push_back(plugin_info);
   AddToPluginGroups(plugin_info);
 }
 
@@ -465,7 +486,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 +499,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 +527,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 +546,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 +561,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 +576,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 +619,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;
     }
   }
@@ -670,28 +695,35 @@ 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_++);
+  if(group->AddPlugin(web_plugin_info, next_priority_++)){
+    // We don't need to protect the flag here because it is protected from the
+    // callers of |AddToPluginGroups|.
+    plugins_vector_is_dirty_ = true;
+  }
   return group;
 }
 
 bool PluginList::EnablePlugin(const FilePath& filename) {
+  RebuildPluginsList();
   AutoLock lock(lock_);
 
   bool did_enable = false;
 
-  std::set<FilePath>::iterator entry = disabled_plugins_.find(filename);
-  if (entry == disabled_plugins_.end())
+  DisabledPluginsList::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;
+      AddToPluginGroups(**it)->RefreshEnabledState();
       did_enable = true;
     }
   }
@@ -699,23 +731,34 @@ bool PluginList::EnablePlugin(const FilePath& filename) {
   return did_enable;
 }
 
-bool PluginList::DisablePlugin(const FilePath& filename) {
+bool PluginList::DisablePlugin(const FilePath& filename, bool policy_disabled) {
+  RebuildPluginsList();
   AutoLock lock(lock_);
 
   bool did_disable = false;
 
-  if (disabled_plugins_.find(filename) != disabled_plugins_.end())
+  DisabledPluginsList::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;
+      AddToPluginGroups(**it)->RefreshEnabledState();
       did_disable = true;
     }
   }
@@ -728,15 +771,20 @@ bool PluginList::EnableGroup(bool enable, const string16& group_name) {
   {
     AutoLock lock(lock_);
 
-    std::set<string16>::iterator entry = disabled_groups_.find(group_name);
+    DisabledGroupsList::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 +806,16 @@ void PluginList::DisableOutdatedPluginGroups() {
   disable_outdated_plugins_ = true;
 }
 
+void PluginList::GetDisabledPlugins(DisabledPluginsList* disabled_plugins) {
+  AutoLock lock(lock_);
+  *disabled_plugins = disabled_plugins_;
+}
+
+void PluginList::GetDisabledGroups(DisabledGroupsList* disabled_groups) {
+  AutoLock lock(lock_);
+  *disabled_groups = disabled_groups_;
+}
+
 PluginList::~PluginList() {
   Shutdown();
 }