Include component extensions when initializing extension-controlled prefs.

As of https://crrev.com/170137, component extensions/apps are installed
when their version changes, performing all the usual extension-install
work, including registering the extension with ExtensionPrefValueMap.
When a component extension is loaded without a version change, much of
this is skipped, leading to
ExtensionPrefValueMap::CanExtensionControlPref() hitting a NOTREACHED().

Fix this by including component extensions when initializing
extension-controlled prefs, which includes registering the extension
with ExtensionPrefValueMap.

Bug: 454513
Change-Id: Ic478e5ac5367e3bb22cebd6e9422e9ca5bd48849
Reviewed-on: https://chromium-review.googlesource.com/897223Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Commit-Queue: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577380}

chrome/browser/chromeos/extensions/accessibility_features_apitest.cc

@@ -194,6 +194,36 @@ IN_PROC_BROWSER_TEST_P(AccessibilityFeaturesApiTest, Get) {
       << message_;
 }
 
+IN_PROC_BROWSER_TEST_P(AccessibilityFeaturesApiTest, PRE_Get_ComponentApp) {
+  EXPECT_FALSE(RunPlatformAppTestWithFlags(GetTestExtensionPath(), "{}",
+                                           kFlagLoadAsComponent))
+      << message_;
+}
+
+// A regression test for https://crbug.com/454513. Ensure that loading a
+// component extension with the same version as has previously loaded, correctly
+// sets up access to accessibility prefs. Otherwise,this is the same as the
+// |Get| test.
+IN_PROC_BROWSER_TEST_P(AccessibilityFeaturesApiTest, Get_ComponentApp) {
+  // WARNING: Make sure that spoken feedback is not among enabled_features
+  // (see |AccessibilityFeaturesApiTest.Set| test for the reason).
+  std::vector<std::string> enabled_features = {"largeCursor", "stickyKeys",
+                                               "highContrast"};
+
+  std::vector<std::string> disabled_features = {
+      "spokenFeedback", "screenMagnifier", "autoclick", "virtualKeyboard"};
+
+  ASSERT_TRUE(
+      InitPrefServiceForTest(GetPrefs(), enabled_features, disabled_features));
+
+  std::string test_arg;
+  ASSERT_TRUE(GenerateTestArg("getterTest", enabled_features, disabled_features,
+                              &test_arg));
+  EXPECT_TRUE(RunPlatformAppTestWithFlags(
+      GetTestExtensionPath(), test_arg.c_str(), kFlagLoadAsComponent))
+      << message_;
+}
+
 // Tests that an extension with modify permission can modify accessibility
 // features, while an extension that doesn't have the permission can't.
 IN_PROC_BROWSER_TEST_P(AccessibilityFeaturesApiTest, Set) {

chrome/browser/extensions/extension_prefs_unittest.cc

@@ -993,6 +993,13 @@ class ExtensionPrefsComponentExtension : public ExtensionPrefsTest {
     EXPECT_FALSE(prefs()->ReadPrefAsURLPatternSet(no_component_extension_->id(),
                                                   pref_key, &scriptable_hosts,
                                                   valid_schemes));
+
+    // Both extensions should be registered with the ExtensionPrefValueMap.
+    // See https://crbug.com/454513.
+    EXPECT_TRUE(prefs_.extension_pref_value_map()->CanExtensionControlPref(
+        component_extension_->id(), "a_pref", false));
+    EXPECT_TRUE(prefs_.extension_pref_value_map()->CanExtensionControlPref(
+        no_component_extension_->id(), "a_pref", false));
   }
 
  private:

chrome/test/data/extensions/api_test/accessibility_features/modify_permission/manifest.json

 {
+  // chrome-extension://lbgjohhgghbkcgejgklgcmfijhbheflf/
+  "key": "MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDasz2sLsAlmcF0v7/FGwzWVP/T+CLhvWpojKckVp8RH0bN/x3HvQ8FUweTymsaLbqxMHn8LbMOYt9uvLg7MuUcs0puzo7vPEwW7FPwLdIke2Fth+uXgkBFUFvtrOoAyIXmiRRFoIi9qfNVQOvIz0nv0c7UEKoHT3UnT0ekxSl7lwIDAQAB",
   "name": "A test extension for accessibilityFeatures API; no modify",
   "version": "0.0.1",
   "manifest_version": 2,

chrome/test/data/extensions/api_test/accessibility_features/read_permission/manifest.json

 {
+  // chrome-extension://lbgjohhgghbkcgejgklgcmfijhbheflf/
+  "key": "MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDasz2sLsAlmcF0v7/FGwzWVP/T+CLhvWpojKckVp8RH0bN/x3HvQ8FUweTymsaLbqxMHn8LbMOYt9uvLg7MuUcs0puzo7vPEwW7FPwLdIke2Fth+uXgkBFUFvtrOoAyIXmiRRFoIi9qfNVQOvIz0nv0c7UEKoHT3UnT0ekxSl7lwIDAQAB",
   "name": "A test extension for accessibilityFeatures API; no modify",
   "version": "0.0.1",
   "manifest_version": 2,

extensions/browser/extension_prefs.cc

@@ -1212,23 +1212,24 @@ void ExtensionPrefs::UpdateManifest(const Extension* extension) {
 
 std::unique_ptr<ExtensionInfo> ExtensionPrefs::GetInstalledInfoHelper(
     const std::string& extension_id,
-    const base::DictionaryValue* extension) const {
+    const base::DictionaryValue* extension,
+    bool include_component_extensions) const {
   int location_value;
   if (!extension->GetInteger(kPrefLocation, &location_value))
     return std::unique_ptr<ExtensionInfo>();
 
   Manifest::Location location = static_cast<Manifest::Location>(location_value);
-  if (location == Manifest::COMPONENT) {
-    // Component extensions are ignored. Component extensions may have data
-    // saved in preferences, but they are already loaded at this point (by
-    // ComponentLoader) and shouldn't be populated into the result of
+  if (location == Manifest::COMPONENT && !include_component_extensions) {
+    // Component extensions are ignored by default. Component extensions may
+    // have data saved in preferences, but they are already loaded at this point
+    // (by ComponentLoader) and shouldn't be populated into the result of
     // GetInstalledExtensionsInfo, otherwise InstalledLoader would also want to
     // load them.
     return std::unique_ptr<ExtensionInfo>();
   }
 
   // Only the following extension types have data saved in the preferences.
-  if (location != Manifest::INTERNAL &&
+  if (location != Manifest::INTERNAL && location != Manifest::COMPONENT &&
       !Manifest::IsUnpackedLocation(location) &&
       !Manifest::IsExternalLocation(location)) {
     NOTREACHED();
@@ -1255,7 +1256,8 @@ std::unique_ptr<ExtensionInfo> ExtensionPrefs::GetInstalledInfoHelper(
 }
 
 std::unique_ptr<ExtensionInfo> ExtensionPrefs::GetInstalledExtensionInfo(
-    const std::string& extension_id) const {
+    const std::string& extension_id,
+    bool include_component_extensions) const {
   const base::DictionaryValue* ext = NULL;
   const base::DictionaryValue* extensions =
       prefs_->GetDictionary(pref_names::kExtensions);
@@ -1268,11 +1270,13 @@ std::unique_ptr<ExtensionInfo> ExtensionPrefs::GetInstalledExtensionInfo(
     return std::unique_ptr<ExtensionInfo>();
   }
 
-  return GetInstalledInfoHelper(extension_id, ext);
+  return GetInstalledInfoHelper(extension_id, ext,
+                                include_component_extensions);
 }
 
 std::unique_ptr<ExtensionPrefs::ExtensionsInfo>
-ExtensionPrefs::GetInstalledExtensionsInfo() const {
+ExtensionPrefs::GetInstalledExtensionsInfo(
+    bool include_component_extensions) const {
   std::unique_ptr<ExtensionsInfo> extensions_info(new ExtensionsInfo);
 
   const base::DictionaryValue* extensions =
@@ -1282,8 +1286,8 @@ ExtensionPrefs::GetInstalledExtensionsInfo() const {
     if (!crx_file::id_util::IdIsValid(extension_id.key()))
       continue;
 
-    std::unique_ptr<ExtensionInfo> info =
-        GetInstalledExtensionInfo(extension_id.key());
+    std::unique_ptr<ExtensionInfo> info = GetInstalledExtensionInfo(
+        extension_id.key(), include_component_extensions);
     if (info)
       extensions_info->push_back(std::move(info));
   }
@@ -1306,7 +1310,8 @@ ExtensionPrefs::GetUninstalledExtensionsInfo() const {
       continue;
 
     std::unique_ptr<ExtensionInfo> info =
-        GetInstalledInfoHelper(extension_id.key(), ext);
+        GetInstalledInfoHelper(extension_id.key(), ext,
+                               /*include_component_extensions = */ false);
     if (info)
       extensions_info->push_back(std::move(info));
   }
@@ -1401,7 +1406,8 @@ std::unique_ptr<ExtensionInfo> ExtensionPrefs::GetDelayedInstallInfo(
   if (!extension_prefs->GetDictionary(kDelayedInstallInfo, &ext))
     return std::unique_ptr<ExtensionInfo>();
 
-  return GetInstalledInfoHelper(extension_id, ext);
+  return GetInstalledInfoHelper(extension_id, ext,
+                                /*include_component_extensions = */ false);
 }
 
 ExtensionPrefs::DelayReason ExtensionPrefs::GetDelayedInstallReason(
@@ -1609,7 +1615,8 @@ void ExtensionPrefs::InitPrefStore() {
   std::unique_ptr<ExtensionsInfo> extensions_info;
   {
     SCOPED_UMA_HISTOGRAM_TIMER("Extensions.InitPrefGetExtensionsTime");
-    extensions_info = GetInstalledExtensionsInfo();
+    extensions_info =
+        GetInstalledExtensionsInfo(/*include_component_extensions = */ true);
   }
 
   if (extensions_disabled_) {
@@ -1643,11 +1650,6 @@ void ExtensionPrefs::InitPrefStore() {
     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();

extensions/browser/extension_prefs.h

@@ -432,7 +432,8 @@ class ExtensionPrefs : public KeyedService {
   // version directory and the location. Blacklisted extensions won't be saved
   // and neither will external extensions the user has explicitly uninstalled.
   // Caller takes ownership of returned structure.
-  std::unique_ptr<ExtensionsInfo> GetInstalledExtensionsInfo() const;
+  std::unique_ptr<ExtensionsInfo> GetInstalledExtensionsInfo(
+      bool include_component_extensions = false) const;
 
   // Same as above, but only includes external extensions the user has
   // explicitly uninstalled.
@@ -441,7 +442,8 @@ class ExtensionPrefs : public KeyedService {
   // Returns the ExtensionInfo from the prefs for the given extension. If the
   // extension is not present, NULL is returned.
   std::unique_ptr<ExtensionInfo> GetInstalledExtensionInfo(
-      const std::string& extension_id) const;
+      const std::string& extension_id,
+      bool include_component_extensions = false) const;
 
   // We've downloaded an updated .crx file for the extension, but are waiting
   // to install it.
@@ -620,7 +622,8 @@ class ExtensionPrefs : public KeyedService {
   // |extension| dictionary.
   std::unique_ptr<ExtensionInfo> GetInstalledInfoHelper(
       const std::string& extension_id,
-      const base::DictionaryValue* extension) const;
+      const base::DictionaryValue* extension,
+      bool include_component_extensions) const;
 
   // Interprets the list pref, |pref_key| in |extension_id|'s preferences, as a
   // URLPatternSet. The |valid_schemes| specify how to parse the URLPatterns.