Attempt to fix the crash in blink Scroll Animator.

Attempt to fix the crash in blink Scroll Animator.

https://crrev.com/1695763003 didn't fix the crash. Maybe the crash is because
the delegate is actually null but the call succeeds and eventually results in
a crash.

Another speculation is that ScrollAnimatorCompositorCoordinator gets garbage
collected by oilpan, but the player doesn't know about it. In that case, the
fix would be to use USING_PRE_FINALIZER and clear the player in dispose().

BUG=586400
Committed: https://crrev.com/8ce9d13097b1b0e14b9710b0b2670a510e258502
Cr-Commit-Position: refs/heads/master@{#377005}

[ymalik, 2016-02-18 22:34:40]

Description was changed from

==========
Attempt to fix the crash in blink Scroll Animator.

https://crrev.com/1695763003 didn't fix the crash. Maybe the crash is because
the delegate is actually null but the call succeeds and eventually results in
a crash.

Another speculation is that ScrollAnimatorCompositorCoordinator gets garbage
collected by oilpan, but the player doesn't know about it. In that case, the
fix would be to use USING_PRE_FINALIZER and clear the player in dispose(). Will
try that next if this doesn't work since (using USING_PRE_FINALIZER is less
efficient).

BUG=586400
==========

to

==========
Attempt to fix the crash in blink Scroll Animator.

https://crrev.com/1695763003 didn't fix the crash. Maybe the crash is because
the delegate is actually null but the call succeeds and eventually results in
a crash.

Another speculation is that ScrollAnimatorCompositorCoordinator gets garbage
collected by oilpan, but the player doesn't know about it. In that case, the
fix would be to use USING_PRE_FINALIZER and clear the player in dispose(). Will
try that next if this doesn't work since (using USING_PRE_FINALIZER is less
efficient).

BUG=586400
==========

[ymalik, 2016-02-18 22:34:40]

ymalik@chromium.org changed reviewers:
+ ajuma@chromium.org

[ajuma, 2016-02-18 22:44:35]

This change looks good, but I think you'll still need the other change you
mentioned in the description, since (based on my understanding of Oilpan) if we
wait till ScrollAnimatorCompositorCoordinator's destructor is run to clear the
delegate, there will be a period of time (in between when a pre-finalizer would
have run and when the destructor actually runs) where
ScrollAnimatorCompositorCoordinator is reachable from CompositorAnimationPlayer,
but it's not safe to call scrollableArea() on
ScrollAnimatorCompositorCoordinator.

[bokan, 2016-02-18 22:51:15]

bokan@chromium.org changed reviewers:
+ bokan@chromium.org

[bokan, 2016-02-18 22:51:16]

Why not just trace the m_delegate so that OilPan knows not to destruct it in the
first place?

[ymalik, 2016-02-18 23:18:08]

On 2016/02/18 22:51:16, bokan wrote:
> Why not just trace the m_delegate so that OilPan knows not to destruct it in
the
> first place?

I think for that CompositorAnimationPlayer will need to inherit from one of the
GarbageCollectedFinalized to be added to the oilpan heap. Not sure how easy that
is and what the right thing to do is.

[ymalik, 2016-02-20 22:39:05]

Description was changed from

==========
Attempt to fix the crash in blink Scroll Animator.

https://crrev.com/1695763003 didn't fix the crash. Maybe the crash is because
the delegate is actually null but the call succeeds and eventually results in
a crash.

Another speculation is that ScrollAnimatorCompositorCoordinator gets garbage
collected by oilpan, but the player doesn't know about it. In that case, the
fix would be to use USING_PRE_FINALIZER and clear the player in dispose(). Will
try that next if this doesn't work since (using USING_PRE_FINALIZER is less
efficient).

BUG=586400
==========

to

==========
Attempt to fix the crash in blink Scroll Animator.

https://crrev.com/1695763003 didn't fix the crash. Maybe the crash is because
the delegate is actually null but the call succeeds and eventually results in
a crash.

Another speculation is that ScrollAnimatorCompositorCoordinator gets garbage
collected by oilpan, but the player doesn't know about it. In that case, the
fix would be to use USING_PRE_FINALIZER and clear the player in dispose().

BUG=586400
==========

[ymalik, 2016-02-20 22:41:58]

Hey guys, PTAL :)

Added WILL_BE_USING_PRE_FINALIZER to ScrollAnimatorCompositorCoordinator. I
realized that we already had that for ScrollAnimatorMac. I think doing what
David suggested is more work because we will have to add the
CompositorAnimationDelegate onto the oilpan heap.

[ajuma, 2016-02-22 15:15:01]

https://codereview.chromium.org/1714663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.h (left):

https://codereview.chromium.org/1714663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.h:48:
WILL_BE_USING_PRE_FINALIZER(ScrollAnimatorMac, dispose);
I'm not sure about this change. Is this because you've added a pre-finalizer to
an ancestor class? I don't think that implies that this class' dispose method
will get called.

https://codereview.chromium.org/1714663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (left):

https://codereview.chromium.org/1714663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:97: #if OS(MACOSX)
&& ENABLE(OILPAN)
Instead of using an #if, WDYT of adding a virtual method that has an empty
implementation in ScrollAnimatorCompositorCoordinator and whose implementation
in ScrollAnimatorMac calls dispose()?

[bokan, 2016-02-22 15:55:35]

https://codereview.chromium.org/1714663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (left):

https://codereview.chromium.org/1714663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:97: #if OS(MACOSX)
&& ENABLE(OILPAN)
On 2016/02/22 15:15:01, ajuma wrote:
> Instead of using an #if, WDYT of adding a virtual method that has an empty
> implementation in ScrollAnimatorCompositorCoordinator and whose implementation
> in ScrollAnimatorMac calls dispose()?

Why do we need to manually call dispose here at all? The animator is GCd so
can't we just clear the pointer and let Oilpan call dispose when the time is
right?

If there's something else that we need to happen *right here* (e.g. I see
ScrollAnimatorMac invalidates some painting code) then we should be doing that
through a method other than dispose.

[ymalik, 2016-02-22 19:22:56]

https://codereview.chromium.org/1714663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.h (left):

https://codereview.chromium.org/1714663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.h:48:
WILL_BE_USING_PRE_FINALIZER(ScrollAnimatorMac, dispose);
On 2016/02/22 15:15:01, ajuma wrote:
> I'm not sure about this change. Is this because you've added a pre-finalizer
to
> an ancestor class? I don't think that implies that this class' dispose method
> will get called.

You're right. This should stay.

https://codereview.chromium.org/1714663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (left):

https://codereview.chromium.org/1714663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:97: #if OS(MACOSX)
&& ENABLE(OILPAN)
On 2016/02/22 15:55:35, bokan wrote:
> On 2016/02/22 15:15:01, ajuma wrote:
> > Instead of using an #if, WDYT of adding a virtual method that has an empty
> > implementation in ScrollAnimatorCompositorCoordinator and whose
implementation
> > in ScrollAnimatorMac calls dispose()?
> 
> Why do we need to manually call dispose here at all? The animator is GCd so
> can't we just clear the pointer and let Oilpan call dispose when the time is
> right?
> 
> If there's something else that we need to happen *right here* (e.g. I see
> ScrollAnimatorMac invalidates some painting code) then we should be doing that
> through a method other than dispose.

I think they have this call here because if oilpan is not enabled, dispose would
be called by the destructor, but if oilpan is enabled, presumably we want to
"dispose" right when ~ScrollableArea is called and not wait for dispose to be
called by oilpan (this function is called by ~ScrollableArea).

[bokan, 2016-02-22 19:35:46]

https://codereview.chromium.org/1714663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (left):

https://codereview.chromium.org/1714663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:97: #if OS(MACOSX)
&& ENABLE(OILPAN)
On 2016/02/22 19:22:56, ymalik1 wrote:
> On 2016/02/22 15:55:35, bokan wrote:
> > On 2016/02/22 15:15:01, ajuma wrote:
> > > Instead of using an #if, WDYT of adding a virtual method that has an empty
> > > implementation in ScrollAnimatorCompositorCoordinator and whose
> implementation
> > > in ScrollAnimatorMac calls dispose()?
> > 
> > Why do we need to manually call dispose here at all? The animator is GCd so
> > can't we just clear the pointer and let Oilpan call dispose when the time is
> > right?
> > 
> > If there's something else that we need to happen *right here* (e.g. I see
> > ScrollAnimatorMac invalidates some painting code) then we should be doing
that
> > through a method other than dispose.
> 
> I think they have this call here because if oilpan is not enabled, dispose
would
> be called by the destructor, but if oilpan is enabled, presumably we want to
> "dispose" right when ~ScrollableArea is called and not wait for dispose to be
> called by oilpan (this function is called by ~ScrollableArea).  

The only place I see this being called from is
PaintLayerScrollableArea::dispose(). With Oilpan on, destructors aren't called
at all IIRC. If we have something here that needs to happen at a specific
time/order, we shouldn't put it in the dispose method. Otherwise, just let
Oilpan handle it.

[loyso (OOO), 2016-02-22 23:47:20]

On 2016/02/22 19:35:46, bokan wrote:
> If we have something here that needs to happen at a specific
> time/order, we shouldn't put it in the dispose method. Otherwise, just let
> Oilpan handle it.
+1! It's better to be done as a special plumbed call to a designated member
function (like willCloseLayerTreeView or willSomething in another subsystems)

[loyso (OOO), 2016-02-22 23:47:29]

loyso@chromium.org changed reviewers:
+ loyso@chromium.org

[loyso (OOO), 2016-02-22 23:47:29]

https://codereview.chromium.org/1714663002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/animation/CompositorAnimationPlayer.cpp
(right):

https://codereview.chromium.org/1714663002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationPlayer.cpp:75:
if (m_delegate)
I think, these changes are unrelated to your fix. Could you set them as a
separate, dependent CL? Nevertheless, I don't see how it is connected to
not-calling setAnimationDelegate(nullptr).

[loyso (OOO), 2016-02-22 23:59:12]

A drive-by note/observation: ScrollableArea::scrollAnimator() approach might be
error prone (lazy init). Not having an explicit lifetime scope for the owned
object can be dangerous.
For example, you destroy ScrollableArea::m_scrollAnimator at some point of
shutdown, but because of complicated control flow you get that
ScrollableArea::scrollAnimator() call which create a new
ScrollableArea::m_scrollAnimator again.

[ymalik, 2016-02-23 00:44:07]

On 2016/02/22 19:35:46, bokan wrote:
>
https://codereview.chromium.org/1714663002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (left):
> 
>
https://codereview.chromium.org/1714663002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:97: #if
OS(MACOSX)
> && ENABLE(OILPAN)
> On 2016/02/22 19:22:56, ymalik1 wrote:
> > On 2016/02/22 15:55:35, bokan wrote:
> > > On 2016/02/22 15:15:01, ajuma wrote:
> > > > Instead of using an #if, WDYT of adding a virtual method that has an
empty
> > > > implementation in ScrollAnimatorCompositorCoordinator and whose
> > implementation
> > > > in ScrollAnimatorMac calls dispose()?
> > > 
> > > Why do we need to manually call dispose here at all? The animator is GCd
so
> > > can't we just clear the pointer and let Oilpan call dispose when the time
is
> > > right?
> > > 
> > > If there's something else that we need to happen *right here* (e.g. I see
> > > ScrollAnimatorMac invalidates some painting code) then we should be doing
> that
> > > through a method other than dispose.
> > 
> > I think they have this call here because if oilpan is not enabled, dispose
> would
> > be called by the destructor, but if oilpan is enabled, presumably we want to
> > "dispose" right when ~ScrollableArea is called and not wait for dispose to
be
> > called by oilpan (this function is called by ~ScrollableArea).  
> 
> The only place I see this being called from is
> PaintLayerScrollableArea::dispose(). With Oilpan on, destructors aren't called
> at all IIRC. If we have something here that needs to happen at a specific
> time/order, we shouldn't put it in the dispose method. Otherwise, just let
> Oilpan handle it.

PaintLayerScrollableArea::dispose() is called from ~PaintLayer, which is not in
the oilpan heap. This is what I meant above when I said "we want to call dispose
right when ~ScrollableArea is called and not wait for dispose to be called by
oilpan" (s/~ScrollableArea/~PaintLayer). I agree that if we have something that
needs to happen at a specific time/order, we shouldn't put it in the dispose
method. 

I just want to make sure that adding "USING_PRE_FINALIZER" to
ScrollAnimatorCompositorCoordinator actually fixes the crash without breaking
anything else. I can do the cleanup in a future CL where we only let oilpan call
dispose().

[ymalik, 2016-02-23 00:49:33]

https://codereview.chromium.org/1714663002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/animation/CompositorAnimationPlayer.cpp
(right):

https://codereview.chromium.org/1714663002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationPlayer.cpp:75:
if (m_delegate)
On 2016/02/22 23:47:29, loyso wrote:
> I think, these changes are unrelated to your fix. Could you set them as a
> separate, dependent CL? Nevertheless, I don't see how it is connected to
> not-calling setAnimationDelegate(nullptr).

The premise of this CL is that we need to call setAnimationDelegate(nullptr) at
the right time (when ScrollAnimator is actually swept away rather than when
~ScrollAnimator is called). So in that case, we need this here so that we don't
crash when a notification comes in after the ScrollAnimator is GCed.

[loyso (OOO), 2016-02-23 01:08:27]

On 2016/02/23 00:49:33, ymalik1 wrote:
> The premise of this CL is that we need to call setAnimationDelegate(nullptr)
at
> the right time (when ScrollAnimator is actually swept away rather than when
> ~ScrollAnimator is called). So in that case, we need this here so that we
don't
> crash when a notification comes in after the ScrollAnimator is GCed.
Thanks, ymalik! Source/platform/animation LGTM

[loyso (OOO), 2016-02-23 01:11:10]

CompositorAnimationPlayer::m_delegate isn't initialized to nullptr in ctor. Is
it?

[loyso (OOO), 2016-02-23 01:16:49]

On 2016/02/23 00:49:33, ymalik1 wrote:
> The premise of this CL is that we need to call setAnimationDelegate(nullptr)
at
> the right time (when ScrollAnimator is actually swept away rather than when
> ~ScrollAnimator is called). So in that case, we need this here so that we
don't
> crash when a notification comes in after the ScrollAnimator is GCed.

void
CompositorAnimationPlayer::setAnimationDelegate(WebCompositorAnimationDelegate*
delegate)
{
    m_delegate = delegate;
    m_animationPlayer->set_layer_animation_delegate(delegate ? this : nullptr);
}

void AnimationPlayer::NotifyAnimationFinished(base::TimeTicks monotonic_time,
    Animation::TargetProperty target_property, int group) {
  if (layer_animation_delegate_)
    layer_animation_delegate_->NotifyAnimationFinished(monotonic_time,
                                                       target_property, group);
}

notifications shouldn't come in, though (since you call
m_animationPlayer->set_layer_animation_delegate(nullptr) effectively)

[loyso (OOO), 2016-02-23 01:26:16]

Fixing the ctor init here: https://codereview.chromium.org/1720313002/

[ajuma, 2016-02-23 14:32:40]

lgtm

[bokan, 2016-02-23 15:02:12]

non-owner lgtm

[ymalik, 2016-02-23 15:10:22]

ymalik@chromium.org changed reviewers:
+ jbroman@chromium.org

[ymalik, 2016-02-23 15:10:22]

+Jeremy for Source/platform

[jbroman, 2016-02-23 15:18:17]

rs lgtm

[ymalik, 2016-02-23 15:25:31]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-02-23 15:26:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1714663002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1714663002/60001

[commit-bot: I haz the power, 2016-02-23 16:59:18]

Description was changed from

==========
Attempt to fix the crash in blink Scroll Animator.

https://crrev.com/1695763003 didn't fix the crash. Maybe the crash is because
the delegate is actually null but the call succeeds and eventually results in
a crash.

Another speculation is that ScrollAnimatorCompositorCoordinator gets garbage
collected by oilpan, but the player doesn't know about it. In that case, the
fix would be to use USING_PRE_FINALIZER and clear the player in dispose().

BUG=586400
==========

to

==========
Attempt to fix the crash in blink Scroll Animator.

https://crrev.com/1695763003 didn't fix the crash. Maybe the crash is because
the delegate is actually null but the call succeeds and eventually results in
a crash.

Another speculation is that ScrollAnimatorCompositorCoordinator gets garbage
collected by oilpan, but the player doesn't know about it. In that case, the
fix would be to use USING_PRE_FINALIZER and clear the player in dispose().

BUG=586400
==========

[commit-bot: I haz the power, 2016-02-23 16:59:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-23 17:00:42]

Description was changed from

==========
Attempt to fix the crash in blink Scroll Animator.

https://crrev.com/1695763003 didn't fix the crash. Maybe the crash is because
the delegate is actually null but the call succeeds and eventually results in
a crash.

Another speculation is that ScrollAnimatorCompositorCoordinator gets garbage
collected by oilpan, but the player doesn't know about it. In that case, the
fix would be to use USING_PRE_FINALIZER and clear the player in dispose().

BUG=586400
==========

to

==========
Attempt to fix the crash in blink Scroll Animator.

https://crrev.com/1695763003 didn't fix the crash. Maybe the crash is because
the delegate is actually null but the call succeeds and eventually results in
a crash.

Another speculation is that ScrollAnimatorCompositorCoordinator gets garbage
collected by oilpan, but the player doesn't know about it. In that case, the
fix would be to use USING_PRE_FINALIZER and clear the player in dispose().

BUG=586400

Committed: https://crrev.com/8ce9d13097b1b0e14b9710b0b2670a510e258502
Cr-Commit-Position: refs/heads/master@{#377005}
==========

[commit-bot: I haz the power, 2016-02-23 17:00:43]

Patchset 4 (id:??) landed as
https://crrev.com/8ce9d13097b1b0e14b9710b0b2670a510e258502
Cr-Commit-Position: refs/heads/master@{#377005}