Fix DictionaryValue leak in component_loader.cc

chrome/browser/extensions/component_loader.cc

Index: chrome/browser/extensions/component_loader.cc
diff --git a/chrome/browser/extensions/component_loader.cc b/chrome/browser/extensions/component_loader.cc
index 44bfce1e9f59a9882c48ad21dfef697c5cff74de..01911428b5213d51b70aae489f0ea97f54815fe5 100644
--- a/chrome/browser/extensions/component_loader.cc
+++ b/chrome/browser/extensions/component_loader.cc
@@ -111,16 +111,33 @@ bool IsNormalSession() {
 }  // namespace
 
 ComponentLoader::ComponentExtensionInfo::ComponentExtensionInfo(
-    const base::DictionaryValue* manifest, const base::FilePath& directory)
-    : manifest(manifest),
-      root_directory(directory) {
+    std::unique_ptr<base::DictionaryValue> manifest_param,

[Devlin, 2016/11/17 16:02:54]

optional nit: I'd prefer to just call this manifest and do `:
manifest(manifest), `, but I understand if that looks weird.

[lazyboy, 2016/11/17 22:50:38]

The issue why I did this is when we use |manifest| in line 121 below, it refers
to the parameter not the struct member. So thought instead of using
this->manifest I'd rename the param.

[Devlin, 2016/11/17 23:34:48]

ah, makes sense, I'm good with this.

+    const base::FilePath& directory)
+    : manifest(std::move(manifest_param)), root_directory(directory) {
   if (!root_directory.IsAbsolute()) {
     CHECK(PathService::Get(chrome::DIR_RESOURCES, &root_directory));
     root_directory = root_directory.Append(directory);
   }
-  extension_id = GenerateId(manifest, root_directory);
+  extension_id = GenerateId(manifest.get(), root_directory);
 }
 
+ComponentLoader::ComponentExtensionInfo::ComponentExtensionInfo(
+    ComponentExtensionInfo&& other)
+    : manifest(std::move(other.manifest)),
+      root_directory(std::move(other.root_directory)),
+      extension_id(std::move(other.extension_id)) {}
+
+ComponentLoader::ComponentExtensionInfo&
+ComponentLoader::ComponentExtensionInfo::operator=(
+    ComponentExtensionInfo&& other) {
+  manifest = std::move(other.manifest);
+  root_directory = std::move(other.root_directory);
+  extension_id = std::move(other.extension_id);
+  return *this;
+}
+
+ComponentLoader::ComponentExtensionInfo::~ComponentExtensionInfo() {}
+
 ComponentLoader::ComponentLoader(ExtensionServiceInterface* extension_service,
                                  PrefService* profile_prefs,
                                  PrefService* local_state,
@@ -147,42 +164,23 @@ void ComponentLoader::LoadAll() {
   }
 }
 
-base::DictionaryValue* ComponentLoader::ParseManifest(
+std::unique_ptr<base::DictionaryValue> ComponentLoader::ParseManifest(
     base::StringPiece manifest_contents) const {
   JSONStringValueDeserializer deserializer(manifest_contents);
   std::unique_ptr<base::Value> manifest = deserializer.Deserialize(NULL, NULL);
 
   if (!manifest.get() || !manifest->IsType(base::Value::TYPE_DICTIONARY)) {
     LOG(ERROR) << "Failed to parse extension manifest.";
-    return NULL;
+    return std::unique_ptr<base::DictionaryValue>();
   }
-  // Transfer ownership to the caller.
-  return static_cast<base::DictionaryValue*>(manifest.release());
+  return base::WrapUnique(
+      static_cast<base::DictionaryValue*>(manifest.release()));

[Devlin, 2016/11/17 16:02:54]

static casting base::Values isn't really preferred - can we use
DictionaryValue::From() here?  (which also avoids the somewhat-awkward
WrapUnique(release()))

[lazyboy, 2016/11/17 22:50:38]

Done.

 }
 
 void ComponentLoader::ClearAllRegistered() {

[Devlin, 2016/11/17 16:02:54]

This is only called from the dtor, and component_extensions_ will be deleted
automatically.  We can just get rid of this method now, right?

[lazyboy, 2016/11/17 22:50:37]

Correct.
Done.

-  for (RegisteredComponentExtensions::iterator it =
-          component_extensions_.begin();
-      it != component_extensions_.end(); ++it) {
-      delete it->manifest;
-  }
-
   component_extensions_.clear();
 }
 
-std::string ComponentLoader::GetExtensionID(
-    int manifest_resource_id,
-    const base::FilePath& root_directory) {
-  base::DictionaryValue* manifest =
-      ParseManifest(ResourceBundle::GetSharedInstance().GetRawDataResource(
-          manifest_resource_id));
-  if (!manifest)
-    return std::string();
-
-  ComponentExtensionInfo info(manifest, root_directory);
-  return info.extension_id;
-}
-
 std::string ComponentLoader::Add(int manifest_resource_id,
                                  const base::FilePath& root_directory) {
   if (!ignore_whitelist_for_testing_ &&
@@ -205,25 +203,28 @@ std::string ComponentLoader::Add(const base::StringPiece& manifest_contents,
                                  bool skip_whitelist) {
   // The Value is kept for the lifetime of the ComponentLoader. This is
   // required in case LoadAll() is called again.
-  base::DictionaryValue* manifest = ParseManifest(manifest_contents);
+  std::unique_ptr<base::DictionaryValue> manifest =
+      ParseManifest(manifest_contents);
   if (manifest)
-    return Add(manifest, root_directory, skip_whitelist);
+    return Add(std::move(manifest), root_directory, skip_whitelist);
   return std::string();
 }
 
-std::string ComponentLoader::Add(const base::DictionaryValue* parsed_manifest,
-                                 const base::FilePath& root_directory,
-                                 bool skip_whitelist) {
-  ComponentExtensionInfo info(parsed_manifest, root_directory);
+std::string ComponentLoader::Add(
+    std::unique_ptr<base::DictionaryValue> parsed_manifest,
+    const base::FilePath& root_directory,
+    bool skip_whitelist) {
+  ComponentExtensionInfo info(std::move(parsed_manifest), root_directory);
   if (!ignore_whitelist_for_testing_ &&
       !skip_whitelist &&
       !IsComponentExtensionWhitelisted(info.extension_id))
     return std::string();
 
-  component_extensions_.push_back(info);
+  component_extensions_.push_back(std::move(info));
+  ComponentExtensionInfo& added_info = component_extensions_.back();
   if (extension_service_->is_ready())
-    Load(info);
-  return info.extension_id;
+    Load(added_info);
+  return added_info.extension_id;
 }
 
 std::string ComponentLoader::AddOrReplace(const base::FilePath& path) {
@@ -240,7 +241,7 @@ std::string ComponentLoader::AddOrReplace(const base::FilePath& path) {
 
   // We don't check component extensions loaded by path because this is only
   // used by developers for testing.
-  return Add(manifest.release(), absolute_path, true);
+  return Add(std::move(manifest), absolute_path, true);
 }
 
 void ComponentLoader::Reload(const std::string& extension_id) {
@@ -271,7 +272,7 @@ void ComponentLoader::Remove(const base::FilePath& root_directory) {
   RegisteredComponentExtensions::iterator it = component_extensions_.begin();

[Devlin, 2016/11/17 16:02:54]

drive-by - inline this in the loop

[lazyboy, 2016/11/17 22:50:38]

Done.

   for (; it != component_extensions_.end(); ++it) {
     if (it->root_directory == root_directory) {
-      Remove(GenerateId(it->manifest, root_directory));
+      Remove(GenerateId(it->manifest.get(), root_directory));
       break;
     }
   }
@@ -400,12 +401,13 @@ void ComponentLoader::AddWithNameAndDescription(
 
   // The Value is kept for the lifetime of the ComponentLoader. This is
   // required in case LoadAll() is called again.
-  base::DictionaryValue* manifest = ParseManifest(manifest_contents);
+  std::unique_ptr<base::DictionaryValue> manifest =
+      ParseManifest(manifest_contents);
 
   if (manifest) {
     manifest->SetString(manifest_keys::kName, name_string);
     manifest->SetString(manifest_keys::kDescription, description_string);
-    Add(manifest, root_directory, true);
+    Add(std::move(manifest), root_directory, true);
   }
 }
 
@@ -638,7 +640,6 @@ void ComponentLoader::
 }
 
 void ComponentLoader::UnloadComponent(ComponentExtensionInfo* component) {
-  delete component->manifest;
   if (extension_service_->is_ready()) {
     extension_service_->
         RemoveComponentExtension(component->extension_id);
@@ -692,10 +693,8 @@ void ComponentLoader::FinishAddComponentFromDir(
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!manifest)
     return;  // Error already logged.
-  std::string actual_extension_id = Add(
-      manifest.release(),
-      root_directory,
-      false);
+  std::string actual_extension_id =
+      Add(std::move(manifest), root_directory, false);
   CHECK_EQ(extension_id, actual_extension_id);
   if (!done_cb.is_null())
     done_cb.Run();