Fix memory leak in SetDefaultContentSettings.

chrome/browser/content_settings/host_content_settings_map.cc

Index: chrome/browser/content_settings/host_content_settings_map.cc
diff --git a/chrome/browser/content_settings/host_content_settings_map.cc b/chrome/browser/content_settings/host_content_settings_map.cc
index 39c38a939349651c70d02e0a1759414851fc117a..ae2dfc2721e08d8767aaba5a4a8d25fab5780132 100644
--- a/chrome/browser/content_settings/host_content_settings_map.cc
+++ b/chrome/browser/content_settings/host_content_settings_map.cc
@@ -210,12 +210,15 @@ void HostContentSettingsMap::SetDefaultContentSetting(
   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(
@@ -238,6 +241,12 @@ void HostContentSettingsMap::SetWebsiteSetting(
       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();
 }