Fix DictionaryValue leak in component_loader.cc

chrome/browser/extensions/component_loader.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/extensions/component_loader.h"
 
 #include <string>
 
 #include "base/command_line.h"
 #include "base/files/file_util.h"
@@ skipping 93 matching lines @@
   return !base::CommandLine::ForCurrentProcess()->HasSwitch(
              chromeos::switches::kGuestSession) &&
          user_manager::UserManager::IsInitialized() &&
          user_manager::UserManager::Get()->IsUserLoggedIn();
 }
 #endif  // defined(OS_CHROMEOS)
 
 }  // 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,
                                  Profile* profile)
     : profile_prefs_(profile_prefs),
       local_state_(local_state),
       profile_(profile),
       extension_service_(extension_service),
       ignore_whitelist_for_testing_(false),
       weak_factory_(this) {}
 
 ComponentLoader::~ComponentLoader() {
   ClearAllRegistered();
 }
 
 void ComponentLoader::LoadAll() {
   TRACE_EVENT0("browser,startup", "ComponentLoader::LoadAll");
   SCOPED_UMA_HISTOGRAM_TIMER("Extensions.LoadAllComponentTime");
 
   for (RegisteredComponentExtensions::iterator it =
           component_extensions_.begin();
       it != component_extensions_.end(); ++it) {
     Load(*it);
   }
 }
 
-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_ &&
       !IsComponentExtensionWhitelisted(manifest_resource_id))
     return std::string();
 
   base::StringPiece manifest_contents =
       ResourceBundle::GetSharedInstance().GetRawDataResource(
           manifest_resource_id);
   return Add(manifest_contents, root_directory, true);
 }
 
 std::string ComponentLoader::Add(const base::StringPiece& manifest_contents,
                                  const base::FilePath& root_directory) {
   return Add(manifest_contents, root_directory, false);
 }
 
 std::string ComponentLoader::Add(const base::StringPiece& manifest_contents,
                                  const base::FilePath& root_directory,
                                  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) {
   base::FilePath absolute_path = base::MakeAbsoluteFilePath(path);
   std::string error;
   std::unique_ptr<base::DictionaryValue> manifest(
       file_util::LoadManifest(absolute_path, &error));
   if (!manifest) {
     LOG(ERROR) << "Could not load extension from '" <<
                   absolute_path.value() << "'. " << error;
     return std::string();
   }
   Remove(GenerateId(manifest.get(), absolute_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) {
   for (RegisteredComponentExtensions::iterator it =
          component_extensions_.begin(); it != component_extensions_.end();
          ++it) {
     if (it->extension_id == extension_id) {
       Load(*it);
       break;
     }
   }
 }
 
 void ComponentLoader::Load(const ComponentExtensionInfo& info) {
   std::string error;
   scoped_refptr<const Extension> extension(CreateExtension(info, &error));
   if (!extension.get()) {
     LOG(ERROR) << error;
     return;
   }
 
   CHECK_EQ(info.extension_id, extension->id()) << extension->name();
   extension_service_->AddComponentExtension(extension.get());
 }
 
 void ComponentLoader::Remove(const base::FilePath& root_directory) {
   // Find the ComponentExtensionInfo for the extension.
   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;
     }
   }
 }
 
 void ComponentLoader::Remove(const std::string& id) {
   RegisteredComponentExtensions::iterator it = component_extensions_.begin();

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

ditto (and below)

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

Done.

   for (; it != component_extensions_.end(); ++it) {
     if (it->extension_id == id) {
       UnloadComponent(&(*it));
       it = component_extensions_.erase(it);

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

Drive-by - No reason to assign |it| here

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

Done.

       break;
     }
   }
 }
 
 bool ComponentLoader::Exists(const std::string& id) const {
   RegisteredComponentExtensions::const_iterator it =
       component_extensions_.begin();
   for (; it != component_extensions_.end(); ++it)
     if (it->extension_id == id)
@@ skipping 97 matching lines @@
   if (!ignore_whitelist_for_testing_ &&
       !IsComponentExtensionWhitelisted(manifest_resource_id))
     return;
 
   base::StringPiece manifest_contents =
       ResourceBundle::GetSharedInstance().GetRawDataResource(
           manifest_resource_id);
 
   // 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);
   }
 }
 
 void ComponentLoader::AddChromeApp() {
 #if BUILDFLAG(ENABLE_APP_LIST)
   AddWithNameAndDescription(
       IDR_CHROME_APP_MANIFEST, base::FilePath(FILE_PATH_LITERAL("chrome_app")),
       l10n_util::GetStringUTF8(IDS_SHORT_PRODUCT_NAME),
       l10n_util::GetStringUTF8(IDS_CHROME_SHORTCUT_DESCRIPTION));
 #endif
@@ skipping 212 matching lines @@
       (command_line->HasSwitch(switches::kTestType) ||
        command_line->HasSwitch(
            switches::kDisableComponentExtensionsWithBackgroundPages))) {
     return;
   }
 
   AddHangoutServicesExtension();
 }
 
 void ComponentLoader::UnloadComponent(ComponentExtensionInfo* component) {
-  delete component->manifest;
   if (extension_service_->is_ready()) {
     extension_service_->
         RemoveComponentExtension(component->extension_id);
   }
 }
 
 void ComponentLoader::EnableFileSystemInGuestMode(const std::string& id) {
 #if defined(OS_CHROMEOS)
   if (!IsNormalSession()) {
     // TODO(dpolukhin): Hack to enable HTML5 temporary file system for
@@ skipping 33 matching lines @@
 }
 
 void ComponentLoader::FinishAddComponentFromDir(
     const base::FilePath& root_directory,
     const char* extension_id,
     const base::Closure& done_cb,
     std::unique_ptr<base::DictionaryValue> manifest) {
   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();
 }
 #endif
 
 }  // namespace extensions