Fixed memory leaks in download manager unit tests.

Fixed memory leaks in download manager unit tests.

BUG=83638
TEST=Valgrind does not detect the leak.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86484

[Lei Zhang, 2011-05-23 21:25:23]

Is it possible to use scoped_ptr to avoid having to manually delete the
DownloadInfo?

[Randy Smith (Not in Mondays), 2011-05-23 22:30:40]

Please clean up the issue description.

http://codereview.chromium.org/6990044/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/6990044/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager_unittest.cc:316: delete info;
If you're in a position where you don't need to pass ownership, can you just
make the DownloadCreateInfo a stack variable and skip this new/delete stuff?
(Applies to entire CL.)

[Paweł Hajdan Jr., 2011-05-24 10:08:05]

Drive-by with a testing comment.

http://codereview.chromium.org/6990044/diff/6001/chrome/browser/download/down...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/6990044/diff/6001/chrome/browser/download/down...
chrome/browser/download/download_manager_unittest.cc:316: delete info;
Could you use a scoped_ptr instead? Manually managing resources is the #1 cause
of leaks in tests.

http://codereview.chromium.org/6990044/diff/6001/chrome/browser/download/down...
chrome/browser/download/download_manager_unittest.cc:365: delete info;
Same here, please use scoped_ptr (in fact, applies to entire CL).

[ahendrickson, 2011-05-24 14:19:02]

Regarding the DownloadCreateInfo pointer:

Normally ownership is passed into the download system, which will delete it.

These tests bypass the code that deletes it, so they have to delete it
themselves.  I could make it a stack variable instead, but I think that would
give people who read the code the mistaken impression that the download system
*doesn't* assume ownership.  The same goes for using a scoped_ptr.

What do you guys think?  Is it worth it to leave it as is, explicitly deleted?

[Randy Smith (Not in Mondays), 2011-05-24 14:32:48]

On 2011/05/24 14:19:02, ahendrickson wrote:
> Regarding the DownloadCreateInfo pointer:
> 
> Normally ownership is passed into the download system, which will delete it.
> 
> These tests bypass the code that deletes it, so they have to delete it
> themselves.  I could make it a stack variable instead, but I think that would
> give people who read the code the mistaken impression that the download system
> *doesn't* assume ownership.  The same goes for using a scoped_ptr.
> 
> What do you guys think?  Is it worth it to leave it as is, explicitly deleted?

I don't think there's any difference in message sent between a scoped_ptr and
deleting it, and a scoped_ptr is safer, so between those two I'd go for
scoped_ptr.  WRT scoped_ptr vs. stack variable, I agree that that gives a bit of
an incorrect impression, but not a strong one--the functions that assume
ownership of the DCI pointer aren't the ones you're calling.  I'd make sure that
the functions that *do* assume ownership are commented in the header file, and
put a comment in the test making clear that though the system normally takes
ownership, these functions don't.  I'd recommend doing that whether or not you
use a stack variable.  

Having said all that, my personal preference is still for a stack variable, but
I'm happy with a scoped_ptr.

[ahendrickson, 2011-05-24 15:34:34]

scoped_ptr it is!

http://codereview.chromium.org/6990044/diff/6001/chrome/browser/download/down...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/6990044/diff/6001/chrome/browser/download/down...
chrome/browser/download/download_manager_unittest.cc:316: delete info;
On 2011/05/24 10:08:05, Paweł Hajdan Jr. wrote:
> Could you use a scoped_ptr instead? Manually managing resources is the #1
cause
> of leaks in tests.

Done.

http://codereview.chromium.org/6990044/diff/6001/chrome/browser/download/down...
chrome/browser/download/download_manager_unittest.cc:365: delete info;
On 2011/05/24 10:08:05, Paweł Hajdan Jr. wrote:
> Same here, please use scoped_ptr (in fact, applies to entire CL).

Done.

[Paweł Hajdan Jr., 2011-05-24 16:35:08]

Code I commented in the drive-by LGTM, thanks!

[ahendrickson, 2011-05-24 17:55:27]

Lei, could you take another look?

[Lei Zhang, 2011-05-24 19:12:11]

LGTM++

[commit-bot: I haz the power, 2011-05-24 20:52:22]

Change committed as 86484