[Extensions] Fix a crash in ExtensionPrefValueMap

When extensions are disabled with --disable-extensions, ExtensionPrefs
would completely skip initializing the Extension-controlled preferences
(which encompasses the content settings overrides, preferences API, and
chrome settings overrides). This would result in no entry being added
to the ExtensionPrefValueMap for extensions if --disable-extensions was
used. This is generally desirable, since otherwise the settings may
still take effect, even though the extensions were not enabled.

However, some extensions are still loaded with --disable-extensions.
Obviously, this includes component extensions, but this also
(surprisingly) includes external extensions, such as those added by
the windows registry.

The result of this is that if an external extension used an API that
relied on the ExtensionPrefValueMap, such as the chrome settings
overrides, while --disable-extensions was used, there would be no
entry in the value map. Chrome would then crash.

Fix this by having ExtensionPrefs check if the extension will still be
loaded, even if extensions are disabled. If it will, always load the
corresponding extension-controlled preferences.

Add two tests to cover this case: one general unit test to ensure the
extension-controlled prefs are initialized for external extensions
(which will always be loaded), and one end-to-end test with adding an
external extension using the chrome settings overrides API.

Bug: 828295
Change-Id: I0b01f6ed2fd41874f8fa47378f111c0692cb1ea5
Reviewed-on: https://chromium-review.googlesource.com/1013305Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552209}

chrome/browser/extensions/api/preference/preference_api_prefs_unittest.cc

@@ -156,10 +156,8 @@ void ExtensionControlledPrefsTest::UninstallExtension(
 void ExtensionControlledPrefsTest::EnsureExtensionInstalled(
     Extension* extension) {
   // Install extension the first time a preference is set for it.
-  Extension* extensions[] = { extension1(),
-                              extension2(),
-                              extension3(),
-                              extension4() };
+  Extension* extensions[] = {extension1(), extension2(), extension3(),
+                             extension4(), internal_extension()};
   for (size_t i = 0; i < kNumInstalledExtensions; ++i) {
     if (extension == extensions[i] && !installed_[i]) {
       prefs()->OnExtensionInstalled(extension,
@@ -175,10 +173,8 @@ void ExtensionControlledPrefsTest::EnsureExtensionInstalled(
 
 void ExtensionControlledPrefsTest::EnsureExtensionUninstalled(
     const std::string& extension_id) {
-  Extension* extensions[] = { extension1(),
-                              extension2(),
-                              extension3(),
-                              extension4() };
+  Extension* extensions[] = {extension1(), extension2(), extension3(),
+                             extension4(), internal_extension()};
   for (size_t i = 0; i < kNumInstalledExtensions; ++i) {
     if (extensions[i]->id() == extension_id) {
       installed_[i] = false;
@@ -439,19 +435,33 @@ class ControlledPrefsDisableExtensions : public ExtensionControlledPrefsTest {
       : iteration_(0) {}
   ~ControlledPrefsDisableExtensions() override {}
   void Initialize() override {
-    InstallExtensionControlledPref(extension1(), kPref1,
-                                   new base::Value("val1"));
+    InstallExtensionControlledPref(internal_extension(), kPref1,
+                                   new base::Value("internal extension value"));
+
+    EXPECT_TRUE(Manifest::IsExternalLocation(extension1()->location()));
+    InstallExtensionControlledPref(extension1(), kPref2,
+                                   new base::Value("external extension value"));
     // This becomes only active in the second verification phase.
     prefs_.set_extensions_disabled(true);
   }
   void Verify() override {
-    std::string actual = prefs()->pref_service()->GetString(kPref1);
+    // Internal extensions are not loaded with --disable-extensions. This means
+    // that the preference will be reset on the second verification run (when
+    // the ExtensionPrefs are recreated).
+    std::string pref1_actual = prefs()->pref_service()->GetString(kPref1);
     if (iteration_ == 0) {
-      EXPECT_EQ("val1", actual);
+      EXPECT_EQ("internal extension value", pref1_actual);
       ++iteration_;
     } else {
-      EXPECT_EQ(kDefaultPref1, actual);
+      EXPECT_EQ(kDefaultPref1, pref1_actual);
     }
+
+    // External extensions are loaded even when extensions are disabled (though
+    // they likely shouldn't be, see https://crbug.com/833540). Because of this,
+    // the preference should still be controlled by the external extension.
+    // Regression test for https://crbug.com/828295.
+    std::string pref2_actual = prefs()->pref_service()->GetString(kPref2);
+    EXPECT_EQ("external extension value", pref2_actual);
   }
 
  private:

chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc

@@ -9,11 +9,13 @@
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/prefs/session_startup_pref.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/search_engines/ui_thread_search_terms_data.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/search_test_utils.h"
 #include "components/prefs/pref_service.h"
@@ -23,8 +25,14 @@
 #include "components/search_engines/template_url_prepopulate_data.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/version_info/version_info.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/mock_external_provider.h"
+#include "extensions/browser/test_extension_registry_observer.h"
+#include "extensions/common/feature_switch.h"
 #include "extensions/common/features/feature_channel.h"
 
+namespace extensions {
+
 namespace {
 #if defined(OS_WIN) || defined(OS_MACOSX)
 // Prepopulated id hardcoded in test_extension.
@@ -224,6 +232,71 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, BeforeTemplateUrlServiceLoad) {
   EXPECT_FALSE(url_service->IsExtensionControlledDefaultSearch());
 }
 
+constexpr char kExternalId[] = "eloigjjcaheakkihngcgjhbopomgemdj";
+
+class ExtensionsDisabledWithSettingsOverrideAPI : public ExtensionBrowserTest {
+ public:
+  ExtensionsDisabledWithSettingsOverrideAPI()
+      : prompt_for_external_extensions_(
+            FeatureSwitch::prompt_for_external_extensions(),
+            false) {}
+  ~ExtensionsDisabledWithSettingsOverrideAPI() override = default;
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    // A little tricky: we disable extensions (via the commandline) on the
+    // non-PRE run. The PRE run is responsible for installing the external
+    // extension.
+    ExtensionBrowserTest::SetUpCommandLine(command_line);
+    const char* test_name =
+        testing::UnitTest::GetInstance()->current_test_info()->name();
+    if (!base::StartsWith(test_name, "PRE_", base::CompareCase::SENSITIVE)) {
+      command_line->AppendSwitch(::switches::kDisableExtensions);
+    }
+  }
+
+ private:
+  FeatureSwitch::ScopedOverride prompt_for_external_extensions_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionsDisabledWithSettingsOverrideAPI);
+};
+
+// The following test combo is a regression test for https://crbug.com/828295.
+// When extensions are disabled with --disable-extensions, no
+// extension-controlled prefs were loaded. However, external extensions (such as
+// those from policy or specified in the registry) are still loaded with
+// --disable-extensions (this itself is somewhat strange; see
+// https://crbug.com/833540). This caused a CHECK failure in the settings
+// overrides API when an external extension used the API and
+// --disable-extensions was also used.
+// As a fix, we ensure that we load extension-controlled preferences for
+// extensions that will be loaded, even with --disable-extensions.
+IN_PROC_BROWSER_TEST_F(ExtensionsDisabledWithSettingsOverrideAPI,
+                       PRE_TestSettingsOverridesWithExtensionsDisabled) {
+  // This first part of the test adds an external extension that uses the
+  // settings overrides API.
+  ExtensionRegistry* registry = ExtensionRegistry::Get(profile());
+  TestExtensionRegistryObserver observer(registry);
+  auto provider = std::make_unique<MockExternalProvider>(
+      extension_service(), Manifest::EXTERNAL_PREF);
+  provider->UpdateOrAddExtension(
+      kExternalId, "2.1",
+      test_data_dir_.AppendASCII("api_test/settings_overrides/homepage.crx"));
+  extension_service()->AddProviderForTesting(std::move(provider));
+  extension_service()->CheckForExternalUpdates();
+  const Extension* extension = observer.WaitForExtensionLoaded();
+  EXPECT_EQ(kExternalId, extension->id());
+}
+IN_PROC_BROWSER_TEST_F(ExtensionsDisabledWithSettingsOverrideAPI,
+                       TestSettingsOverridesWithExtensionsDisabled) {
+  // The external extension was actually uninstalled at this point (see
+  // https://crbug.com/833540). However, it was first loaded, before being
+  // orphaned, which would have caused the settings API to look at the
+  // extension controlled preferences. As long as this didn't crash, the test
+  // succeeded.
+  EXPECT_TRUE(ExtensionsBrowserClient::Get()->AreExtensionsDisabled(
+      *base::CommandLine::ForCurrentProcess(), profile()));
+}
+
 #else
 IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, SettingsOverridesDisallowed) {
   const extensions::Extension* extension =
@@ -238,3 +311,4 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, SettingsOverridesDisallowed) {
 #endif
 
 }  // namespace
+}  // namespace extensions

chrome/browser/extensions/extension_prefs_unittest.cc

@@ -762,6 +762,10 @@ PrefsPrepopulatedTestBase::PrefsPrepopulatedTestBase()
       Extension::NO_FLAGS,
       &error);
 
+  internal_extension_ = Extension::Create(
+      prefs_.temp_dir().AppendASCII("internal extension"), Manifest::INTERNAL,
+      simple_dict, Extension::NO_FLAGS, &error);
+
   for (size_t i = 0; i < kNumInstalledExtensions; ++i)
     installed_[i] = false;
 }

chrome/browser/extensions/extension_prefs_unittest.h

@@ -56,7 +56,7 @@ class ExtensionPrefsTest : public testing::Test {
 
 class PrefsPrepopulatedTestBase : public ExtensionPrefsTest {
  public:
-  static const size_t kNumInstalledExtensions = 4;
+  static const size_t kNumInstalledExtensions = 5;
 
   PrefsPrepopulatedTestBase();
   ~PrefsPrepopulatedTestBase() override;
@@ -65,15 +65,20 @@ class PrefsPrepopulatedTestBase : public ExtensionPrefsTest {
   Extension* extension2() { return extension2_.get(); }
   Extension* extension3() { return extension3_.get(); }
   Extension* extension4() { return extension4_.get(); }
+  Extension* internal_extension() { return internal_extension_.get(); }
 
  protected:
   bool installed_[kNumInstalledExtensions];
 
+  // The following extensions all have Manifest::Location set to EXTERNAL_PREF.
   scoped_refptr<Extension> extension1_;
   scoped_refptr<Extension> extension2_;
   scoped_refptr<Extension> extension3_;
   scoped_refptr<Extension> extension4_;
 
+  // This extension has a location of Manifest::INTERNAL.
+  scoped_refptr<Extension> internal_extension_;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(PrefsPrepopulatedTestBase);
 };

chrome/browser/extensions/extension_service.cc

@@ -1200,9 +1200,9 @@ void ExtensionService::AddExtension(const Extension* extension) {
   // TODO(jstritar): We may be able to get rid of this branch by overriding the
   // default extension state to DISABLED when the --disable-extensions flag
   // is set (http://crbug.com/29067).
-  if (!extensions_enabled() && !extension->is_theme() &&
-      extension->location() != Manifest::COMPONENT &&
-      !Manifest::IsExternalLocation(extension->location()) &&
+  if (!extensions_enabled_ &&
+      !Manifest::ShouldAlwaysLoadExtension(extension->location(),
+                                           extension->GetType()) &&
       disable_flag_exempted_extensions_.count(extension->id()) == 0) {
     return;
   }

chrome/test/data/extensions/api_test/settings_overrides/homepage.pem

+-----BEGIN PRIVATE KEY-----
+MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCv1RzunlaktPSq
+Is2RBDdiUvShsx8BZ7Hb+RxTBZk3RSST1nGfSyNJ0k0KWx9EnO5sfcOybgoyfDA8
+rRw49InK26+xRlyFQUgd3O+sv6hsDxL/MR2Zta1tuzY6GtZjAQEEjlJwJoGinHZu
+yInWx9ok8gIIV0yDhcY69UQ+w6nP25PTWUlxYWFrsGCNKtYpjQHV1alJYrY8IbZS
+jYKllaV2O3LCOr2OOg4QWA8XWhUsYbcDhU4ho58+5EQasCeCG1l8VhSb7O+HwPx8
+9Unkr3/ByXD0XVglQrdo/QSst2ISLhrY0WkJw2gRsp8Q5k35+xZt2SOVJNKhOhVu
+KonjikcdAgMBAAECggEAPop4UH18RvhgP/XCFW8g/fWQssUh5Z+K+TmJdBQ9M5YB
+1R3E60pQAm1tXp4bqI2DFo7DIT+Xm+5lP+e47p84NbHfM5hZx0H5kzmm/FRtZQYV
+lTRUmXwv9CRPPOqXz74nLQYsV1yy5nJtg6kLshwCwPpdc7cNbYiTAhyECikRSO9z
+XYXbqkcU+LJ46acqxMzude7+efYwybIn0guH8AlW4mOnw99+Oo/S4WrbwYKnk80d
+YMysuGaf2rhxcmL03pR+pzoCy17qdbsmpWOw3DnsdtS6HF5V5yMtiyEXHTFP542d
+D+dSYo8J4m0AkQ9fmWM/7sbMbHDdmu1CTuOPb2vj9wKBgQDg3Y0YTaj3SfzUF6CN
+Q0AFpwaWsT66oLU6qD508yOXTJRuNH5f94mFtuBndR1nf4KUza74qMdmj2Yzx7Zs
+NJOABfv3Gztg+8vOtXXQtosmIEMskMxkfOvB3aczZLEKYH/owVmZG/5JbR9B46BI
+CjM/jFgc51UeWPvjR84LCEZdpwKBgQDILZFyPCGLURQ+2itBwY1ar5pThBMEpCZU
+KQGc5fzBbX/2pmCBCdqEooYRKEwj7oxkVyVGBlyd4enwmTaMUhnJ/dIiO1mNkruG
+FL4sYJdK//l+JgzWtt+rNULOAAP1VtSIfmZUtzB+OJxyQabQFpJCYpYBXc76mQ0h
+ZRky3ZQ1mwKBgGxivdCaFjhj5EbCwLN2V9Shpm5aLrc8IT6Nu9UZIO2bjuvzx3ow
+UYwvEskrTSoG7wUUgyL+D2bi9cs9lzNewD30JHf+G0fq9ljDZG9enYGaCWbgwHQr
+NmPyl9YQ0bkDvIOPNsWzFYWE4I0cz7QJnpwh5JLB+mgvUfJCORFeQrR/AoGARlJz
+yQzEv/uAwD/GeHg1X0lXTsWUJMpByMkLDwJgk9cS4CQ8vLgocvXhYT+l1+2illFi
+/8GWYcSOeX5pgkwJUUOAwZPV9v04MXcyA8b+/1MDY8ScOAXQd4XjbRjmSxbw3O6C
+pnogaAcdNqpHnsx/HyEhD2PUJYcE7NlmS885lMcCgYEAqYY9DYTbSGuo+Eqnb6TQ
+RaZ5OEqbxhX4981EMnbW103RM9/MaQjaRrSpc77XdbnWiVkQk7iwczrzJR+HZafb
+pMEsjnl3d92dfRpP+eysAfy0CXxMnxHVsF7uZHKbckuj4COp3UNxMQ1SI6qdid+W
+sGQRkY54OXQfWcG3NRRZg0Y=
+-----END PRIVATE KEY-----

chrome/test/data/extensions/api_test/settings_overrides/homepage/manifest.json

+{
+  "name": "Home Page Overrider",
+  "version": "2.1",
+  "manifest_version": 2,
+  "description": "Overrides your home page",
+  "chrome_settings_overrides": {
+    "homepage": "https://example.com"
+  }
+}

extensions/browser/extension_prefs.cc

@@ -12,6 +12,7 @@
 
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
@@ -1553,20 +1554,35 @@ void ExtensionPrefs::InitPrefStore() {
   TRACE_EVENT0("browser,startup", "ExtensionPrefs::InitPrefStore")
   SCOPED_UMA_HISTOGRAM_TIMER("Extensions.InitPrefStoreTime");
 
-  if (extensions_disabled_) {
-    extension_pref_value_map_->NotifyInitializationCompleted();
-    return;
-  }
-
   // When this is called, the PrefService is initialized and provides access
   // to the user preferences stored in a JSON file.
-  ExtensionIdList extension_ids;
+  std::unique_ptr<ExtensionsInfo> extensions_info;
   {
     SCOPED_UMA_HISTOGRAM_TIMER("Extensions.InitPrefGetExtensionsTime");
-    GetExtensions(&extension_ids);
+    extensions_info = GetInstalledExtensionsInfo();
   }
 
-  InitExtensionControlledPrefs(extension_ids);
+  if (extensions_disabled_) {
+    // Normally, if extensions are disabled, we don't want to load the
+    // controlled prefs from that extension. However, some extensions are
+    // *always* loaded, even with e.g. --disable-extensions. For these, we
+    // need to load the extension-controlled preferences.
+    // See https://crbug.com/828295.
+    auto predicate = [](const auto& info) {
+      // Erase the entry if the extension won't be loaded.
+      return !Manifest::ShouldAlwaysLoadExtension(
+          info->extension_location,
+          Manifest::GetTypeFromManifestValue(*info->extension_manifest));
+    };
+    base::EraseIf(*extensions_info, predicate);
+  }
+
+  // TODO(devlin): |extensions_info| won't contain records for component
+  // extensions (see GetInstalledInfoHelper()). It probably should, because
+  // otherwise component extensions using APIs that rely on extension-controlled
+  // prefs may crash.
+
+  InitExtensionControlledPrefs(*extensions_info);
 
   extension_pref_value_map_->NotifyInitializationCompleted();
 }
@@ -1831,12 +1847,14 @@ void ExtensionPrefs::PopulateExtensionInfoPrefs(
 }
 
 void ExtensionPrefs::InitExtensionControlledPrefs(
-    const ExtensionIdList& extension_ids) {
+    const ExtensionsInfo& extensions_info) {
   TRACE_EVENT0("browser,startup",
                "ExtensionPrefs::InitExtensionControlledPrefs")
   SCOPED_UMA_HISTOGRAM_TIMER("Extensions.InitExtensionControlledPrefsTime");
 
-  for (const ExtensionId& extension_id : extension_ids) {
+  for (const auto& info : extensions_info) {
+    const ExtensionId& extension_id = info->extension_id;
+
     base::Time install_time = GetInstallTime(extension_id);
     bool is_enabled = !IsExtensionDisabled(extension_id);
     bool is_incognito_enabled = IsIncognitoEnabled(extension_id);

extensions/browser/extension_prefs.h

@@ -691,7 +691,7 @@ class ExtensionPrefs : public KeyedService {
       const base::Optional<int>& dnr_ruleset_checksum,
       prefs::DictionaryValueUpdate* extension_dict) const;
 
-  void InitExtensionControlledPrefs(const ExtensionIdList& extension_ids);
+  void InitExtensionControlledPrefs(const ExtensionsInfo& extensions_info);
 
   // Loads preferences for the given |extension_id| into the pref value map.
   void LoadExtensionControlledPrefs(const ExtensionId& extension_id,

extensions/common/manifest.cc

@@ -110,28 +110,53 @@ Manifest::Location Manifest::GetHigherPriorityLocation(
   return (loc1_rank > loc2_rank ? loc1 : loc2 );
 }
 
-Manifest::Manifest(Location location,
-                   std::unique_ptr<base::DictionaryValue> value)
-    : location_(location), value_(std::move(value)), type_(TYPE_UNKNOWN) {
-  if (value_->HasKey(keys::kTheme)) {
-    type_ = TYPE_THEME;
-  } else if (value_->HasKey(keys::kExport)) {
-    type_ = TYPE_SHARED_MODULE;
-  } else if (value_->HasKey(keys::kApp)) {
-    if (value_->Get(keys::kWebURLs, NULL) ||
-        value_->Get(keys::kLaunchWebURL, NULL)) {
-      type_ = TYPE_HOSTED_APP;
-    } else if (value_->Get(keys::kPlatformAppBackground, NULL)) {
-      type_ = TYPE_PLATFORM_APP;
+// static
+Manifest::Type Manifest::GetTypeFromManifestValue(
+    const base::DictionaryValue& value) {
+  Type type = TYPE_UNKNOWN;
+  if (value.HasKey(keys::kTheme)) {
+    type = TYPE_THEME;
+  } else if (value.HasKey(keys::kExport)) {
+    type = TYPE_SHARED_MODULE;
+  } else if (value.HasKey(keys::kApp)) {
+    if (value.Get(keys::kWebURLs, nullptr) ||
+        value.Get(keys::kLaunchWebURL, nullptr)) {
+      type = TYPE_HOSTED_APP;
+    } else if (value.Get(keys::kPlatformAppBackground, nullptr)) {
+      type = TYPE_PLATFORM_APP;
     } else {
-      type_ = TYPE_LEGACY_PACKAGED_APP;
+      type = TYPE_LEGACY_PACKAGED_APP;
     }
   } else {
-    type_ = TYPE_EXTENSION;
+    type = TYPE_EXTENSION;
   }
-  CHECK_NE(type_, TYPE_UNKNOWN);
+  DCHECK_NE(type, TYPE_UNKNOWN);
+
+  return type;
+}
+
+// static
+bool Manifest::ShouldAlwaysLoadExtension(Manifest::Location location,
+                                         Manifest::Type type) {
+  if (location == Manifest::COMPONENT)
+    return true;  // Component extensions are always allowed.
+
+  if (type == Manifest::TYPE_THEME)
+    return true;  // Themes are allowed, even with --disable-extensions.
+
+  // TODO(devlin): This seems wrong. See https://crbug.com/833540.
+  if (Manifest::IsExternalLocation(location))
+    return true;
+
+  return false;
 }
 
+Manifest::Manifest(Location location,
+                   std::unique_ptr<base::DictionaryValue> value)
+    : location_(location),
+      value_(std::move(value)),
+      type_(GetTypeFromManifestValue(*value_)) {}
+
 Manifest::~Manifest() {
 }
 

extensions/common/manifest.h

@@ -123,6 +123,14 @@ class Manifest {
     return IsUnpackedLocation(location);
   }
 
+  // Returns the Manifest::Type for the given |value|.
+  static Type GetTypeFromManifestValue(const base::DictionaryValue& value);
+
+  // Returns true if an item with the given |location| and |type| should
+  // always be loaded, even if extensions are otherwise disabled.
+  static bool ShouldAlwaysLoadExtension(Manifest::Location location,
+                                        Manifest::Type type);
+
   Manifest(Location location, std::unique_ptr<base::DictionaryValue> value);
   virtual ~Manifest();