Fix memory leak in SetDefaultContentSettings.

chrome/browser/content_settings/host_content_settings_map.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/content_settings/host_content_settings_map.h"
 
 #include <utility>
 
 #include "base/basictypes.h"
 #include "base/command_line.h"
@@ skipping 192 matching lines @@
                           false);
   }
 }
 
 void HostContentSettingsMap::SetDefaultContentSetting(
     ContentSettingsType content_type,
     ContentSetting setting) {
   DCHECK_NE(content_type, CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE);
   DCHECK(IsSettingAllowedForType(setting, content_type));
 
-  base::Value* value = Value::CreateIntegerValue(setting);
-  if (!content_settings_providers_[DEFAULT_PROVIDER]->SetWebsiteSetting(
-      ContentSettingsPattern::Wildcard(), ContentSettingsPattern::Wildcard(),
-      content_type, std::string(), value)) {
-    delete value;
-  }
+  base::Value* value = NULL;
+  if (setting != CONTENT_SETTING_DEFAULT)
+    value = Value::CreateIntegerValue(setting);
+  SetWebsiteSetting(
+      ContentSettingsPattern::Wildcard(),
+      ContentSettingsPattern::Wildcard(),
+      content_type,
+      std::string(),
+      value);

[Bernhard Bauer, 2011/11/18 16:32:42]

You should still check the return value for the case when the DefaultProvider
doesn't take ownership.

[Bernhard Bauer, 2011/11/18 16:44:44]

Uh, disregard this, I thought that SetWebsiteSetting returns a bool.

 }
 
 void HostContentSettingsMap::SetWebsiteSetting(
     const ContentSettingsPattern& primary_pattern,
     const ContentSettingsPattern& secondary_pattern,
     ContentSettingsType content_type,
     const std::string& resource_identifier,
     base::Value* value) {
   DCHECK(IsValueAllowedForType(value, content_type));
   DCHECK(content_settings::SupportsResourceIdentifier(content_type) ||
          resource_identifier.empty());
   for (ProviderIterator provider = content_settings_providers_.begin();
        provider != content_settings_providers_.end();
        ++provider) {
     if (provider->second->SetWebsiteSetting(primary_pattern,
                                             secondary_pattern,
                                             content_type,
                                             resource_identifier,
                                             value)) {
       return;
     }
   }
+  // There should be at least one provider that accepts the ownership of the

[James Hawkins, 2011/11/18 16:39:44]

Eh I'm not so sure about this. This falls under the same category of not
handling DCHECK failure conditions.

If this leak does show up on the Valgrind bots, it's a signal that the
NOTREACHED() is wrong. If we leave the delete in, we'll never catch that.

http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

[Bernhard Bauer, 2011/11/18 16:44:44]

Hm, but shouldn't the right way to catch this be a NOTREACHED, and not a
valgrind error? I think there are more debug builds being run without valgrind
than release builds with valgrind.

[James Hawkins, 2011/11/18 16:53:40]

Yes, you're right; however, my original complaint still stands. The argument
being that handling a NOTREACHED() (similar to handling a DCHECK()) dilutes the
assertion and reduces readability.

[markusheintz_, 2011/11/18 17:01:18]

The NOTREACHED is really not reached. I don't know why.
Theoretically it could be reached it a client would try to set a Default setting
in incognito mode. I could also rm the delete and rely on the NOTREACHED. That's
right. I guess I should.

The original problem was in 
http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti...

The scoped_ptr in content_settings_default_provider.cc:154 prevents a leak that
could happen in end of the block starting in
content_settings_default_provider.cc:176 if SetWebsiteSetting is called with a
|value| that equals the hardcoded default value.

Let me quickly dig out the unit_test that leaked.

[markusheintz_, 2011/11/18 21:04:31]

If you like we can change the NOTREACHED() to a CHECK(false).

We do not need the delete value here. As there must be one provider that accepts
the value and takes it's ownership. Anything else is an error.

[James Hawkins, 2011/11/18 21:07:23]

There's no need to change the NOTREACHED(). If it's really not-reachable, the
delete should be removed (along with the comment).

+  // passed |value|. However if we would set a default setting on the incognito
+  // map the |DefaultProvider| would refuse to accept the setting. This should
+  // not happen that's why there is the NOTREACHED. However if it would happen,
+  // then we do not want to leak memory. Hence delete the |value|.
+  delete value;
   NOTREACHED();
 }
 
 void HostContentSettingsMap::SetContentSetting(
     const ContentSettingsPattern& primary_pattern,
     const ContentSettingsPattern& secondary_pattern,
     ContentSettingsType content_type,
     const std::string& resource_identifier,
     ContentSetting setting) {
   DCHECK_NE(content_type, CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE);
@@ skipping 196 matching lines @@
     }
   }
 
   if (info) {
     info->source = content_settings::SETTING_SOURCE_NONE;
     info->primary_pattern = ContentSettingsPattern();
     info->secondary_pattern = ContentSettingsPattern();
   }
   return NULL;
 }