Fix memory leak in SyncableExtensionSettingsStorage.

Fix memory leak in SyncableExtensionSettingsStorage.

BUG=97511
TEST=

[not at google - send to devlin, 2011-09-22 01:30:49]

Which trybot should I put it through?  I just did "git try" but realise that I
don't know whether they run memory checks.

[not at google - send to devlin, 2011-09-22 01:31:03]

P.S. this is the last of the memory leaks.

[akalin, 2011-09-22 01:36:52]

http://codereview.chromium.org/7978051/diff/1/chrome/browser/extensions/synca...
File chrome/browser/extensions/syncable_extension_settings_storage.cc (right):

http://codereview.chromium.org/7978051/diff/1/chrome/browser/extensions/synca...
chrome/browser/extensions/syncable_extension_settings_storage.cc:180: if
(new_sync_state->RemoveWithoutPathExpansion(*it, &sync_value)) {
i'd feel better if a scoped_ptr was involved somehow.  Maybe:

Value* sync_value ...;
scoped_ptr<Value> scoped_sync_value ...;
if (...RemoveWithoutPathExpansion(..., &sync_value)) {
  scoped_sync_value.reset(sync_value);
  ...
    changes.push_back(..., sync_value.release());
  }
}

http://codereview.chromium.org/7978051/diff/1/chrome/browser/extensions/synca...
chrome/browser/extensions/syncable_extension_settings_storage.cc:206: Value*
value;
since you're modifying this file:

Value* value = NULL;
CHECK(new_sync_state->RemoveWithoutPathExpansion(key, &value));
...

[akalin, 2011-09-22 01:37:15]

On 2011/09/22 01:30:49, kalman1 wrote:
> Which trybot should I put it through?  I just did "git try" but realise that I
> don't know whether they run memory checks.

Since you're modifying suppressions files, git try will send it to the valgrind
bots automagically. :)

[akalin, 2011-09-22 01:37:48]

On 2011/09/22 01:37:15, akalin wrote:
> On 2011/09/22 01:30:49, kalman1 wrote:
> > Which trybot should I put it through?  I just did "git try" but realise that
I
> > don't know whether they run memory checks.
> 
> Since you're modifying suppressions files, git try will send it to the
valgrind
> bots automagically. :)

also try 'git try -r <revision you're synced to>' to avoid update errors

[not at google - send to devlin, 2011-09-22 02:31:13]

git try'ing (against the correct version)

http://codereview.chromium.org/7978051/diff/1/chrome/browser/extensions/synca...
File chrome/browser/extensions/syncable_extension_settings_storage.cc (right):

http://codereview.chromium.org/7978051/diff/1/chrome/browser/extensions/synca...
chrome/browser/extensions/syncable_extension_settings_storage.cc:180: if
(new_sync_state->RemoveWithoutPathExpansion(*it, &sync_value)) {
On 2011/09/22 01:36:52, akalin wrote:
> i'd feel better if a scoped_ptr was involved somehow.  Maybe:
> 
> Value* sync_value ...;
> scoped_ptr<Value> scoped_sync_value ...;
> if (...RemoveWithoutPathExpansion(..., &sync_value)) {
>   scoped_sync_value.reset(sync_value);
>   ...
>     changes.push_back(..., sync_value.release());
>   }
> }

Yeah there are several options, it's just so damn ugly... I was hoping to avoid
having two pointers to the same thing, but oh well, it's not so bad, and better
than any of the alternatives.

http://codereview.chromium.org/7978051/diff/1/chrome/browser/extensions/synca...
chrome/browser/extensions/syncable_extension_settings_storage.cc:206: Value*
value;
On 2011/09/22 01:36:52, akalin wrote:
> since you're modifying this file:
> 
> Value* value = NULL;
> CHECK(new_sync_state->RemoveWithoutPathExpansion(key, &value));
> ...

Ok.  Though I think it's fine to assume that RemoveWithoutPathExpansion given a
key that you know comes from the dictionary... works.

[oshima, 2011-09-22 03:16:27]

kalman, you don't need LGTM from me as akalin is best person to review your
change. Just cc to me so that I can check this in case the CL didn't fix all
leaks.

[oshima, 2011-09-22 03:16:56]

On 2011/09/22 03:16:27, oshima wrote:
> kalman, you don't need LGTM from me as akalin is best person to review your
> change. Just cc to me so that I can check this in case the CL didn't fix all
> leaks.

Doh, ignore LGT* above.

[akalin, 2011-09-22 03:18:12]

On 2011/09/22 03:16:56, oshima wrote:
> On 2011/09/22 03:16:27, oshima wrote:
> > kalman, you don't need LGTM from me as akalin is best person to review your
> > change. Just cc to me so that I can check this in case the CL didn't fix all
> > leaks.
> 
> Doh, ignore LGT* above.

LGTM

I can try/land this, too

[kalman_google, 2011-09-22 03:20:42]

I ran the trybots myself but turns out -r is a svn revision not a git
revision.  Makes sense.


On Thu, Sep 22, 2011 at 1:18 PM, <akalin@chromium.org> wrote:

> On 2011/09/22 03:16:56, oshima wrote:
>
>> On 2011/09/22 03:16:27, oshima wrote:
>> > kalman, you don't need LGTM from me as akalin is best person to review
>> your
>> > change. Just cc to me so that I can check this in case the CL didn't fix
>> all
>> > leaks.
>>
>
>  Doh, ignore LGT* above.
>>
>
> LGTM
>
> I can try/land this, too
>
>
>
http://codereview.chromium.**org/7978051/<http://codereview.chromium.org/7978...
>

[akalin, 2011-09-22 05:51:17]

Committed as 102246