chrome/browser/accessibility/browser_accessibility_manager_unittest.cc - Issue 7639019: Fix BrowserAccessibilityManager valgrind errors.

Side by Side Diff: chrome/browser/accessibility/browser_accessibility_manager_unittest.cc

Issue 7639019: Fix BrowserAccessibilityManager valgrind errors. (Closed) Base URL: http://git.chromium.org/git/chromium.git@trunk

Patch Set: Created 9 years, 4 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch | Annotate | Revision Log

« no previous file with comments | « no previous file | tools/valgrind/memcheck/suppressions.txt » ('j') | no next file with comments »

OLD	NEW
1 // Copyright (c) 2011 The Chromium Authors. All rights reserved.	1 // Copyright (c) 2011 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #include "base/string16.h"	5 #include "base/string16.h"

6 #include "base/utf_string_conversions.h"	6 #include "base/utf_string_conversions.h"

7 #include "content/browser/accessibility/browser_accessibility.h"	7 #include "content/browser/accessibility/browser_accessibility.h"

8 #include "content/browser/accessibility/browser_accessibility_manager.h"	8 #include "content/browser/accessibility/browser_accessibility_manager.h"

9 #include "content/common/view_messages.h"	9 #include "content/common/view_messages.h"

10 #include "testing/gtest/include/gtest/gtest.h"	10 #include "testing/gtest/include/gtest/gtest.h"

(...skipping 607 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
618	618

619 // Verify the root has not changed.	619 // Verify the root has not changed.

620 EXPECT_EQ(root, manager->GetRoot());	620 EXPECT_EQ(root, manager->GetRoot());

621	621

622 // And the proper child remains.	622 // And the proper child remains.

623 EXPECT_EQ(WebAccessibility::ROLE_BUTTON, acc2_2->role());	623 EXPECT_EQ(WebAccessibility::ROLE_BUTTON, acc2_2->role());

624 EXPECT_EQ(1001, acc2_2->renderer_id());	624 EXPECT_EQ(1001, acc2_2->renderer_id());

625	625

626 // Verify we don't reuse objects that have changed roles.	626 // Verify we don't reuse objects that have changed roles.

627 EXPECT_NE(acc1_2, acc2_2);	627 EXPECT_NE(acc1_2, acc2_2);

	628

	629 // Ensure we properly cleaned up.

	630 delete manager;
	Paweł Hajdan Jr. 2011/08/12 17:10:19 How about just making manager a scoped_ptr? Much l 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 I totally agree with you in general, but in this c Show quoted text 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. Show quoted text > > Any ASSERT before this code will cause a leak on failure. Using scoped_ptr would > prevent that. Chris Guillory 2011/08/12 18:44:06 We had this comment in a previous code review. dma Show quoted text 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:08 I think these points are valid and we should use a Show quoted text 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.
	631 ASSERT_EQ(0, CountedBrowserAccessibility::global_obj_count_);

628 }	632 }

OLD	NEW