Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
795113002:
compositor/layer_animator: handle delegate removal in LayerAnimation::StopAnimating (Closed)
|
Created:
6 years ago by Lukasz Jagielski Modified:
5 years, 11 months ago Reviewers:
sky CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLayerAnimator should withdraw from processing animation queue when its delegate becomes nullptr.
BUG=
Committed: https://crrev.com/205709806da5c07eb640fe256d473f1a64f56e44
Cr-Commit-Position: refs/heads/master@{#310354}
Patch Set 1 #Patch Set 2 : Added test #
Total comments: 5
Patch Set 3 : Removed bad test and implemented correct one. #
Total comments: 2
Patch Set 4 : Worked everything around. #
Total comments: 2
Patch Set 5 : Fixed test. #
Total comments: 5
Patch Set 6 : More fixups for test #
Messages
Total messages: 21 (2 generated)
ljagielski@opera.com changed reviewers: + sky@chromium.org
Hi, PTAL at my CL attempt. I have no bug for this, because I'm unable to reproduce this in chromium tree. I came across this crash running some browsertest for our product and I think this fix also applies to chromium. It happened when hiding a web contents modal dialog coincided with browser process termination, animations started by AddLayersAnimationsForRotate were completed in an unexpected order and there was a crash. Regards, Łukasz
How about some test coverage?
On 2014/12/11 16:28:17, sky wrote: > How about some test coverage? LayerAnimator has code you can use in testing to step through animations. See LayerAnimator::set_disable_timer_for_test.
On 2014/12/11 16:35:21, sky wrote: > On 2014/12/11 16:28:17, sky wrote: > > How about some test coverage? > > LayerAnimator has code you can use in testing to step through animations. See > LayerAnimator::set_disable_timer_for_test. Ok, I added a test for this. I'm not completely sure though if it's what you expected. Thanks, Łukasz
https://codereview.chromium.org/795113002/diff/20001/ui/compositor/layer_anim... File ui/compositor/layer_animator_unittest.cc (right): https://codereview.chromium.org/795113002/diff/20001/ui/compositor/layer_anim... ui/compositor/layer_animator_unittest.cc:2616: nit: no newline here. https://codereview.chromium.org/795113002/diff/20001/ui/compositor/layer_anim... ui/compositor/layer_animator_unittest.cc:2617: CompletionOrderAnimationObserver() {}; no ; https://codereview.chromium.org/795113002/diff/20001/ui/compositor/layer_anim... ui/compositor/layer_animator_unittest.cc:2629: const std::vector<LayerAnimationSequence *>& GetCompletionOrder() { Make this function const too and name completion_order(). https://codereview.chromium.org/795113002/diff/20001/ui/compositor/layer_anim... ui/compositor/layer_animator_unittest.cc:2633: void OnLayerAnimationScheduled(LayerAnimationSequence* sequence) override {} Keep the LayerAnimationObserver methods together. Style guid says, "When you derive from a base class, group any overriding functions in your header file in one labeled section". https://codereview.chromium.org/795113002/diff/20001/ui/compositor/layer_anim... ui/compositor/layer_animator_unittest.cc:2643: TEST(LayerAnimatorTest, StopAnimatingCompletionOrder) { This is an interesting test, but what you want is a test that fails without your patch and passes with your patch.
https://codereview.chromium.org/795113002/diff/40001/ui/wm/core/window_animat... File ui/wm/core/window_animations.cc (right): https://codereview.chromium.org/795113002/diff/40001/ui/wm/core/window_animat... ui/wm/core/window_animations.cc:408: sequences_.push_back(sequence); I think you're working around the bug. OnLayerAnimationEnded is being called correctly and 'this' attempts to delete itself. The problem is the deletion is triggered from a call in LayerAnimator. LayerAnimator is ref counted and isn't destroyed. When the stack unravels LayerAnimator attempts to use its delegate(), which is the Layer that was destroyed and we crash. The fix should be in LayerAnimator. https://codereview.chromium.org/795113002/diff/40001/ui/wm/core/window_animat... ui/wm/core/window_animations.cc:458: ui::LayerAnimationSequence *opacity_sequence = nit: "ui::LayerAnimationSequence *" -> "ui::LayerAnimationSequence* "
On 2014/12/15 17:48:54, sky wrote: > https://codereview.chromium.org/795113002/diff/40001/ui/wm/core/window_animat... > File ui/wm/core/window_animations.cc (right): > Ok, but what is the correct behaviour here, aside from not crashing. Should LayerAnimator give up processing the pending animations in StopAnimatingInternal() as soon as nullptr delegate is discovered? Or maybe it should remove all the pending and running animations instantly? Or maybe something else? > https://codereview.chromium.org/795113002/diff/40001/ui/wm/core/window_animat... > ui/wm/core/window_animations.cc:408: sequences_.push_back(sequence); > I think you're working around the bug. OnLayerAnimationEnded is being called > correctly and 'this' attempts to delete itself. The problem is the deletion is > triggered from a call in LayerAnimator. LayerAnimator is ref counted and isn't > destroyed. When the stack unravels LayerAnimator attempts to use its delegate(), > which is the Layer that was destroyed and we crash. The fix should be in > LayerAnimator. > > https://codereview.chromium.org/795113002/diff/40001/ui/wm/core/window_animat... > ui/wm/core/window_animations.cc:458: ui::LayerAnimationSequence > *opacity_sequence = > nit: "ui::LayerAnimationSequence *" -> "ui::LayerAnimationSequence* "
Without a delegate there is no point in continuing. -Scott On Tue, Dec 16, 2014 at 12:40 PM, <ljagielski@opera.com> wrote: > On 2014/12/15 17:48:54, sky wrote: > > https://codereview.chromium.org/795113002/diff/40001/ui/wm/core/window_animat... >> >> File ui/wm/core/window_animations.cc (right): > > > Ok, but what is the correct behaviour here, aside from not crashing. Should > LayerAnimator > give up processing the pending animations in StopAnimatingInternal() as soon > as > nullptr delegate > is discovered? Or maybe it should remove all the pending and running > animations > instantly? Or > maybe something else? > > > > > https://codereview.chromium.org/795113002/diff/40001/ui/wm/core/window_animat... >> >> ui/wm/core/window_animations.cc:408: sequences_.push_back(sequence); >> I think you're working around the bug. OnLayerAnimationEnded is being >> called >> correctly and 'this' attempts to delete itself. The problem is the >> deletion is >> triggered from a call in LayerAnimator. LayerAnimator is ref counted and >> isn't >> destroyed. When the stack unravels LayerAnimator attempts to use its > > delegate(), >> >> which is the Layer that was destroyed and we crash. The fix should be in >> LayerAnimator. > > > > https://codereview.chromium.org/795113002/diff/40001/ui/wm/core/window_animat... >> >> ui/wm/core/window_animations.cc:458: ui::LayerAnimationSequence >> *opacity_sequence = >> nit: "ui::LayerAnimationSequence *" -> "ui::LayerAnimationSequence* " > > > > > https://codereview.chromium.org/795113002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ok, I hope I got a bit closer to correct solution :) Regards, Łukasz On 2014/12/16 23:28:09, sky wrote: > Without a delegate there is no point in continuing. > > -Scott > > On Tue, Dec 16, 2014 at 12:40 PM, <mailto:ljagielski@opera.com> wrote: > > On 2014/12/15 17:48:54, sky wrote: > > > > > https://codereview.chromium.org/795113002/diff/40001/ui/wm/core/window_animat... > >> > >> File ui/wm/core/window_animations.cc (right): > > > > > > Ok, but what is the correct behaviour here, aside from not crashing. Should > > LayerAnimator > > give up processing the pending animations in StopAnimatingInternal() as soon > > as > > nullptr delegate > > is discovered? Or maybe it should remove all the pending and running > > animations > > instantly? Or > > maybe something else? > > > > > > > > > > > https://codereview.chromium.org/795113002/diff/40001/ui/wm/core/window_animat... > >> > >> ui/wm/core/window_animations.cc:408: sequences_.push_back(sequence); > >> I think you're working around the bug. OnLayerAnimationEnded is being > >> called > >> correctly and 'this' attempts to delete itself. The problem is the > >> deletion is > >> triggered from a call in LayerAnimator. LayerAnimator is ref counted and > >> isn't > >> destroyed. When the stack unravels LayerAnimator attempts to use its > > > > delegate(), > >> > >> which is the Layer that was destroyed and we crash. The fix should be in > >> LayerAnimator. > > > > > > > > > https://codereview.chromium.org/795113002/diff/40001/ui/wm/core/window_animat... > >> > >> ui/wm/core/window_animations.cc:458: ui::LayerAnimationSequence > >> *opacity_sequence = > >> nit: "ui::LayerAnimationSequence *" -> "ui::LayerAnimationSequence* " > > > > > > > > > > https://codereview.chromium.org/795113002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/795113002/diff/60001/ui/compositor/layer_anim... File ui/compositor/layer_animator_unittest.cc (right): https://codereview.chromium.org/795113002/diff/60001/ui/compositor/layer_anim... ui/compositor/layer_animator_unittest.cc:2622: ~DelegateOwnerAnimationObserver() { animator_->SetDelegate(nullptr); } override https://codereview.chromium.org/795113002/diff/60001/ui/compositor/layer_anim... ui/compositor/layer_animator_unittest.cc:2653: scoped_refptr<LayerAnimator> animator( Can you write this test using the layer and deleting the layer as the window unit test is doing? That way the test better matches usage.
I tweaked the test according to comments. Regards, Łukasz
https://codereview.chromium.org/795113002/diff/80001/ui/compositor/layer_anim... File ui/compositor/layer_animator_unittest.cc (right): https://codereview.chromium.org/795113002/diff/80001/ui/compositor/layer_anim... ui/compositor/layer_animator_unittest.cc:2621: virtual ~LayerOwnerAnimationObserver() {} no virtual, just override. https://codereview.chromium.org/795113002/diff/80001/ui/compositor/layer_anim... ui/compositor/layer_animator_unittest.cc:2633: LayerAnimationDelegate* get_animator_delegate() { Style guide says this should be named animator_layer() (match the field name). https://codereview.chromium.org/795113002/diff/80001/ui/compositor/layer_anim... ui/compositor/layer_animator_unittest.cc:2640: void reset_layer() { animator_layer_.reset(); } This function is a single liner and doesn't add any value. Make the call sites call animator_layer_.reset() and nuke the function. https://codereview.chromium.org/795113002/diff/80001/ui/compositor/layer_anim... ui/compositor/layer_animator_unittest.cc:2647: TEST(LayerAnimatorTest, ObserverRemovesLayerInStopAnimating) { Removes->Deletes. https://codereview.chromium.org/795113002/diff/80001/ui/compositor/layer_anim... ui/compositor/layer_animator_unittest.cc:2675: EXPECT_EQ(observer.get_animator_delegate(), nullptr); assertions have the format expected, actual. nullptr should be first.
LGTM
The CQ bit was checked by ljagielski@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/795113002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/205709806da5c07eb640fe256d473f1a64f56e44 Cr-Commit-Position: refs/heads/master@{#310354} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
