Fix DictionaryValue leak in component_loader.cc

Fix DictionaryValue leak in component_loader.cc

ComponentLoader::GetExtensionID() used to leak |manifest|, but this
function was unused, so removed it.

Component::Add(DictionaryValue*,FilePath&,bool) also leaks DictionaryValue
when it bails out early.

BUG=666153
Committed: https://crrev.com/f4e5cdb1ac9b339725dae67d5f6af9df8c5fe6bb
Cr-Commit-Position: refs/heads/master@{#433036}

[lazyboy, 2016-11-17 01:57:52]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[lazyboy, 2016-11-17 02:04:08]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 02:05:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 04:08:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 04:08:43]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)

[Devlin, 2016-11-17 16:02:55]

lgtm with nits (mostly on the existing code).  Thanks for the cleanup! :)

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:114:
std::unique_ptr<base::DictionaryValue> manifest_param,
optional nit: I'd prefer to just call this manifest and do `:
manifest(manifest), `, but I understand if that looks weird.

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:177:
static_cast<base::DictionaryValue*>(manifest.release()));
static casting base::Values isn't really preferred - can we use
DictionaryValue::From() here?  (which also avoids the somewhat-awkward
WrapUnique(release()))

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:180: void
ComponentLoader::ClearAllRegistered() {
This is only called from the dtor, and component_extensions_ will be deleted
automatically.  We can just get rid of this method now, right?

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:272:
RegisteredComponentExtensions::iterator it = component_extensions_.begin();
drive-by - inline this in the loop

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:282:
RegisteredComponentExtensions::iterator it = component_extensions_.begin();
ditto (and below)

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:286: it =
component_extensions_.erase(it);
Drive-by - No reason to assign |it| here

[lazyboy, 2016-11-17 22:50:27]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

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

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:114:
std::unique_ptr<base::DictionaryValue> manifest_param,
On 2016/11/17 16:02:54, Devlin wrote:
> optional nit: I'd prefer to just call this manifest and do `:
> manifest(manifest), `, but I understand if that looks weird.

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.

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:177:
static_cast<base::DictionaryValue*>(manifest.release()));
On 2016/11/17 16:02:54, Devlin wrote:
> static casting base::Values isn't really preferred - can we use
> DictionaryValue::From() here?  (which also avoids the somewhat-awkward
> WrapUnique(release()))

Done.

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:180: void
ComponentLoader::ClearAllRegistered() {
On 2016/11/17 16:02:54, Devlin wrote:
> This is only called from the dtor, and component_extensions_ will be deleted
> automatically.  We can just get rid of this method now, right?

Correct.
Done.

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:272:
RegisteredComponentExtensions::iterator it = component_extensions_.begin();
On 2016/11/17 16:02:54, Devlin wrote:
> drive-by - inline this in the loop

Done.

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:282:
RegisteredComponentExtensions::iterator it = component_extensions_.begin();
On 2016/11/17 16:02:54, Devlin wrote:
> ditto (and below)

Done.

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:286: it =
component_extensions_.erase(it);
On 2016/11/17 16:02:54, Devlin wrote:
> Drive-by - No reason to assign |it| here

Done.

[commit-bot: I haz the power, 2016-11-17 22:51:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/2504333003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:114:
std::unique_ptr<base::DictionaryValue> manifest_param,
On 2016/11/17 22:50:38, lazyboy wrote:
> On 2016/11/17 16:02:54, Devlin wrote:
> > optional nit: I'd prefer to just call this manifest and do `:
> > manifest(manifest), `, but I understand if that looks weird.
> 
> 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.

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

[commit-bot: I haz the power, 2016-11-18 00:55:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-18 00:55:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[lazyboy, 2016-11-18 02:14:20]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2016-11-18 02:14:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2504333003/#ps20001
(title: "address comments + rewrite all loops")

[commit-bot: I haz the power, 2016-11-18 02:16:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 02:59:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-18 03:03:57]

Description was changed from

==========
Fix DictionaryValue leak in component_loader.cc

ComponentLoader::GetExtensionID() used to leak |manifest|, but this
function was unused, so removed it.

Component::Add(DictionaryValue*,FilePath&,bool) also leaks DictionaryValue
when it bails out early.

BUG=666153
==========

to

==========
Fix DictionaryValue leak in component_loader.cc

ComponentLoader::GetExtensionID() used to leak |manifest|, but this
function was unused, so removed it.

Component::Add(DictionaryValue*,FilePath&,bool) also leaks DictionaryValue
when it bails out early.

BUG=666153

Committed: https://crrev.com/f4e5cdb1ac9b339725dae67d5f6af9df8c5fe6bb
Cr-Commit-Position: refs/heads/master@{#433036}
==========

[commit-bot: I haz the power, 2016-11-18 03:03:58]

Patchset 2 (id:??) landed as
https://crrev.com/f4e5cdb1ac9b339725dae67d5f6af9df8c5fe6bb
Cr-Commit-Position: refs/heads/master@{#433036}