Fix 2 bugs related to remembering loaded unpacked extensions.

- Extension disabled infobar was being shown at startup.
- Crashed extensions were persisted as unpacked extensions, regardless of how
they were installed.

BUG=30116
BUG=38856

Review URL: http://codereview.chromium.org/1157005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@42394 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 129 insertions, 91 deletions

diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index 990c6a2..4e37d4c 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -2577,13 +2577,15 @@ void Browser::Observe(NotificationType type,
       break;
 
     case NotificationType::EXTENSION_UPDATE_DISABLED: {
-      // Show the UI.
+      // Show the UI if the extension was disabled for escalated permissions.
       Profile* profile = Source<Profile>(source).ptr();
       DCHECK_EQ(profile_, profile);
       ExtensionsService* service = profile->GetExtensionsService();
       DCHECK(service);
       Extension* extension = Details<Extension>(details).ptr();
-      ShowExtensionDisabledUI(service, profile_, extension);
+      if (service->extension_prefs()->DidExtensionEscalatePermissions(
+              extension->id()))
+        ShowExtensionDisabledUI(service, profile_, extension);
       break;
     }
 
diff --git a/chrome/browser/extensions/extension_disabled_infobar_delegate.cc b/chrome/browser/extensions/extension_disabled_infobar_delegate.cc
index 20acb53..ad82ef35 100644
--- a/chrome/browser/extensions/extension_disabled_infobar_delegate.cc
+++ b/chrome/browser/extensions/extension_disabled_infobar_delegate.cc
@@ -39,7 +39,7 @@ class ExtensionDisabledDialogDelegate
   // ExtensionInstallUI::Delegate
   virtual void InstallUIProceed(bool create_app_shortcut) {
     ExtensionPrefs* prefs = service_->extension_prefs();
-    prefs->SetShowInstallWarningOnEnable(extension_, false);
+    prefs->SetDidExtensionEscalatePermissions(extension_, false);
     service_->EnableExtension(extension_->id());
     Release();
   }
diff --git a/chrome/browser/extensions/extension_prefs.cc b/chrome/browser/extensions/extension_prefs.cc
index c9f391f..3040903 100644
--- a/chrome/browser/extensions/extension_prefs.cc
+++ b/chrome/browser/extensions/extension_prefs.cc
@@ -36,7 +36,7 @@ const wchar_t kPrefVersion[] = L"manifest.version";
 const wchar_t kPrefBlacklist[] = L"blacklist";
 
 // Indicates whether to show an install warning when the user enables.
-const wchar_t kShowInstallWarning[] = L"install_warning_on_enable";
+const wchar_t kExtensionDidEscalatePermissions[] = L"install_warning_on_enable";
 
 // A preference that tracks extension shelf configuration.  This is a list
 // object read from the Preferences file, containing a list of toolstrip URLs.
@@ -188,7 +188,15 @@ bool ExtensionPrefs::IsExtensionBlacklisted(const std::string& extension_id) {
 
 bool ExtensionPrefs::DidExtensionEscalatePermissions(
     const std::string& extension_id) {
-  return ReadExtensionPrefBoolean(extension_id, kShowInstallWarning);
+  return ReadExtensionPrefBoolean(extension_id,
+                                  kExtensionDidEscalatePermissions);
+}
+
+void ExtensionPrefs::SetDidExtensionEscalatePermissions(
+    Extension* extension, bool did_escalate) {
+  UpdateExtensionPref(extension->id(), kExtensionDidEscalatePermissions,
+                      Value::CreateBooleanValue(did_escalate));
+  prefs_->SavePersistentPrefs();
 }
 
 void ExtensionPrefs::UpdateBlacklist(
@@ -418,13 +426,6 @@ void ExtensionPrefs::SetExtensionState(Extension* extension,
   prefs_->SavePersistentPrefs();
 }
 
-void ExtensionPrefs::SetShowInstallWarningOnEnable(
-    Extension* extension, bool require) {
-  UpdateExtensionPref(extension->id(), kShowInstallWarning,
-                      Value::CreateBooleanValue(require));
-  prefs_->SavePersistentPrefs();
-}
-
 std::string ExtensionPrefs::GetVersionString(const std::string& extension_id) {
   DictionaryValue* extension = GetExtensionPref(extension_id);
   if (!extension)
@@ -499,81 +500,103 @@ DictionaryValue* ExtensionPrefs::GetExtensionPref(
   return extension;
 }
 
-// static
-ExtensionPrefs::ExtensionsInfo* ExtensionPrefs::CollectExtensionsInfo(
-    ExtensionPrefs* prefs) {
-  scoped_ptr<DictionaryValue> extension_data(
-      prefs->CopyCurrentExtensions());
+// Helper function for GetInstalledExtensionsInfo.
+static ExtensionInfo* GetInstalledExtensionInfoImpl(
+    DictionaryValue* extension_data,
+    DictionaryValue::key_iterator extension_id) {
+  DictionaryValue* ext;
+  if (!extension_data->GetDictionaryWithoutPathExpansion(*extension_id, &ext)) {
+    LOG(WARNING) << "Invalid pref for extension " << *extension_id;
+    NOTREACHED();
+    return NULL;
+  }
+  if (ext->HasKey(kPrefBlacklist)) {
+    bool is_blacklisted = false;
+    if (!ext->GetBoolean(kPrefBlacklist, &is_blacklisted)) {
+      NOTREACHED() << "Invalid blacklist pref:" << *extension_id;
+      return NULL;
+    }
+    if (is_blacklisted) {
+      LOG(WARNING) << "Blacklisted extension: " << *extension_id;
+      return NULL;
+    }
+  }
+  int state_value;
+  if (!ext->GetInteger(kPrefState, &state_value)) {
+    LOG(WARNING) << "Missing state pref for extension " << *extension_id;
+    NOTREACHED();
+    return NULL;
+  }
+  if (state_value == Extension::KILLBIT) {
+    LOG(WARNING) << "External extension has been uninstalled by the user "
+                 << *extension_id;
+    return NULL;
+  }
+  FilePath::StringType path;
+  if (!ext->GetString(kPrefPath, &path)) {
+    LOG(WARNING) << "Missing path pref for extension " << *extension_id;
+    NOTREACHED();
+    return NULL;
+  }
+  int location_value;
+  if (!ext->GetInteger(kPrefLocation, &location_value)) {
+    LOG(WARNING) << "Missing location pref for extension " << *extension_id;
+    NOTREACHED();
+    return NULL;
+  }
+
+  // Only the following extension types can be installed permanently in the
+  // preferences.
+  Extension::Location location =
+      static_cast<Extension::Location>(location_value);
+  if (location != Extension::INTERNAL &&
+      location != Extension::LOAD &&
+      !Extension::IsExternalLocation(location)) {
+    NOTREACHED();
+    return NULL;
+  }
+
+  DictionaryValue* manifest = NULL;
+  if (!ext->GetDictionary(kPrefManifest, &manifest)) {
+    LOG(WARNING) << "Missing manifest for extension " << *extension_id;
+    // Just a warning for now.
+  }
+
+  return new ExtensionInfo(manifest, WideToASCII(*extension_id),
+                           FilePath(path), location);
+}
+
+ExtensionPrefs::ExtensionsInfo* ExtensionPrefs::GetInstalledExtensionsInfo() {
+  scoped_ptr<DictionaryValue> extension_data(CopyCurrentExtensions());
 
   ExtensionsInfo* extensions_info = new ExtensionsInfo;
 
   for (DictionaryValue::key_iterator extension_id(
            extension_data->begin_keys());
        extension_id != extension_data->end_keys(); ++extension_id) {
-    DictionaryValue* ext;
-    if (!extension_data->GetDictionaryWithoutPathExpansion(*extension_id,
-                                                            &ext)) {
-      LOG(WARNING) << "Invalid pref for extension " << *extension_id;
-      NOTREACHED();
-      continue;
-    }
-    if (ext->HasKey(kPrefBlacklist)) {
-      bool is_blacklisted = false;
-      if (!ext->GetBoolean(kPrefBlacklist, &is_blacklisted)) {
-        NOTREACHED() << "Invalid blacklist pref:" << *extension_id;
-        continue;
-      }
-      if (is_blacklisted) {
-        LOG(WARNING) << "Blacklisted extension: " << *extension_id;
-        continue;
-      }
-    }
-    int state_value;
-    if (!ext->GetInteger(kPrefState, &state_value)) {
-      LOG(WARNING) << "Missing state pref for extension " << *extension_id;
-      NOTREACHED();
-      continue;
-    }
-    if (state_value == Extension::KILLBIT) {
-      LOG(WARNING) << "External extension has been uninstalled by the user "
-                   << *extension_id;
-      continue;
-    }
-    FilePath::StringType path;
-    if (!ext->GetString(kPrefPath, &path)) {
-      LOG(WARNING) << "Missing path pref for extension " << *extension_id;
-      NOTREACHED();
-      continue;
-    }
-    int location_value;
-    if (!ext->GetInteger(kPrefLocation, &location_value)) {
-      LOG(WARNING) << "Missing location pref for extension " << *extension_id;
-      NOTREACHED();
-      continue;
-    }
+    ExtensionInfo* info = GetInstalledExtensionInfoImpl(extension_data.get(),
+                                                        extension_id);
+    if (info)
+      extensions_info->push_back(linked_ptr<ExtensionInfo>(info));
+  }
 
-    // Only the following extension types can be installed permanently in the
-    // preferences.
-    Extension::Location location =
-        static_cast<Extension::Location>(location_value);
-    if (location != Extension::INTERNAL &&
-        location != Extension::LOAD &&
-        !Extension::IsExternalLocation(location)) {
-      NOTREACHED();
-      continue;
-    }
+  return extensions_info;
+}
 
-    DictionaryValue* manifest = NULL;
-    if (!ext->GetDictionary(kPrefManifest, &manifest)) {
-      LOG(WARNING) << "Missing manifest for extension " << *extension_id;
-      // Just a warning for now.
-    }
+ExtensionInfo* ExtensionPrefs::GetInstalledExtensionInfo(
+    const std::string& extension_id) {
+  scoped_ptr<DictionaryValue> extension_data(CopyCurrentExtensions());
 
-    extensions_info->push_back(linked_ptr<ExtensionInfo>(new ExtensionInfo(
-        manifest, WideToASCII(*extension_id), FilePath(path), location)));
+  for (DictionaryValue::key_iterator extension_iter(
+           extension_data->begin_keys());
+       extension_iter != extension_data->end_keys(); ++extension_iter) {
+    if (WideToASCII(*extension_iter) == extension_id) {
+      return GetInstalledExtensionInfoImpl(extension_data.get(),
+                                           extension_iter);
+    }
   }
 
-  return extensions_info;
+  return NULL;
 }
 
 // static
diff --git a/chrome/browser/extensions/extension_prefs.h b/chrome/browser/extensions/extension_prefs.h
index 4b026c2..66faf77 100644
--- a/chrome/browser/extensions/extension_prefs.h
+++ b/chrome/browser/extensions/extension_prefs.h
@@ -59,9 +59,13 @@ class ExtensionPrefs {
   // Called to change the extension's state when it is enabled/disabled.
   void SetExtensionState(Extension* extension, Extension::State);
 
-  // If |require| is true, the preferences for |extension| will be set to
+  // Did the extension ask to escalate its permission during an upgrade?
+  bool DidExtensionEscalatePermissions(const std::string& id);
+
+  // If |did_escalate| is true, the preferences for |extension| will be set to
   // require the install warning when the user tries to enable.
-  void SetShowInstallWarningOnEnable(Extension* extension, bool require);
+  void SetDidExtensionEscalatePermissions(Extension* extension,
+                                          bool did_escalate);
 
   // Returns the version string for the currently installed extension, or
   // the empty string if not found.
@@ -83,9 +87,6 @@ class ExtensionPrefs {
   // Based on extension id, checks prefs to see if it is blacklisted.
   bool IsExtensionBlacklisted(const std::string& id);
 
-  // Did the extension ask to escalate its permission during an upgrade?
-  bool DidExtensionEscalatePermissions(const std::string& id);
-
   // Returns the last value set via SetLastPingDay. If there isn't such a
   // pref, the returned Time will return true for is_null().
   base::Time LastPingDay(const std::string& extension_id);
@@ -103,7 +104,11 @@ class ExtensionPrefs {
   // 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.
-  static ExtensionsInfo* CollectExtensionsInfo(ExtensionPrefs* extension_prefs);
+  ExtensionsInfo* GetInstalledExtensionsInfo();
+
+  // Returns the ExtensionInfo from the prefs for the given extension. If the
+  // extension is not present, NULL is returned.
+  ExtensionInfo* GetInstalledExtensionInfo(const std::string& extension_id);
 
   static void RegisterUserPrefs(PrefService* prefs);
 
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index db233aa..4c3e039 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -59,7 +59,7 @@ class InstalledExtensionSet {
  public:
   explicit InstalledExtensionSet(ExtensionPrefs* prefs) {
     scoped_ptr<ExtensionPrefs::ExtensionsInfo> info(
-        ExtensionPrefs::CollectExtensionsInfo(prefs));
+        prefs->GetInstalledExtensionsInfo());
 
     for (size_t i = 0; i < info->size(); ++i) {
       std::string version;
@@ -247,11 +247,19 @@ void ExtensionsService::ReloadExtension(const std::string& extension_id) {
     path = unloaded_extension_paths_[extension_id];
   }
 
-  // We should always be able to remember the extension's path. If it's not in
-  // the map, someone failed to update |unloaded_extension_paths_|.
-  CHECK(!path.empty());
-
-  LoadExtension(path);
+  // Check the installed extensions to see if what we're reloading was already
+  // installed.
+  scoped_ptr<ExtensionInfo> installed_extension(
+      extension_prefs_->GetInstalledExtensionInfo(extension_id));
+  if (installed_extension.get() &&
+      installed_extension->extension_manifest.get()) {
+    LoadInstalledExtension(*installed_extension, false);
+  } else {
+    // We should always be able to remember the extension's path. If it's not in
+    // the map, someone failed to update |unloaded_extension_paths_|.
+    CHECK(!path.empty());
+    LoadExtension(path);
+  }
 }
 
 void ExtensionsService::UninstallExtension(const std::string& extension_id,
@@ -368,7 +376,7 @@ void ExtensionsService::LoadAllExtensions() {
 
   // Load the previously installed extensions.
   scoped_ptr<ExtensionPrefs::ExtensionsInfo> info(
-      ExtensionPrefs::CollectExtensionsInfo(extension_prefs_.get()));
+      extension_prefs_->GetInstalledExtensionsInfo());
 
   // If any extensions need localization, we bounce them all to the file thread
   // for re-reading and localization.
@@ -738,7 +746,7 @@ void ExtensionsService::OnExtensionLoaded(Extension* extension,
           // Extension has changed permissions significantly. Disable it. We
           // send a notification below.
           extension_prefs_->SetExtensionState(extension, Extension::DISABLED);
-          extension_prefs_->SetShowInstallWarningOnEnable(extension, true);
+          extension_prefs_->SetDidExtensionEscalatePermissions(extension, true);
         }
       } else {
         // We already have the extension of the same or older version.
diff --git a/chrome/browser/extensions/extensions_service.h b/chrome/browser/extensions/extensions_service.h
index aef46b7..86737f0 100644
--- a/chrome/browser/extensions/extensions_service.h
+++ b/chrome/browser/extensions/extensions_service.h
@@ -222,7 +222,7 @@ class ExtensionsService
   // Called when the initial extensions load has completed.
   virtual void OnLoadedInstalledExtensions();
 
-  // Called by the backend when an extension has been loaded.
+  // Called when an extension has been loaded.
   void OnExtensionLoaded(Extension* extension,
                          bool allow_privilege_increase);