Migrate enabled state of plugins into specialized Flash and PDF prefs.

chrome/browser/plugins/plugin_prefs.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/plugins/plugin_prefs.h"
 
 #include <stddef.h>
 
 #include <memory>
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/path_service.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/pattern.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/plugins/plugin_installer.h"
 #include "chrome/browser/plugins/plugin_metadata.h"
 #include "chrome/browser/plugins/plugin_prefs_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_content_client.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
+#include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/content_settings/core/common/content_settings.h"
+#include "components/content_settings/core/common/pref_names.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/plugin_service.h"
+#include "content/public/common/content_constants.h"
 #include "content/public/common/webplugininfo.h"
 
 #if !defined(DISABLE_NACL)
 #include "components/nacl/common/nacl_constants.h"
 #endif
 
 using content::BrowserThread;
 using content::PluginService;
 
 namespace {
 
 bool IsComponentUpdatedPepperFlash(const base::FilePath& plugin) {
   if (plugin.BaseName().value() == chrome::kPepperFlashPluginFilename) {
     base::FilePath component_updated_pepper_flash_dir;
     if (PathService::Get(chrome::DIR_COMPONENT_UPDATED_PEPPER_FLASH_PLUGIN,
                          &component_updated_pepper_flash_dir) &&
         component_updated_pepper_flash_dir.IsParent(plugin)) {
       return true;
     }
   }
 
   return false;
 }
 
 bool IsPDFViewerPlugin(const base::string16& plugin_name) {
   return plugin_name == base::ASCIIToUTF16(ChromeContentClient::kPDFPluginName);
 }
 
+bool IsAdobeFlashPlayerPlugin(const base::string16& plugin_name) {
+  return (plugin_name == base::ASCIIToUTF16(content::kFlashPluginName) ||
+          plugin_name == base::ASCIIToUTF16("Adobe Flash Player"));

[Bernhard Bauer, 2016/11/11 12:15:24]

Would it make sense to move this to a constant in chrome/ rather than
duplicating it over and over?

[pastarmovj, 2016/11/14 13:58:00]

Done.

+}
+
 }  // namespace
 
 PluginPrefs::PluginState::PluginState() {
 }
 
 PluginPrefs::PluginState::~PluginState() {
 }
 
 bool PluginPrefs::PluginState::Get(const base::FilePath& plugin,
                                    bool* enabled) const {
@@ skipping 254 matching lines @@
     base::string16 plugin_name;
     if ((*current)->GetAsString(&plugin_name)) {
       dest->insert(plugin_name);
     }
   }
 }
 
 void PluginPrefs::SetPrefs(PrefService* prefs) {
   prefs_ = prefs;
   bool update_internal_dir = false;
+  bool plugins_migrated = false;
   base::FilePath last_internal_dir =
       prefs_->GetFilePath(prefs::kPluginsLastInternalDirectory);
   base::FilePath cur_internal_dir;
   if (PathService::Get(chrome::DIR_INTERNAL_PLUGINS, &cur_internal_dir) &&
       cur_internal_dir != last_internal_dir) {
     update_internal_dir = true;
     prefs_->SetFilePath(
         prefs::kPluginsLastInternalDirectory, cur_internal_dir);
   }
 
   {  // Scoped update of prefs::kPluginsPluginsList.
     ListPrefUpdate update(prefs_, prefs::kPluginsPluginsList);
     base::ListValue* saved_plugins_list = update.Get();
     if (saved_plugins_list && !saved_plugins_list->empty()) {
       for (const auto& plugin_value : *saved_plugins_list) {
         base::DictionaryValue* plugin;
         if (!plugin_value->GetAsDictionary(&plugin)) {
           LOG(WARNING) << "Invalid entry in " << prefs::kPluginsPluginsList;
           continue;  // Oops, don't know what to do with this item.
         }
 
-        base::string16 group_name;
+        base::string16 name;
+        if (!plugin->GetString("name", &name)) {
+          // This should not happen in the post NPAPI world. I remember that my
+          // initial assumption was the same about NPAPI plugins back then and
+          // and then was bitterly surprised when my change hit the canary and
+          // not handling well the empty names became a top crasher. Not so bad
+          // this time but we'd rather not CHECK on that.

[Bernhard Bauer, 2016/11/11 12:15:24]

Couple of things:
1) This explanation seems a bit too detailed (and really, subjective), in
particular given that it's not clear who "I" is.
2) The code doesn't actually do anything apart from logging, so I don't see how
it would prevent a CHECK.
3) If you say "X should not happen", don't add a check for X. In this particular
case, we either make it a precondition that the name is non-empty, in which case
we DCHECK it, and accept crashes in case that turns out to be untrue (which is
fine, because that's actually the only way we'll learn about it!), or we
outright accept any name, but then we shouldn't spam the log about it either.
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
has some good guidance there.

[pastarmovj, 2016/11/14 13:58:00]

 Restructued this code. It doesn't matter anymore just like it didn't before.

+          LOG(WARNING) << "Nameless plugin detected in the post-NPAPI world?!";
+        }
+
         bool enabled;
         if (!plugin->GetBoolean("enabled", &enabled))
           enabled = true;
 
+        // Migrate disabled plugins and re-enabled them all internally.
+        // TODO(pastarmovj): crbug/662006: Remove migration eventually.

[Bernhard Bauer, 2016/11/11 12:15:24]

Nit: For ideal clickability, use a full URL: https://crbug/com/662006.

[pastarmovj, 2016/11/14 13:58:00]

Was trying to save a line :) changed.

[Bernhard Bauer, 2016/11/14 14:35:38]

Gotcha. FWIW, TODO(https://crbug.com/662006) is also acceptable (even prefered
by some people) and would also be shorter :)

+        if (!enabled) {
+          if (IsPDFViewerPlugin(name))
+            prefs->SetBoolean(prefs::kPluginsAlwaysOpenPdfExternally, true);
+          if (IsAdobeFlashPlayerPlugin(name)) {
+            HostContentSettingsMapFactory::GetForProfile(profile_)
+                ->SetDefaultContentSetting(CONTENT_SETTINGS_TYPE_PLUGINS,
+                                           CONTENT_SETTING_BLOCK);
+          }
+          // TODO(pastarmovj): crbug/662002: Remove the enabled flag completely.
+          enabled = true;
+          plugin->SetBoolean("enabled", true);
+          plugins_migrated = true;

[Bernhard Bauer, 2016/11/11 12:15:24]

So, this would set the flag even if the migrated plugin is neither the PDF
viewer nor Flash. Is that really what we want?

[pastarmovj, 2016/11/14 13:58:00]

Yes. I want to know that any plugin was user-disabled (which is going to be
undone so once per-user, provided that the about:plugins page is gone). This way
we can measure when we have reset [almost]all visible in uma instances.

+        }
+
         base::FilePath::StringType path;
         // The plugin list constains all the plugin files in addition to the
         // plugin groups.
         if (plugin->GetString("path", &path)) {
           // Files have a path attribute, groups don't.
           base::FilePath plugin_path(path);
 
           // The path to the internal plugin directory changes everytime Chrome
           // is auto-updated, since it contains the current version number. For
           // example, it changes from foobar\Chrome\Application\21.0.1180.83 to
@@ skipping 26 matching lines @@
 
             // If |relative_path| is empty, |plugin_path| is not within
             // |last_internal_dir|. We don't need to update it.
             if (!relative_path.empty()) {
               plugin_path = cur_internal_dir.Append(relative_path);
               path = plugin_path.value();
               plugin->SetString("path", path);
             }
           }
 
           plugin_state_.Set(plugin_path, enabled);

[Bernhard Bauer, 2016/11/11 12:15:24]

So, if we enable all plugins anyway, why do we even need this state?

[pastarmovj, 2016/11/14 13:58:00]

Not anymore but untangling this is going to make this CL too complex. This is
the plan for http://crbug.com/662002 anyways.

-        } else if (!enabled && plugin->GetString("name", &group_name)) {
-          // Otherwise this is a list of groups.
-          plugin_group_state_[group_name] = false;
         }
       }
     } else {
       // If the saved plugin list is empty, then the call to UpdatePreferences()
       // below failed in an earlier run, possibly because the user closed the
       // browser too quickly.
 
       // Only want one PDF plugin enabled at a time. See http://crbug.com/50105
       // for background.
       plugin_group_state_[base::ASCIIToUTF16(
           PluginMetadata::kAdobeReaderGroupName)] = false;
     }
   }  // Scoped update of prefs::kPluginsPluginsList.
 
+  UMA_HISTOGRAM_BOOLEAN("Plugin.EnabledStatusMigrationDone", plugins_migrated);
+
   // Build the set of policy enabled/disabled plugin patterns once and cache it.
   // Don't do this in the constructor, there's no profile available there.
+  // TODO(pastarmovj): crbug/662002: Remove the prefs completely.
   ListValueToStringSet(prefs_->GetList(prefs::kPluginsDisabledPlugins),
                        &policy_disabled_plugin_patterns_);
   ListValueToStringSet(
       prefs_->GetList(prefs::kPluginsDisabledPluginsExceptions),
       &policy_disabled_plugin_exception_patterns_);
   ListValueToStringSet(prefs_->GetList(prefs::kPluginsEnabledPlugins),
                        &policy_enabled_plugin_patterns_);
   always_open_pdf_externally_ =
       prefs_->GetBoolean(prefs::kPluginsAlwaysOpenPdfExternally);
 
@@ skipping 99 matching lines @@
   }
 }
 
 void PluginPrefs::NotifyPluginStatusChanged() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_PLUGIN_ENABLE_STATUS_CHANGED,
       content::Source<Profile>(profile_),
       content::NotificationService::NoDetails());
 }