AnimationEffectTiming:GetParentObject should return a valid object

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM: Animation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hiro, Assigned: boris)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox47 affected, firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Currently nobody sets mParent in the class.
(Assignee)

Updated

2 years ago
Assignee: nobody → boris.chiou
(Assignee)

Comment 1

2 years ago
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?
(Assignee)

Comment 2

2 years ago
(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 hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8737073 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8737080 - Attachment is obsolete: true
(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 hidden (obsolete)
(Assignee)

Comment 8

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8737089 - Attachment is obsolete: true
Comment hidden (obsolete)
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.
Attachment #8739826 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 11

2 years ago
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 hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

2 years ago
Attachment #8739826 - Flags: review?(bzbarsky)
(Assignee)

Comment 14

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

I should fix this before landing.
> 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 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.
Attachment #8739826 - Flags: review?(bzbarsky)
(Assignee)

Comment 17

2 years ago
(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.
Flags: needinfo?(bbirtles)
(Assignee)

Comment 19

2 years ago
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.
(Assignee)

Comment 20

2 years ago
(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.
(Assignee)

Comment 21

2 years ago
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.
(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.
Flags: needinfo?(bbirtles)
Not clearing things in Unlink sounds like asking for a leak to me...
(Assignee)

Comment 24

2 years ago
Created attachment 8741227 [details]
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/
Attachment #8739826 - Attachment description: MozReview Request: Bug 1249564 - Assign the parent object of AnimationEffectTiming(ReadOnly) → MozReview Request: Bug 1249564 - Part 1: Assign the parent object of AnimationEffectTiming(ReadOnly). r?birtles, r?bz
Attachment #8741227 - Flags: review?(bbirtles)
Attachment #8739826 - Flags: review?(bzbarsky)
(Assignee)

Comment 25

2 years ago
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/
(Assignee)

Comment 26

2 years ago
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?.
Attachment #8739826 - Flags: review+ → review?(bbirtles)
(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 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
Attachment #8739826 - Flags: review?(bbirtles) → review+
Comment on attachment 8741227 [details]
MozReview Request: Bug 1249564 - Part 2: Cycle collect AnimationEffectTimingReadOnly. r?birtles

https://reviewboard.mozilla.org/r/46069/#review42881
Attachment #8741227 - Flags: review?(bbirtles) → review+
Attachment #8739826 - Flags: review?(bzbarsky) → review+
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 34

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4f1cef92aec4
https://hg.mozilla.org/mozilla-central/rev/d2b0f265bea5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1283625
You need to log in before you can comment on or make changes to this bug.