Fix memory leak in SyncableExtensionSettingsStorage.

chrome/browser/extensions/syncable_extension_settings_storage.cc

 // Copyright (c) 2011 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/syncable_extension_settings_storage.h"
 
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/extensions/extension_settings_sync_util.h"
 #include "chrome/browser/sync/api/sync_data.h"
 #include "chrome/browser/sync/protocol/extension_setting_specifics.pb.h"
@@ skipping 159 matching lines @@
     const DictionaryValue& sync_state, const DictionaryValue& settings) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   // Treat this as a list of changes to sync and use ProcessSyncChanges.
   // This gives notifications etc for free.
   scoped_ptr<DictionaryValue> new_sync_state(sync_state.DeepCopy());
 
   ExtensionSettingSyncDataList changes;
   for (DictionaryValue::key_iterator it = settings.begin_keys();
       it != settings.end_keys(); ++it) {
     Value* sync_value = NULL;
     if (new_sync_state->RemoveWithoutPathExpansion(*it, &sync_value)) {

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

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());
  }
}

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

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.

       Value* local_value = NULL;
       settings.GetWithoutPathExpansion(*it, &local_value);
       if (!local_value->Equals(sync_value)) {
         // Sync value is different, update local setting with new value.
         changes.push_back(
             ExtensionSettingSyncData(
                 SyncChange::ACTION_UPDATE, extension_id_, *it, sync_value));
+      } else {
+        // Values are the same, no change needed.
+        delete sync_value;
       }
     } else {
       // Not synced, delete local setting.
       changes.push_back(
           ExtensionSettingSyncData(
               SyncChange::ACTION_DELETE,
               extension_id_,
               *it,
               new DictionaryValue()));
     }
   }
 
   // Add all new settings to local settings.
   while (!new_sync_state->empty()) {
     std::string key = *new_sync_state->begin_keys();
     Value* value;

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

since you're modifying this file:

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

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

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

     new_sync_state->RemoveWithoutPathExpansion(key, &value);
     changes.push_back(
         ExtensionSettingSyncData(
             SyncChange::ACTION_ADD, extension_id_, key, value));
   }
 
   if (changes.empty()) {
     return SyncError();
   }
 
@@ skipping 118 matching lines @@
   if (error.IsSet()) {
     LOG(WARNING) << "Failed to send changes to sync: " << error.message();
     return;
   }
 
   for (std::vector<std::string>::const_iterator it = keys.begin();
       it != keys.end(); ++it) {
     synced_keys_.erase(*it);
   }
 }