ash_unittests leaking NonClientFrameController |
||||||||||
Issue descriptionThis happens as of https://chromium-review.googlesource.com/c/chromium/src/+/1123654 first happened in https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/28323 [2923/2924] AppListPresenterDelegateHomeLauncherTest.VisibilityDuringWallpaperPreview (610 ms) [ RUN ] AcceleratorTest.PostAcceleratorWorks [ OK ] AcceleratorTest.PostAcceleratorWorks (320 ms) [----------] 1 test from AcceleratorTest (320 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (320 ms total) [ PASSED ] 1 test. ================================================================= ==6008==ERROR: LeakSanitizer: detected memory leaks Direct leak of 88 byte(s) in 1 object(s) allocated from: #0 0x6b10e2 in operator new(unsigned long) /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cc:106:3 #1 0x6db8464 in CreateAndParentTopLevelWindowInRoot ash/wm/top_level_window_factory.cc:155:9 #2 0x6db8464 in ash::CreateAndParentTopLevelWindow(ash::WindowManager*, ui::mojom::WindowType, aura::PropertyConverter*, std::__1::map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, std::__1::less<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > > > >*) ash/wm/top_level_window_factory.cc:214 #3 0x6f596c1 in ash::WindowServiceDelegateImpl::NewTopLevel(aura::PropertyConverter*, base::flat_map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, std::__1::less<void> > const&) ash/ws/window_service_delegate_impl.cc:85:7 #4 0xbc045fd in ui::ws2::WindowTree::NewTopLevelWindow(unsigned int, unsigned long, base::flat_map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, std::__1::less<void> > const&) services/ui/ws2/window_tree.cc:1242:36 #5 0x827a19f in NewTopLevelWindow services/ui/ws2/window_tree_test_helper.cc:49:17 #6 0x827a19f in ui::ws2::WindowTreeTestHelper::NewTopLevelWindow(base::flat_map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, std::__1::less<void> > const&) services/ui/ws2/window_tree_test_helper.cc:56 #7 0xc004088 in ash::AshTestBase::CreateTestWindow(gfx::Rect const&, aura::client::WindowType, int) ash/test/ash_test_base.cc:303:34 #8 0x731af4 in ash::accelerators::AcceleratorTest_PostAcceleratorWorks_Test::TestBody() ash/accelerators/accelerator_unittest.cc:31:42 #9 0x4849d32 in testing::Test::Run() third_party/googletest/src/googletest/src/gtest.cc #10 0x484bc94 in testing::TestInfo::Run() third_party/googletest/src/googletest/src/gtest.cc:2667:11 #11 0x484d066 in testing::TestCase::Run() third_party/googletest/src/googletest/src/gtest.cc:2785:28 #12 0x4872766 in testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/src/googletest/src/gtest.cc:5047:43 #13 0x48719b5 in testing::UnitTest::Run() third_party/googletest/src/googletest/src/gtest.cc #14 0x723a2aa in RUN_ALL_TESTS third_party/googletest/src/googletest/include/gtest/gtest.h:2329:46 #15 0x723a2aa in base::TestSuite::Run() base/test/test_suite.cc:277 #16 0x723f815 in Run base/callback.h:99:12 #17 0x723f815 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, bool, base::OnceCallback<void ()>) base/test/launcher/unit_test_launcher.cc:225 #18 0x723f2d0 in base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>) base/test/launcher/unit_test_launcher.cc:576:10 #19 0x18359c8 in main ash/test/ash_unittests.cc:37:10 #20 0x7f799859af44 in __libc_start_main /build/eglibc-ripdx6/eglibc-2.19/csu/libc-start.c:287 SUMMARY: AddressSanitizer: 88 byte(s) leaked in 1 allocation(s). [2924/2924] AcceleratorTest.PostAcceleratorWorks (320 ms) Please take a look.
,
Jul 20
Oh hmm, the bot has caught up, and it's still failing in ash_unittests... https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/28335 ash_unittests Run on OS: 'Ubuntu-14.04' Shard duration: 0:12:01.105210 failures: MruWindowTrackerTest.DraggedWindowsInListOnlyOnce TopLevelWindowFactoryTest.CreateFullscreenWindow SplitViewWindowSelectorTest.NoNudgingWhenNumRowsChange ShelfViewFocusTest.OverflowNotActivatedWhenOpened WorkspaceLayoutManagerTest.FullscreenInDisplayToBeRestored SplitViewWindowSelectorTest.DraggingUnsnappableAppWithSplitView WorkspaceLayoutManagerTest.RestoreFromMinimizeKeepsRestore MruWindowTrackerTest.MinimizedWindowsAreLru TrayEventFilterTest.ClickingOnKeyboardContainerDoesNotCloseBubble WorkspaceLayoutManagerKeyboardTest.ChildWindowFocused AcceleratorTest.PostAcceleratorWorks WorkspaceLayoutManagerBackdropTest.BackdropTest WorkspaceLayoutManagerBackdropTest.BasicBackdropTests SplitViewWindowSelectorTest.NoNudgingWhenLastItemOnPreviousRowDrops WorkspaceLayoutManagerTest.AdjustSnappedBoundsWidth WallpaperControllerTest.AddFirstWallpaperAnimationEndCallback DragDropImageTest.SetBoundsConsidersDragHintForTouch WorkspaceLayoutManagerBackdropTest.ShelfVisibilityDoesNotChangesBounds MusPropertyMirrorAshTest.OwnedProperties SplitViewWindowSelectorTest.BasicNudging WindowServiceDelegateImplTest.RunDragLoop WorkspaceLayoutManagerSoloTest.UnminimizeWithActivation ShelfLayoutManagerTest.SwipingUpOnShelfInTabletModeForFullscreenAppList WorkspaceLayoutManagerBackdropTest.VerifyBackdropAndItsStacking OverviewButtonTrayTest.SplitviewModeQuickSwitch DragDropImageTest.SetBoundsIgnoresDragHintForMouse SplitViewWindowSelectorTest.PreviewAreaVisibility WorkspaceLayoutManagerTest.WindowShouldBeOnScreenWhenAdded WorkspaceLayoutManagerBackdropTest.BackdropUpdatesOnFullscreenAppListVisibilityNotification WallpaperControllerTest.CancelPreviewWallpaper ... 43 more ... [ RUN ] MruWindowTrackerTest.DraggedWindowsInListOnlyOnce [ OK ] MruWindowTrackerTest.DraggedWindowsInListOnlyOnce (576 ms) [----------] 1 test from MruWindowTrackerTest (576 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (576 ms total) [ PASSED ] 1 test. ================================================================= ==28179==ERROR: LeakSanitizer: detected memory leaks Direct leak of 80 byte(s) in 1 object(s) allocated from: #0 0x6b1812 in operator new(unsigned long) /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cc:106:3 #1 0x6a70a17 in CreateAndParentTopLevelWindowInRoot ash/wm/top_level_window_factory.cc:152:9 #2 0x6a70a17 in ash::CreateAndParentTopLevelWindow(ui::mojom::WindowType, aura::PropertyConverter*, std::__1::map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, std::__1::less<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > > > >*) ash/wm/top_level_window_factory.cc:210 #3 0x6c0046f in ash::WindowServiceDelegateImpl::NewTopLevel(aura::PropertyConverter*, base::flat_map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, std::__1::less<void> > const&) ash/ws/window_service_delegate_impl.cc:86:18 #4 0xb8aa94d in ui::ws2::WindowTree::NewTopLevelWindow(unsigned int, unsigned long, base::flat_map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, std::__1::less<void> > const&) services/ui/ws2/window_tree.cc:1242:36 #5 0x7f232ef in NewTopLevelWindow services/ui/ws2/window_tree_test_helper.cc:49:17 #6 0x7f232ef in ui::ws2::WindowTreeTestHelper::NewTopLevelWindow(base::flat_map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >, std::__1::less<void> > const&) services/ui/ws2/window_tree_test_helper.cc:56 #7 0xbbcb268 in ash::AshTestBase::CreateTestWindow(gfx::Rect const&, aura::client::WindowType, int) ash/test/ash_test_base.cc:289:34 #8 0x1941070 in CreateTestWindow ash/wm/mru_window_tracker_unittest.cc:22:25 #9 0x1941070 in ash::MruWindowTrackerTest_DraggedWindowsInListOnlyOnce_Test::TestBody() ash/wm/mru_window_tracker_unittest.cc:85 #10 0x452bc42 in testing::Test::Run() third_party/googletest/src/googletest/src/gtest.cc #11 0x452dba4 in testing::TestInfo::Run() third_party/googletest/src/googletest/src/gtest.cc:2667:11 #12 0x452ef76 in testing::TestCase::Run() third_party/googletest/src/googletest/src/gtest.cc:2785:28 #13 0x4554676 in testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/src/googletest/src/gtest.cc:5047:43 #14 0x45538c5 in testing::UnitTest::Run() third_party/googletest/src/googletest/src/gtest.cc #15 0x6ee07ca in RUN_ALL_TESTS third_party/googletest/src/googletest/include/gtest/gtest.h:2329:46 #16 0x6ee07ca in base::TestSuite::Run() base/test/test_suite.cc:277 #17 0x6ee5d6f in Run base/callback.h:99:12 #18 0x6ee5d6f in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, bool, base::OnceCallback<void ()>) base/test/launcher/unit_test_launcher.cc:225 #19 0x6ee57f0 in base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>) base/test/launcher/unit_test_launcher.cc:576:10 #20 0x1755579 in main ash/test/ash_unittests.cc:37:10 #21 0x7f8308aaff44 in __libc_start_main /build/eglibc-ripdx6/eglibc-2.19/csu/libc-start.c:287 SUMMARY: AddressSanitizer: 80 byte(s) leaked in 1 allocation(s).
,
Jul 20
That error appears to come from CreateAndParentTopLevelWindowInRoot's use of NonClientFrameController
,
Jul 20
Looking at the relevant changes from James' CL, I wonder if we're actually leaking the ContentsViewMus? https://chromium-review.googlesource.com/c/chromium/src/+/1123654/9/ash/wm/non_client_frame_controller.cc#461 Perhaps it's not added to the view hierarchy before the widget is destroyed? If that's it, we could: 1) Add the ContentsViewMus to the ClientView right away in the ClientViewMus ctor. 2) Lazily create the ContentsViewMus in NonClientFrameController::GetContentsView, as James had in an earlier patch set. Or perhaps we're leaking something else?
,
Jul 21
Gah, I haven't been able to repro locally, following instructions at: https://www.chromium.org/developers/testing/leaksanitizer msw@msw-linux /work/chrome-git/src (master) $ out/lsan/ash_unittests --gtest_filter=MruWindowTrackerTest.DraggedWindowsInListOnlyOnce IMPORTANT DEBUGGING NOTE: batches of tests are run inside their own process. For debugging a test inside a debugger, use the --gtest_filter=<your_test_name> flag along with --single-process-tests. Using sharding settings from environment. This is shard 0/1 Using 1 parallel jobs. Note: Google Test filter = MruWindowTrackerTest.DraggedWindowsInListOnlyOnce [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from MruWindowTrackerTest [ RUN ] MruWindowTrackerTest.DraggedWindowsInListOnlyOnce [ OK ] MruWindowTrackerTest.DraggedWindowsInListOnlyOnce (208 ms) [----------] 1 test from MruWindowTrackerTest (208 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (208 ms total) [ PASSED ] 1 test. [1/1] MruWindowTrackerTest.DraggedWindowsInListOnlyOnce (208 ms) SUCCESS: all tests passed. Tests took 0 seconds.
,
Jul 21
nvm, I got it locally, I'll try to find a quick fix
,
Jul 21
It's indeed the NonClientFrameController itself being leaked, I have a local fix at: https://chromium-review.googlesource.com/c/chromium/src/+/1145948 NonClientFrameController is the WidgetDelegate for a Widget that's owned by the NativeWidget. The window and widget are being deleted, but NonClientFrameController::DeleteDelegate is not implemented. This is a subtle regression caused by changing the base class from WidgetDelegateView to WidgetDelegate: https://chromium-review.googlesource.com/c/chromium/src/+/1123654/9/ash/wm/non_client_frame_controller.h#44
,
Jul 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/49705f2d06911c6817e7f49ad446995861529ca9 commit 49705f2d06911c6817e7f49ad446995861529ca9 Author: Mike Wasserman <msw@chromium.org> Date: Sat Jul 21 01:59:24 2018 Fix ash_unittests NonClientFrameController lsan leak Implement NonClientFrameController::DeleteDelegate to delete itself. Regressed via the base class change WidgetDelegateView -> WidgetDelegate: http://crrev.com/c/1123654/9/ash/wm/non_client_frame_controller.h#44 See the relevant WidgetDelegateView::DeleteDelegate impl here: https://cs.chromium.org/chromium/src/ui/views/widget/widget_delegate.cc?l=200 TBR=jamescook@chromium.org Bug: 866180 Test: No ash_unittests or other leaks detected by lsan, etc. bots. Change-Id: Id8e827653d1e31a2d2abbeef9ea5e7c90e375810 Reviewed-on: https://chromium-review.googlesource.com/1145948 Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#577058} [modify] https://crrev.com/49705f2d06911c6817e7f49ad446995861529ca9/ash/wm/non_client_frame_controller.cc [modify] https://crrev.com/49705f2d06911c6817e7f49ad446995861529ca9/ash/wm/non_client_frame_controller.h
,
Jul 22
Fixed on the bot
,
Jul 23
Thanks for fixing this!
,
Jul 23
TPMs, requesting backport for this small fix for a small memory leak to the M70 branch. (It affects production code, not just tests.)
,
Jul 24
,
Jul 26
Er, merge request to M69, since that's the affected branch.
,
Jul 27
Your change meets the bar and is auto-approved for M69. Please go ahead and merge the CL to branch 3497 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/85479851289867a0f4b6ee0eb1913942104a2f3b commit 85479851289867a0f4b6ee0eb1913942104a2f3b Author: Mike Wasserman <msw@chromium.org> Date: Fri Jul 27 21:17:27 2018 M69 merge: Fix ash_unittests NonClientFrameController lsan leak Implement NonClientFrameController::DeleteDelegate to delete itself. Regressed via the base class change WidgetDelegateView -> WidgetDelegate: http://crrev.com/c/1123654/9/ash/wm/non_client_frame_controller.h#44 See the relevant WidgetDelegateView::DeleteDelegate impl here: https://cs.chromium.org/chromium/src/ui/views/widget/widget_delegate.cc?l=200 TBR=jamescook@chromium.org Bug: 866180 Test: No ash_unittests or other leaks detected by lsan, etc. bots. Change-Id: Id8e827653d1e31a2d2abbeef9ea5e7c90e375810 Reviewed-on: https://chromium-review.googlesource.com/1145948 Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Michael Wasserman <msw@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#577058}(cherry picked from commit 49705f2d06911c6817e7f49ad446995861529ca9) Reviewed-on: https://chromium-review.googlesource.com/1153659 Reviewed-by: James Cook <jamescook@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#175} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/85479851289867a0f4b6ee0eb1913942104a2f3b/ash/wm/non_client_frame_controller.cc [modify] https://crrev.com/85479851289867a0f4b6ee0eb1913942104a2f3b/ash/wm/non_client_frame_controller.h
,
Aug 2
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by msw@chromium.org
, Jul 20