Fix BrowserAccessibilityManager valgrind errors.

Fix BrowserAccessibilityManager valgrind errors.

BUG=91845, 92396, 59243
TEST=*_valgrind try bots green.


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

[David Tseng, 2011-08-12 15:04:30]

Please rubber stamp. I'll check in only if *_valgrind bots pass (currently
running). I tested on a mac_valgrind bot before uploading the patch and all
seemed well.

[Paweł Hajdan Jr., 2011-08-12 17:10:19]

http://codereview.chromium.org/7639019/diff/1/chrome/browser/accessibility/br...
File chrome/browser/accessibility/browser_accessibility_manager_unittest.cc
(right):

http://codereview.chromium.org/7639019/diff/1/chrome/browser/accessibility/br...
chrome/browser/accessibility/browser_accessibility_manager_unittest.cc:630:
delete manager;
How about just making manager a scoped_ptr? Much less error-prone, and trying to
manually manage resources is #1 source of leaks in unit tests. Of course feel
free to keep fixing issue after issue.

Any ASSERT before this code will cause a leak on failure. Using scoped_ptr would
prevent that.

[David Tseng, 2011-08-12 17:30:30]

http://codereview.chromium.org/7639019/diff/1/chrome/browser/accessibility/br...
File chrome/browser/accessibility/browser_accessibility_manager_unittest.cc
(right):

http://codereview.chromium.org/7639019/diff/1/chrome/browser/accessibility/br...
chrome/browser/accessibility/browser_accessibility_manager_unittest.cc:630:
delete manager;
On 2011/08/12 17:10:19, Paweł Hajdan Jr. wrote:
> How about just making manager a scoped_ptr? Much less error-prone, and trying
to
> manually manage resources is #1 source of leaks in unit tests. Of course feel
> free to keep fixing issue after issue.

I totally agree with you in general, but in this case, it's important for us to
check the assert below because BrowserAccessibilityManager performs it's own ref
counting. If there's an error in our ref counting, a scoped_ptr would not help.
It had been my mistake to not put this here in the first place.
> 
> Any ASSERT before this code will cause a leak on failure. Using scoped_ptr
would
> prevent that.

[Chris Guillory, 2011-08-12 18:44:05]

browser_accessibility_manager_unittest.cc LGTM.

http://codereview.chromium.org/7639019/diff/1/chrome/browser/accessibility/br...
File chrome/browser/accessibility/browser_accessibility_manager_unittest.cc
(right):

http://codereview.chromium.org/7639019/diff/1/chrome/browser/accessibility/br...
chrome/browser/accessibility/browser_accessibility_manager_unittest.cc:630:
delete manager;
On 2011/08/12 17:10:19, Paweł Hajdan Jr. wrote:
> How about just making manager a scoped_ptr? Much less error-prone, and trying
to
> manually manage resources is #1 source of leaks in unit tests. Of course feel
> free to keep fixing issue after issue.
> 
> Any ASSERT before this code will cause a leak on failure. Using scoped_ptr
would
> prevent that.

We had this comment in a previous code review. dmazzoni wanted to keep the
delete explicit since the test is actually checking for leaks. I suppose using a
scoped_ptr reset method could accomplish the same thing.

[Chris Guillory, 2011-08-12 18:46:07]

http://codereview.chromium.org/7639019/diff/1/chrome/browser/accessibility/br...
File chrome/browser/accessibility/browser_accessibility_manager_unittest.cc
(right):

http://codereview.chromium.org/7639019/diff/1/chrome/browser/accessibility/br...
chrome/browser/accessibility/browser_accessibility_manager_unittest.cc:630:
delete manager;
On 2011/08/12 17:10:19, Paweł Hajdan Jr. wrote:
> How about just making manager a scoped_ptr? Much less error-prone, and trying
to
> manually manage resources is #1 source of leaks in unit tests. Of course feel
> free to keep fixing issue after issue.
> 
> Any ASSERT before this code will cause a leak on failure. Using scoped_ptr
would
> prevent that.

I think these points are valid and we should use a scoped_pointer.

[David Tseng, 2011-08-12 19:20:16]

On 8/12/11, ctguil@chromium.org <ctguil@chromium.org> wrote:
>
>
http://codereview.chromium.org/7639019/diff/1/chrome/browser/accessibility/br...
> File
> chrome/browser/accessibility/browser_accessibility_manager_unittest.cc
> (right):
>
>
http://codereview.chromium.org/7639019/diff/1/chrome/browser/accessibility/br...
> chrome/browser/accessibility/browser_accessibility_manager_unittest.cc:630:
> delete manager;
> On 2011/08/12 17:10:19, Paweł Hajdan Jr. wrote:
>> How about just making manager a scoped_ptr? Much less error-prone, and
> trying to
>> manually manage resources is #1 source of leaks in unit tests. Of
> course feel
>> free to keep fixing issue after issue.
>
>> Any ASSERT before this code will cause a leak on failure. Using
> scoped_ptr would
>> prevent that.
>
> I think these points are valid and we should use a scoped_pointer.
>
In this test, or all of them?

I'm only weary of doing so because BrowserAccessibilityManager ref
counts BrowserAccessibility's, so having this assert fail rather than
a valgrind error later down the line seems preferable.

A comprimise could be to restructure these tests so they have
setup/teardown methods that perform this deletion.

[dmazzoni, 2011-08-12 20:05:50]

I'm fine with using scoped_ptr and calling scoped_ptr.reset() at the
end to assert that the ref count goes to zero, that would accomplish
the same thing.

- Dominic

[David Tseng, 2011-08-12 20:53:20]

Done.

On 8/12/11, Dominic Mazzoni <dmazzoni@chromium.org> wrote:
> I'm fine with using scoped_ptr and calling scoped_ptr.reset() at the
> end to assert that the ref count goes to zero, that would accomplish
> the same thing.
>
> - Dominic
>

[commit-bot: I haz the power, 2011-08-14 21:43:39]

Change committed as 96741