Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8528031:
Fix memory leak in SetDefaultContentSettings. (Closed)
|
Created:
9 years, 1 month ago by markusheintz_ Modified:
9 years, 1 month ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix memory leak in the SetDefaultContentSettings method of the HostContentSettingaMap.
BUG=104775, 102637
TEST=HostContentSettingsMapTest*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110908
Patch Set 1 #Patch Set 2 : Fix memory leaks #
Total comments: 8
Patch Set 3 : Remove unneccessary delete and comment #
Total comments: 2
Patch Set 4 : Remove Valgrind suppressions. #
Messages
Total messages: 16 (0 generated)
Please have a look at this tiny CL that is based on http://codereview.chromium.org/8539004/ THX
Please review this small CL. It fixes another memory leak and changes SetDefaultContentSetting to use SetWebsiteContent setting. This way we have only one code path for setting values.
http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... File chrome/browser/content_settings/host_content_settings_map.cc (right): http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... chrome/browser/content_settings/host_content_settings_map.cc:221: value); You should still check the return value for the case when the DefaultProvider doesn't take ownership.
http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... File chrome/browser/content_settings/host_content_settings_map.cc (right): http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... chrome/browser/content_settings/host_content_settings_map.cc:244: // There should be at least one provider that accepts the ownership of the 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...
http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... File chrome/browser/content_settings/host_content_settings_map.cc (right): http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... chrome/browser/content_settings/host_content_settings_map.cc:221: value); On 2011/11/18 16:32:42, Bernhard Bauer wrote: > You should still check the return value for the case when the DefaultProvider > doesn't take ownership. Uh, disregard this, I thought that SetWebsiteSetting returns a bool. http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... chrome/browser/content_settings/host_content_settings_map.cc:244: // There should be at least one provider that accepts the ownership of the On 2011/11/18 16:39:44, James Hawkins wrote: > 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... 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.
http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... File chrome/browser/content_settings/host_content_settings_map.cc (right): http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... chrome/browser/content_settings/host_content_settings_map.cc:244: // There should be at least one provider that accepts the ownership of the On 2011/11/18 16:44:44, Bernhard Bauer wrote: > On 2011/11/18 16:39:44, James Hawkins wrote: > > 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... > > 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. 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.
http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... File chrome/browser/content_settings/host_content_settings_map.cc (right): http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... chrome/browser/content_settings/host_content_settings_map.cc:244: // There should be at least one provider that accepts the ownership of the On 2011/11/18 16:39:44, James Hawkins wrote: > 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... 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.
Ok it is actually as simple as this: The other fix from yesterday (http://codereview.chromium.org/8592001/) didn't fix the leak. Valgrind on my local machine as well as on the bots confirm this. This CL fixes it. ExtensionSpecialStoragePolicyTest* passes valgrind without any error on my local linux machine. I run the try bots again incl. the linux_valgrind bot again after removing the "delete value". If they are green again. We are good to go. http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... File chrome/browser/content_settings/host_content_settings_map.cc (right): http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... chrome/browser/content_settings/host_content_settings_map.cc:244: // There should be at least one provider that accepts the ownership of the On 2011/11/18 17:01:18, markusheintz_ wrote: > On 2011/11/18 16:39:44, James Hawkins wrote: > > 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... > > 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. 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.
http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... File chrome/browser/content_settings/host_content_settings_map.cc (right): http://codereview.chromium.org/8528031/diff/3001/chrome/browser/content_setti... chrome/browser/content_settings/host_content_settings_map.cc:244: // There should be at least one provider that accepts the ownership of the On 2011/11/18 21:04:31, markusheintz_ wrote: > On 2011/11/18 17:01:18, markusheintz_ wrote: > > On 2011/11/18 16:39:44, James Hawkins wrote: > > > 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... > > > > 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. > > 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. There's no need to change the NOTREACHED(). If it's really not-reachable, the delete should be removed (along with the comment).
The "delete value "is already removed in the latest pachset.
On 2011/11/18 21:09:30, markusheintz_ wrote: > The "delete value "is already removed in the latest pachset. Interesting Gmail tells my last comment on this issue was considered to be spam. Next try: The comment and "delete value" is already removed in the latest patchset.
http://codereview.chromium.org/8528031/diff/12001/chrome/browser/content_sett... File chrome/browser/content_settings/host_content_settings_map.cc (right): http://codereview.chromium.org/8528031/diff/12001/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map.cc:216: SetWebsiteSetting( I don't understand why you got rid of the if check and delete. If this method returns false, the client must delete |value| (or so the API documentation says).
http://codereview.chromium.org/8528031/diff/12001/chrome/browser/content_sett... File chrome/browser/content_settings/host_content_settings_map.cc (right): http://codereview.chromium.org/8528031/diff/12001/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map.cc:216: SetWebsiteSetting( On 2011/11/18 21:21:13, James Hawkins wrote: > I don't understand why you got rid of the if check and delete. If this method > returns false, the client must delete |value| (or so the API documentation > says). Sorry for the confusion, but I'm not calling the SetWebsiteSetting method of the provider anymore. I call the SetWebsiteSetting method of the HostContentSettingsMap. (see below) This method has not return value.
Gotcha; LGTM. Please remove the suppression that was added (in this CL).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/markusheintz@chromium.org/8528031/14001
Change committed as 110908 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
