1249564 - AnimationEffectTiming:GetParentObject should return a valid object

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Currently nobody sets mParent in the class.

Comment 1

[Boris Chiou [:boris]]

Hi Brian,

I'm thinking what should AnimaitonEffectTiminig::GetParentObject() return. According to WebIDL binding document, walking the GetParentObject chain will eventually get you to a Window, so that every WebIDL object is associated with a particular Window. For AnimationEffectTiming object, I think the GetParentObjact chain could be:
AnimationEffectTiming -> KeyframeEffect -> Animation -> AnimationTimeline -> AnimationTimeline.mWindow.

However, I have a basic problem about the the relationship. Animation uses a ref-counted pointer to a KeyframeEffectReadOnly, and the KeyframeEffectReadOnly also has a ref-counted pointer to this Animation. I'm not familiar with the cycle collection but is it possible that the ref-counted cycle causes some leaks?

Comment 2

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris]  from comment #1)
> Hi Brian,
> 
> I'm thinking what should AnimaitonEffectTiminig::GetParentObject() return.
> According to WebIDL binding document, walking the GetParentObject chain will
> eventually get you to a Window, so that every WebIDL object is associated
> with a particular Window. For AnimationEffectTiming object, I think the
> GetParentObjact chain could be:
> AnimationEffectTiming -> KeyframeEffect -> Animation -> AnimationTimeline ->
> AnimationTimeline.mWindow.

Sorry, I have to update this chain:

KeyframeEffect uses the document of its target as the parent object, so the chain should be:
AnimationEffectTiming -> KeyframeEffect -> target's document.

Comment 3

[Boris Chiou [:boris]]

Attachment: Assign parent Object for AnimationEffectTiming

Comment 4

[Boris Chiou [:boris]]

Attachment: Assign the parent object of AnimationEffectTiming(ReadOnly) (v2)

Comment 5

[Boris Chiou [:boris]]

Attachment: Assign the parent object of AnimationEffectTiming(ReadOnly) (v3)

Update comments.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Boris Chiou [:boris]  from comment #1)
> However, I have a basic problem about the the relationship. Animation uses a
> ref-counted pointer to a KeyframeEffectReadOnly, and the
> KeyframeEffectReadOnly also has a ref-counted pointer to this Animation. I'm
> not familiar with the cycle collection but is it possible that the
> ref-counted cycle causes some leaks?

We shouldn't be leaking. A KeyframeEffectReadOnly needs to keep its Animation alive and vice versa. If they are the only things keeping each other alive then the cycle collector will detect that and break the cycle so we don't leak.

If this ends up contributing to jank, for example (due to creating large cycle collection graphs), then there may be a way to make a KeyframeEffectReadOnly not keep its Animation alive by duplicating state and introducing an observer-like mechanism, but currently it's much simpler to have these two objects keep each other alive.

Comment 7

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles, away 2-10 April) from comment #6)
> We shouldn't be leaking. A KeyframeEffectReadOnly needs to keep its
> Animation alive and vice versa. If they are the only things keeping each
> other alive then the cycle collector will detect that and break the cycle so
> we don't leak.

Good, so we can let the AnimationEffectTimingReadOnly::mParent strongly point to the object who also strongly points to this AnimationEffectTimingReadOnly object. Therefore, I think making AnimationEffectTimingReadOnly::mParent point to the KeyframeEffect(ReadOnly) object directly is the simplest way to fix this.

Comment 8

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles, away 2-10 April) from comment #6)
> We shouldn't be leaking. A KeyframeEffectReadOnly needs to keep its
> Animation alive and vice versa. If they are the only things keeping each
> other alive then the cycle collector will detect that and break the cycle so
> we don't leak.

Thanks, Brian. The reason I asked this is AnimationEffectTimingReadOnly::mParent is a strong pointer, and I think the simplest way for this bug is let the parent object point to the KeyframeEffect(ReadOnly) object. However, I still want to avoid the ref-counted cycle, so let AnimationEffectTimingReadOnly::mParent point to KeyframeEffect's parent object which is the document of its target. This might have problems if we set a new target element which is in a different document. In this case, we have to update the parent object, but we could leave this part in Bug 1067769.

Comment 9

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1249564 - Part 1: Assign the parent object of AnimationEffectTiming(ReadOnly). r?birtles, r?bz

Review commit: https://reviewboard.mozilla.org/r/45361/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45361/

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8739826 [details]
MozReview Request: Bug 1249564 - Part 1: Assign the parent object of AnimationEffectTiming(ReadOnly). r?birtles, r?bz

https://reviewboard.mozilla.org/r/45361/#review42237

r=birtles with comments addressed but would you mind asking bz to have a look since I'm not entirely confident I understand how this normally hangs together.

::: dom/animation/AnimationEffectTiming.h:21
(Diff revision 1)
> -    : AnimationEffectTimingReadOnly(aTiming)
> +    : AnimationEffectTimingReadOnly(aTiming,
> +                                    aEffect ? aEffect->GetParentObject()
> +                                            : nullptr)

Can aEffect ever be null? If not, why not just call aEffect->GetParentObject() and add an assertion that aEffect is not null?

::: dom/animation/AnimationEffectTiming.h:28
(Diff revision 1)
> -  void Unlink() override { mEffect = nullptr; }
> +  void Unlink() override
> +  {
> +    mParent = nullptr;
> +    mEffect = nullptr;
> +  }

Why not make mParent the responsibility of the superclass and call AnimationEffectTimingReadOnly::Unlink() here?

::: dom/animation/AnimationEffectTimingReadOnly.h:55
(Diff revision 1)
>    void GetEasing(nsString& aRetVal) const;
>  
>    const TimingParams& AsTimingParams() const { return mTiming; }
>    void SetTimingParams(const TimingParams& aTiming) { mTiming = aTiming; }
>  
> -  virtual void Unlink() { }
> +  virtual void Unlink() { mParent = nullptr; }

So, as I understand it, this is called when the associated KeyframeEffect is destroyed. But in that case, script can still keep the AnimationEffectTimingReadOnly object alive, right? In that situation, we should still be able to get the parent object, right?

So I think we shouldn't clear mParent here.

Comment 11

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/45361/#review42237

OK, I'd add bz as the reviewer. Thanks.

> Can aEffect ever be null? If not, why not just call aEffect->GetParentObject() and add an assertion that aEffect is not null?

We only new AnimationEffect in KeyframeEffect, and pass |this|, so it never be null. I would call aEffect->GetParentObject directly. Thanks.

> Why not make mParent the responsibility of the superclass and call AnimationEffectTimingReadOnly::Unlink() here?

Sure. But as you mention below, I shouldn't null mParent here.

> So, as I understand it, this is called when the associated KeyframeEffect is destroyed. But in that case, script can still keep the AnimationEffectTimingReadOnly object alive, right? In that situation, we should still be able to get the parent object, right?
> 
> So I think we shouldn't clear mParent here.

Actaully, I'm not sure which is the suitable parent object after we destroy the associtated KeyframeEffet. If the parent object is still the document of the target element, then we shouldn't null it.

Comment 12

[Boris Chiou [:boris]]

Comment on attachment 8739826 [details]
MozReview Request: Bug 1249564 - Part 1: Assign the parent object of AnimationEffectTiming(ReadOnly). r?birtles, r?bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45361/diff/1-2/

Comment 13

[Boris Chiou [:boris]]

Comment on attachment 8739826 [details]
MozReview Request: Bug 1249564 - Part 1: Assign the parent object of AnimationEffectTiming(ReadOnly). r?birtles, r?bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45361/diff/2-3/

Comment 14

[Boris Chiou [:boris]]

try: crashtest failed: dom/animation/test/crashtests/1244595-1.html

I should fix this before landing.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Actaully, I'm not sure which is the suitable parent object after we destroy the associtated KeyframeEffet.

The parent object should be a lifetime invariant for an object, except in a few cases of objects that can change which global they're associated with.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8739826 [details]
MozReview Request: Bug 1249564 - Part 1: Assign the parent object of AnimationEffectTiming(ReadOnly). r?birtles, r?bz

https://reviewboard.mozilla.org/r/45361/#review42341

::: dom/animation/AnimationEffectTiming.h:22
(Diff revision 3)
>  class AnimationEffectTiming : public AnimationEffectTimingReadOnly
>  {
>  public:
>    AnimationEffectTiming(const TimingParams& aTiming, KeyframeEffect* aEffect)
> -    : AnimationEffectTimingReadOnly(aTiming)
> -    , mEffect(aEffect) { }
> +    : AnimationEffectTimingReadOnly(aTiming, aEffect->GetParentObject())
> +    , mEffect(aEffect) { MOZ_ASSERT(aEffect, "null effect"); }

Should probably put the curlies and body on separate lines now that there is a nontrivial body.

::: dom/animation/AnimationEffectTimingReadOnly.h:26
(Diff revision 3)
>  
>  class AnimationEffectTimingReadOnly : public nsWrapperCache
>  {
>  public:
>    AnimationEffectTimingReadOnly() = default;
> -  explicit AnimationEffectTimingReadOnly(const TimingParams& aTiming)
> +  // Bug 1067769: We'd set the target who may be in the different document,

This comment doesn't make any sense to me.  The mParent should be effectively immutable and should lead to the global this object is associated with, which _is_ immutable.  The spec that defines this object needs to define what global that is (either explicitly or implicitly via whatever infrastructure it is Anne wants to put into webidl), and then you can implement the corresponding behavior.

Comment 17

[Boris Chiou [:boris]]

(In reply to Boris Zbarsky [:bz] from comment #16)
> This comment doesn't make any sense to me.  The mParent should be
> effectively immutable and should lead to the global this object is
> associated with, which _is_ immutable.  The spec that defines this object
> needs to define what global that is (either explicitly or implicitly via
> whatever infrastructure it is Anne wants to put into webidl), and then you
> can implement the corresponding behavior.

Hi, Brian

I think I'm totally wrong. The mParent object should be effectively immutable, so it shouldn't be changed after we set another target element which might be in different document. I checked the spec but I'm not sure which one AnimationEffectTiming(ReadOnly) should associated with in this case.

For another case, you mentioned in comment 10, the script can still keep the AnimationEffectTiming object alive, but the associated KeyframeEffect is destroyed. We can use the original document object as its mParent I think.

The spec doesn't say much about it (or do I miss something?), so do you have any advice for the global of AnimationEffectTiming? Or the original document object is enough? Or this is an exception what comment 15 mentioned, so it could be changed.

Thanks.

Comment 18

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13070a448f55

Comment 19

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/45361/#review42237

> We only new AnimationEffect in KeyframeEffect, and pass |this|, so it never be null. I would call aEffect->GetParentObject directly. Thanks.

Sorry, this patch causes a crash.

I cannot call aEffect->GetParentObject() directly because aEffect is under construction here. (We pass |this| as aEffect in the initializaion list of KeyframeEffect::KeyframeEffect()), so we have to pass the parent object directly.

BTW, KeyframeEffectReadOnly::mTiming is _not_ cycle-collected now... Therefore, we will have some leaks if I set the mParent object to the current documnet or window object. It doesn't make any sense to nullify an OwningNonNull<..> object, so I prefer not to implement ImplCycleCollectionUnlink() for it. The alernative way is to use RefPtr<> for mTiming.

Comment 20

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris]  from comment #17)
> I think I'm totally wrong. The mParent object should be effectively
> immutable, so it shouldn't be changed after we set another target element
> which might be in different document.

I'm thinking about the possibility of setting a new target element in a different document. If I'm wrong, I think using the current document or window as the parent is enough.

Comment 21

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/45361/#review42341

> Should probably put the curlies and body on separate lines now that there is a nontrivial body.

Thanks. I will remove this body.

> This comment doesn't make any sense to me.  The mParent should be effectively immutable and should lead to the global this object is associated with, which _is_ immutable.  The spec that defines this object needs to define what global that is (either explicitly or implicitly via whatever infrastructure it is Anne wants to put into webidl), and then you can implement the corresponding behavior.

You're right. I will drop the comment and make sure mParent is immutable.

Comment 22

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Boris Chiou [:boris]  from comment #17)
> (In reply to Boris Zbarsky [:bz] from comment #16)
> > This comment doesn't make any sense to me.  The mParent should be
> > effectively immutable and should lead to the global this object is
> > associated with, which _is_ immutable.  The spec that defines this object
> > needs to define what global that is (either explicitly or implicitly via
> > whatever infrastructure it is Anne wants to put into webidl), and then you
> > can implement the corresponding behavior.
> 
> Hi, Brian
> 
> I think I'm totally wrong. The mParent object should be effectively
> immutable, so it shouldn't be changed after we set another target element
> which might be in different document. I checked the spec but I'm not sure
> which one AnimationEffectTiming(ReadOnly) should associated with in this
> case.

Right, but you're not changing it, right? That's why I suggested Unlink should not clear mParent.

> For another case, you mentioned in comment 10, the script can still keep the
> AnimationEffectTiming object alive, but the associated KeyframeEffect is
> destroyed. We can use the original document object as its mParent I think.

Yes.

> The spec doesn't say much about it (or do I miss something?), so do you have
> any advice for the global of AnimationEffectTiming? Or the original document
> object is enough? Or this is an exception what comment 15 mentioned, so it
> could be changed.

I don't understand all the subtle possibilities here, but I think we're ok to keep using the original document here.

The spec probably needs to be updated to specify this in a number of places but I'll wait to see what Anne adds before going ahead with that.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Not clearing things in Unlink sounds like asking for a leak to me...

Comment 24

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1249564 - Part 2: Cycle collect AnimationEffectTimingReadOnly. r?birtles

Add KeyframeEffectReadOnly::mTiming into the list of cycle collection to
avoid any possible memory leak.

Review commit: https://reviewboard.mozilla.org/r/46069/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46069/

Comment 25

[Boris Chiou [:boris]]

Comment on attachment 8739826 [details]
MozReview Request: Bug 1249564 - Part 1: Assign the parent object of AnimationEffectTiming(ReadOnly). r?birtles, r?bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45361/diff/3-4/

Comment 26

[Boris Chiou [:boris]]

Comment on attachment 8739826 [details]
MozReview Request: Bug 1249564 - Part 1: Assign the parent object of AnimationEffectTiming(ReadOnly). r?birtles, r?bz

Sorry, Brian. I think you'd like to check this again, so I change the status to r?.

Comment 27

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Boris Zbarsky [:bz] from comment #23)
> Not clearing things in Unlink sounds like asking for a leak to me...

This isn't cycle-collector unlinking. It's what the associated KeyframeEffect object uses to tell the AnimationEffectTiming object that it's going away since the AnimationEffectTiming object only has a non-owning reference to the KeyframeEffect. At that point the AnimationEffectTiming object should clear its mEffect pointer but it still holds a reference to its mParent (which *is* included in cycle collection and cleared in the Unlink generated from NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE). Maybe we should rename the method.

Comment 28

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8739826 [details]
MozReview Request: Bug 1249564 - Part 1: Assign the parent object of AnimationEffectTiming(ReadOnly). r?birtles, r?bz

https://reviewboard.mozilla.org/r/45361/#review42875

Comment 29

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8741227 [details]
MozReview Request: Bug 1249564 - Part 2: Cycle collect AnimationEffectTimingReadOnly. r?birtles

https://reviewboard.mozilla.org/r/46069/#review42881

Comment 30

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ba0e38d5717

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8739826 [details]
MozReview Request: Bug 1249564 - Part 1: Assign the parent object of AnimationEffectTiming(ReadOnly). r?birtles, r?bz

https://reviewboard.mozilla.org/r/45361/#review43057

r=me

Comment 32

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56f5ef4511bb

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f1cef92aec4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b0f265bea5

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f1cef92aec4
https://hg.mozilla.org/mozilla-central/rev/d2b0f265bea5