Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
343753007:
Fix a leak in wm::AnimateOnChildWindowVisibilityChanged(). (Closed)
|
Created:
6 years, 6 months ago by please use gerrit instead Modified:
6 years, 5 months ago Reviewers:
sky CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionFix a leak in wm::AnimateOnChildWindowVisibilityChanged().
RotateHidingWindowAnimationObserver is leaking from
wm::AnimateOnChildWindowVisibilityChanged() because it's not observing
animation events. The fix is to make RotateHidingWindowAnimationObserver
observe the animation events.
BUG=389296
TEST=wm_unittests:WindowAnimationsTest.RotateHideNoLeak
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283970
Patch Set 1 #Patch Set 2 : WindowAnimationsTest.HideRotateAnimationNoLeak fails before the patch if LSan is enabled. #Patch Set 3 : Test only, which will fail on LSan bots without the fix. #Patch Set 4 : The fix together with the test only, which will not fail on LSan bots anymore. #
Total comments: 1
Patch Set 5 : Observe changes in layer animator. #Patch Set 6 : Only the test. #Patch Set 7 : The test and the fix. #
Total comments: 2
Patch Set 8 : Apply the animation to the correct layer. #
Total comments: 5
Patch Set 9 : Fix spelling in comment. #Patch Set 10 : Addressed comments. #
Total comments: 2
Patch Set 11 : Remove expect. #
Messages
Total messages: 31 (0 generated)
Scott: PTAL.
How about test coverage?
Scott: PTAL Patch Set 2. WindowAnimationsTest.HideRotateAnimationNoLeak fails before the patch if LSan is enabled.
Scott: To make your life as reviewer easier, I've uploaded the Patch Set 3 that has only the test and Patch Set 4 that has both the test and the fix. I then launched the *asan bots to show that Patch Set 3 will having failing tests, but Patch Set 4 will not.
On 2014/06/27 00:56:23, Rouslan Solomakhin wrote: > Scott: To make your life as reviewer easier, I've uploaded the Patch Set 3 that > has only the test and Patch Set 4 that has both the test and the fix. I then > launched the *asan bots to show that Patch Set 3 will having failing tests, but > Patch Set 4 will not. As expected, Patch Set 3 shows the leak, and Patch Set 4 does not. There're some use-after-frees that I can investigate to see if they are flakes or real bugs also (CreateLabelUnderPanel). Patch Set 3: http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos_asan/b... =============================================================================== SUMMARY: AddressSanitizer: 4024 byte(s) leaked in 15 allocation(s). [1026/1026] WindowAnimationsTest.HideRotateAnimationNoLeak (117 ms) 1 test failed on exit: WindowAnimationsTest.HideRotateAnimationNoLeak 1 test crashed: WindowSelectorTest.CreateLabelUnderPanel Tests took 212 seconds. Stopping Xvfb with pid 22188 ... Xvfb pid file removed /mnt/scratch0/b_used/build/scripts/slave/gsutil cp file:///tmp/tmpJlDhda gs://chrome-gtest-results/raw/2014/6/23/27/1d16ba41f502f7246a2f802df14bfd8b04281da2.json.gz Copying file:///tmp/tmpJlDhda [Content-Type=application/octet-stream]... /mnt/scratch0/b_used/build/scripts/slave/gsutil cp file:///tmp/tmpH2fhWA gs://chrome-gtest-results/bigquery/2014/6/23/27/1d16ba41f502f7246a2f802df14bfd8b04281da2.json.gz Copying file:///tmp/tmpH2fhWA [Content-Type=application/octet-stream]... exit code (as seen by runtest.py): 1 @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@WindowAnimationsTest.HideRotateAni... (run #1):@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@[ RUN ] WindowAnimationsTest.HideRotateAnimationNoLeak@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@Xlib: extension "RANDR" missing on display ":9".@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@[ OK ] WindowAnimationsTest.HideRotateAnimationNoLeak (119 ms)@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@[----------] 1 test from WindowAnimationsTest (119 ms total)@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@[----------] Global test environment tear-down@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@[==========] 1 test from 1 test case ran. (119 ms total)@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@[ PASSED ] 1 test.@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ YOU HAVE 16 DISABLED TESTS@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@=================================================================@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@==24515==ERROR: LeakSanitizer: detected memory leaks@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@Direct leak of 96 byte(s) in 1 object(s) allocated from:@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #0 0x48b5fb in operator new(unsigned long) /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:55@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #1 0x12fe6ba in wm::(anonymous namespace)::AddLayerAnimationsForRotate(aura::Window*, bool) ui/wm/core/window_animations.cc:437@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #2 0x12fd47f in AnimateHideWindow_Rotate ui/wm/core/window_animations.cc:496@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #3 0x12fd47f in AnimateHideWindow ui/wm/core/window_animations.cc:550@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #4 0x12fd47f in wm::AnimateOnChildWindowVisibilityChanged(aura::Window*, bool) ui/wm/core/window_animations.cc:637@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #5 0x1531a25 in ash::AnimateOnChildWindowVisibilityChanged(aura::Window*, bool) ash/wm/window_animations.cc:401@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #6 0xccc5c9 in ash::WindowAnimationsTest_HideRotateAnimationNoLeak_Test::TestBody() ash/wm/window_animations_unittest.cc:108@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #7 0xfc1298 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #8 0xfc1298 in testing::Test::Run() testing/gtest/src/gtest.cc:2061@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #9 0xfc33d8 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #10 0xfc4116 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #11 0xfd6dfa in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #12 0xfd6440 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #13 0xfd6440 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #14 0xf7f864 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #15 0xf7f864 in base::TestSuite::Run() base/test/test_suite.cc:227@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #16 0xf76c81 in Run base/callback.h:401@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #17 0xf76c81 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int, bool, base::Callback\u003Cvoid ()> const&) base/test/launcher/unit_test_launcher.cc:498@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #18 0xf779be in base::LaunchUnitTestsSerially(int, char**, base::Callback\u003Cint ()> const&) base/test/launcher/unit_test_launcher.cc:564@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #19 0xa090f4 in main ash/test/ash_unittests.cc:12@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #20 0x7ff77414a76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226@@@ @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@@@@ Patch Set 4: http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos_asan/b... =============================================================================== [698/1020] WindowAnimationsTest.HideRotateAnimationNoLeak (114 ms)
Yes, please investigate them to make sure they aren't from your patch. -Scott On Fri, Jun 27, 2014 at 8:49 AM, <rouslan@chromium.org> wrote: > On 2014/06/27 00:56:23, Rouslan Solomakhin wrote: > >> Scott: To make your life as reviewer easier, I've uploaded the Patch Set 3 >> > that > >> has only the test and Patch Set 4 that has both the test and the fix. I >> then >> launched the *asan bots to show that Patch Set 3 will having failing >> tests, >> > but > >> Patch Set 4 will not. >> > > As expected, Patch Set 3 shows the leak, and Patch Set 4 does not. > There're some > use-after-frees that I can investigate to see if they are flakes or real > bugs > also (CreateLabelUnderPanel). > > Patch Set 3: > http://build.chromium.org/p/tryserver.chromium/builders/ > linux_chromeos_asan/builds/17271/steps/ash_unittests/logs/stdio > ============================================================ > =================== > SUMMARY: AddressSanitizer: 4024 byte(s) leaked in 15 allocation(s). > [1026/1026] WindowAnimationsTest.HideRotateAnimationNoLeak (117 ms) > 1 test failed on exit: > WindowAnimationsTest.HideRotateAnimationNoLeak > 1 test crashed: > WindowSelectorTest.CreateLabelUnderPanel > Tests took 212 seconds. > Stopping Xvfb with pid 22188 ... > Xvfb pid file removed > > /mnt/scratch0/b_used/build/scripts/slave/gsutil cp file:///tmp/tmpJlDhda > gs://chrome-gtest-results/raw/2014/6/23/27/1d16ba41f502f7246a2f802df14bfd > 8b04281da2.json.gz > Copying file:///tmp/tmpJlDhda [Content-Type=application/octet-stream]... > > /mnt/scratch0/b_used/build/scripts/slave/gsutil cp file:///tmp/tmpH2fhWA > gs://chrome-gtest-results/bigquery/2014/6/23/27/ > 1d16ba41f502f7246a2f802df14bfd8b04281da2.json.gz > Copying file:///tmp/tmpH2fhWA [Content-Type=application/octet-stream]... > exit code (as seen by runtest.py): 1 > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@WindowAnimationsTest. > HideRotateAnimationNoLeak > (run #1):@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@[ RUN ] > WindowAnimationsTest.HideRotateAnimationNoLeak@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@Xlib: extension "RANDR" > missing on > display ":9".@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@[ OK ] > WindowAnimationsTest.HideRotateAnimationNoLeak (119 ms)@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@[----------] 1 test from > WindowAnimationsTest (119 ms total)@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@[----------] Global test > environment > tear-down@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@[==========] 1 test from 1 > test case > ran. (119 ms total)@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@[ PASSED ] 1 test.@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ YOU HAVE 16 DISABLED TESTS@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@==== > =============================================================@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@==24515==ERROR: LeakSanitizer: > detected memory leaks@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@Direct leak of 96 byte(s) in 1 > object(s) allocated from:@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #0 0x48b5fb in operator > new(unsigned long) > /usr/local/google/work/chromium/src/third_party/llvm/ > projects/compiler-rt/lib/asan/asan_new_delete.cc:55@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #1 0x12fe6ba in > wm::(anonymous > namespace)::AddLayerAnimationsForRotate(aura::Window*, bool) > ui/wm/core/window_animations.cc:437@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #2 0x12fd47f in > AnimateHideWindow_Rotate ui/wm/core/window_animations.cc:496@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #3 0x12fd47f in > AnimateHideWindow > ui/wm/core/window_animations.cc:550@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #4 0x12fd47f in > wm::AnimateOnChildWindowVisibilityChanged(aura::Window*, bool) > ui/wm/core/window_animations.cc:637@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #5 0x1531a25 in > ash::AnimateOnChildWindowVisibilityChanged(aura::Window*, bool) > ash/wm/window_animations.cc:401@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #6 0xccc5c9 in > ash::WindowAnimationsTest_HideRotateAnimationNoLeak_Test::TestBody() > ash/wm/window_animations_unittest.cc:108@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #7 0xfc1298 in > HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> > testing/gtest/src/gtest.cc:2045@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #8 0xfc1298 in > testing::Test::Run() testing/gtest/src/gtest.cc:2061@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #9 0xfc33d8 in > testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #10 0xfc4116 in > testing::TestCase::Run() testing/gtest/src/gtest.cc:2344@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #11 0xfd6dfa in > testing::internal::UnitTestImpl::RunAllTests() > testing/gtest/src/gtest.cc:4065@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #12 0xfd6440 in > HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, > bool> > testing/gtest/src/gtest.cc:2045@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #13 0xfd6440 in > testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #14 0xf7f864 in > RUN_ALL_TESTS > testing/gtest/include/gtest/gtest.h:2231@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #15 0xf7f864 in > base::TestSuite::Run() base/test/test_suite.cc:227@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #16 0xf76c81 in Run > base/callback.h:401@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #17 0xf76c81 in > base::(anonymous > namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, > int, > bool, base::Callback\u003Cvoid ()> const&) > base/test/launcher/unit_test_launcher.cc:498@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #18 0xf779be in > base::LaunchUnitTestsSerially(int, char**, base::Callback\u003Cint ()> > const&) > base/test/launcher/unit_test_launcher.cc:564@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #19 0xa090f4 in main > ash/test/ash_unittests.cc:12@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@ #20 0x7ff77414a76c in > __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226@@@ > @@@STEP_LOG_LINE@HideRotateAnimationNoLeak@@@@ > > > Patch Set 4: > http://build.chromium.org/p/tryserver.chromium/builders/ > linux_chromeos_asan/builds/17272/steps/ash_unittests/logs/stdio > ============================================================ > =================== > [698/1020] WindowAnimationsTest.HideRotateAnimationNoLeak (114 ms) > > > > https://codereview.chromium.org/343753007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/06/27 18:09:50, sky wrote: > Yes, please investigate them to make sure they aren't from your patch. > On Fri, Jun 27, 2014 at 8:49 AM, <mailto:rouslan@chromium.org> wrote: > > There're some > > use-after-frees that I can investigate to see if they are flakes or real > > bugs > > also (CreateLabelUnderPanel). Investigations locally did not turn up any information on CreateLabelUnderPanel. I'm going to write it up to a flake. PTAL Patch Set 4.
https://codereview.chromium.org/343753007/diff/60001/ui/wm/core/window_animat... File ui/wm/core/window_animations.cc (right): https://codereview.chromium.org/343753007/diff/60001/ui/wm/core/window_animat... ui/wm/core/window_animations.cc:115: void OnAnimationCompleted() { I would have thought we end up here. What particular subclass gets created and how come it doesn't invoke this?
On 2014/06/27 22:32:56, sky (OOO) wrote: > https://codereview.chromium.org/343753007/diff/60001/ui/wm/core/window_animat... > File ui/wm/core/window_animations.cc (right): > > https://codereview.chromium.org/343753007/diff/60001/ui/wm/core/window_animat... > ui/wm/core/window_animations.cc:115: void OnAnimationCompleted() { > I would have thought we end up here. What particular subclass gets created RotateHidingWindowAnimationObserver is created here: https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/window_... > how come it doesn't invoke this? I can take a look.
On 2014/06/27 23:41:50, Rouslan Solomakhin wrote: > On 2014/06/27 22:32:56, sky (OOO) wrote: > > > https://codereview.chromium.org/343753007/diff/60001/ui/wm/core/window_animat... > > File ui/wm/core/window_animations.cc (right): > > > > > https://codereview.chromium.org/343753007/diff/60001/ui/wm/core/window_animat... > > ui/wm/core/window_animations.cc:115: void OnAnimationCompleted() { > > I would have thought we end up here. What particular subclass gets created > > RotateHidingWindowAnimationObserver is created here: > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/window_... > > > how come it doesn't invoke this? > > I can take a look. Ahah! That was an excellent question, because it revealed that RotateHidingWindowAnimationObserver never observed ui::LayerAnimator, so it would never call OnAnimationCompleted. The fix is now much simpler: observe the events. The test, however, is no longer usable. I will attempt to write a test reproduces the problem correctly. If you have any suggestions, please let me know :-)
Scott: PTAL Patch Sets 6 & 7. Patch Set 6 has only the test, which is failing the asan try-jobs. Patch Set 7 has the additional bug-fix, which is now passing the asan try-jobs. I've also updated CL description to description: "RotateHidingWindowAnimationObserver is leaking from wm::AnimateOnChildWindowVisibilityChanged() because it's not observing animation events. The fix is to make RotateHidingWindowAnimationObserver observe the animation events."
On 2014/07/01 22:07:33, Rouslan Solomakhin wrote: > Patch Set 6 has only the test, which is failing the asan try-jobs. None of the bots are running wm_unittests, which prevents us from seeing the failure here.
Scott: ping.
https://codereview.chromium.org/343753007/diff/120001/ui/wm/core/window_anima... File ui/wm/core/window_animations.cc (right): https://codereview.chromium.org/343753007/diff/120001/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:485: observer->SetLastSequence(last_sequence); If you do this here, doesn't that mean the animation that is started on the next line applies to the new layer and not the old?
https://codereview.chromium.org/343753007/diff/120001/ui/wm/core/window_anima... File ui/wm/core/window_animations.cc (right): https://codereview.chromium.org/343753007/diff/120001/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:485: observer->SetLastSequence(last_sequence); On 2014/07/09 20:32:12, sky wrote: > If you do this here, doesn't that mean the animation that is started on the next > line applies to the new layer and not the old? Hm, I think you're right. The problem I'm encountering is that DetachAndRecreateLayers() should be ran while the window still exists, but before the animation sequence is finished. Unit tests finish animations immediately, so DetachAndRecreateLayers() cannot be run after the next line. Do you have any pointers on how to make unit tests finish animations only after AddLayerAnimationsForRotate exits?
I think I've figured it out. I need to use ui::ScopedAnimationDurationScaleMode. Let me try it an get back to you.
Scott: PTAL Patch Set 8. It applies the animation to the correct layer.
https://codereview.chromium.org/343753007/diff/140001/ui/wm/core/window_anima... File ui/wm/core/window_animations.cc (right): https://codereview.chromium.org/343753007/diff/140001/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:492: observer->detached_layer_tree_root()->GetAnimator()->ScheduleAnimation( Isn't the important part here your changes to SetLastSequence? In which case can't you leave 484 old and nuke 492-495 new? https://codereview.chromium.org/343753007/diff/140001/ui/wm/core/window_anima... File ui/wm/core/window_animations_unittest.cc (right): https://codereview.chromium.org/343753007/diff/140001/ui/wm/core/window_anima... ui/wm/core/window_animations_unittest.cc:291: // The rotation animation for hidiing a window should not leak the animation hidiing->hiding
Please see the responses inline. https://codereview.chromium.org/343753007/diff/140001/ui/wm/core/window_anima... File ui/wm/core/window_animations.cc (right): https://codereview.chromium.org/343753007/diff/140001/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:492: observer->detached_layer_tree_root()->GetAnimator()->ScheduleAnimation( On 2014/07/14 19:18:18, sky wrote: > Isn't the important part here your changes to SetLastSequence? Yes. > In which case > can't you leave 484 old and nuke 492-495 new? Yes, but then we would not have a unit test for checking the leak. Is that OK with you? Here is why: Calling [ window->layer()->GetAnimator()->ScheduleAnimation(last_sequence) ] before [ observer->DetachAndRecreateLayers() ] works when running in the browser, because scheduled animations will complete after exiting [ AddLayerAnimationsForRotate() ] function. However, unit tests run animations in either (a) zero time or (b) not at all, in my experience. a) Zero time animations finish [ last_sequence ] before [ set_last_sequence() ], so this causes a leak in the new unit test. b) Any other speed of animations does not finish the animations in unit tests. Only [ OnLayerAnimationScheduled() ] gets called. [ OnLayerAnimationEnded() ] and [ OnLayerAnimationAborted() ] are never called. This also causes a leak in the new unit test. TL;DR: Simpler change means no unit test. https://codereview.chromium.org/343753007/diff/140001/ui/wm/core/window_anima... File ui/wm/core/window_animations_unittest.cc (right): https://codereview.chromium.org/343753007/diff/140001/ui/wm/core/window_anima... ui/wm/core/window_animations_unittest.cc:291: // The rotation animation for hidiing a window should not leak the animation On 2014/07/14 19:18:18, sky wrote: > hidiing->hiding Done.
LayerAnimator has some functions for use in tests. Can you use set_disable_timer_for_test?
Scott: PTAL Patch Set 10. The trick for unit tests was to call StopAnimating() on the layer of the original window before detachment. https://codereview.chromium.org/343753007/diff/140001/ui/wm/core/window_anima... File ui/wm/core/window_animations.cc (right): https://codereview.chromium.org/343753007/diff/140001/ui/wm/core/window_anima... ui/wm/core/window_animations.cc:492: observer->detached_layer_tree_root()->GetAnimator()->ScheduleAnimation( On 2014/07/14 19:18:18, sky wrote: > Isn't the important part here your changes to SetLastSequence? In which case > can't you leave 484 old and nuke 492-495 new? Done.
LGTM https://codereview.chromium.org/343753007/diff/180001/ui/wm/core/window_anima... File ui/wm/core/window_animations_unittest.cc (right): https://codereview.chromium.org/343753007/diff/180001/ui/wm/core/window_anima... ui/wm/core/window_animations_unittest.cc:305: EXPECT_NO_FATAL_FAILURE(animating_layer->GetAnimator()->StopAnimating()); None of the call you're calling in this macro asserts/expects. So there is no point in wrapping it in EXPECT_NO_FATAL_FAILURE.
Thank you for the review! Sending to CQ now. https://codereview.chromium.org/343753007/diff/180001/ui/wm/core/window_anima... File ui/wm/core/window_animations_unittest.cc (right): https://codereview.chromium.org/343753007/diff/180001/ui/wm/core/window_anima... ui/wm/core/window_animations_unittest.cc:305: EXPECT_NO_FATAL_FAILURE(animating_layer->GetAnimator()->StopAnimating()); On 2014/07/17 15:59:40, sky wrote: > None of the call you're calling in this macro asserts/expects. So there is no > point in wrapping it in EXPECT_NO_FATAL_FAILURE. Done.
The CQ bit was checked by rouslan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/343753007/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by rouslan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/343753007/200001
Message was sent while issue was closed.
Change committed as 283970 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
