New issue
Advanced search Search tips

Issue 757244 link

Starred by 1 user
Status: Fixed
Owner:
lazyboy@chromium.org
Closed: Aug 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

DictionaryValue leak in

Project Member Reported by lazyboy@chromium.org, Aug 20 2017 
ExternalProviderImpl::UpdatePrefs implicitly takes ownership of its DictionaryValue* param, for which there's one clear leak:

https://cs.chromium.org/chromium/src/chrome/browser/extensions/external_provider_impl.cc?rcl=8b9b1c33fe14cbe901f6547d11c516c820268889&l=149

void ExternalProviderImpl::UpdatePrefs(base::DictionaryValue* prefs) {
  ..
  if (!service_)
    return;  // LEAK
   prefs_.reset(prefs); // OWNERSHIP TRANSFER
  ..
}

Make relevant params use std::unique_ptr.
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 20 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7431b371c87b1ee42ae4abfb8ef8999dc27f0ef

commit a7431b371c87b1ee42ae4abfb8ef8999dc27f0ef
Author: Istiaque Ahmed <lazyboy@chromium.org>
Date: Sun Aug 20 18:33:37 2017

Change DictionaryValue* param in ExternalProviderImpl to unique_ptr.

Change ExternalProviderImpl::Set/UpdatePrefs' params from
DictionaryValue* to unique_ptr.
ExternalProviderImpl::UpdatePrefs had a leak because of this, because
there's an early out in that function.

Bug:  757244 
Test: Internal change only.
Change-Id: I25c57f341adc0a01fc0579066f38ef2a6b494cb0
Reviewed-on: https://chromium-review.googlesource.com/621233
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495844}
[modify] https://crrev.com/a7431b371c87b1ee42ae4abfb8ef8999dc27f0ef/chrome/browser/extensions/default_apps.cc
[modify] https://crrev.com/a7431b371c87b1ee42ae4abfb8ef8999dc27f0ef/chrome/browser/extensions/default_apps.h
[modify] https://crrev.com/a7431b371c87b1ee42ae4abfb8ef8999dc27f0ef/chrome/browser/extensions/extension_service_unittest.cc
[modify] https://crrev.com/a7431b371c87b1ee42ae4abfb8ef8999dc27f0ef/chrome/browser/extensions/external_loader.cc
[modify] https://crrev.com/a7431b371c87b1ee42ae4abfb8ef8999dc27f0ef/chrome/browser/extensions/external_provider_impl.cc
[modify] https://crrev.com/a7431b371c87b1ee42ae4abfb8ef8999dc27f0ef/chrome/browser/extensions/external_provider_impl.h

Comment 2 by lazyboy@chromium.org, Aug 23 2017

Status: Fixed (was: Started)
►

Sign in to add a comment