AnimationTimeline should return the same parent object even after document.open(), or be reparented

RESOLVED FIXED in mozilla38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: birtles, Assigned: jwatt)

Tracking

Trunk
mozilla38
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Bug 1104435 comment 5 notes that AnimationTimeline's implementation of GetParentObject returns mDocument->GetParentObject which will return a different object after a call to document.open().
(Reporter)

Comment 1

4 years ago
Also, we should address bug 1104435 comment 6 here too.

Comment 2

4 years ago
So either we should reparent AnimationTimeline during document.open() (similar to Document) or always just have the parent object pointing to the original global or something.

Updated

4 years ago
Summary: AnimationTimeline should return the same parent object even after document.open() → AnimationTimeline should return the same parent object even after document.open(), or be reparented
(Assignee)

Comment 3

4 years ago
Step 15 in https://html.spec.whatwg.org/multipage/webappapis.html#dom-document-open would suggest that maybe the document's timeline (document.timeline) should be replaced. This algorithm basically seems to reset (in a loose sence) the document, so I think that makes sense. As far as I can see that would mean clearing mAnimationTimeline in nsDocument::ResetToURI(), possibly after pausing it to make sure that it's no longer using cycles?
(Assignee)

Comment 4

4 years ago
(In reply to Olli Pettay [:smaug] from comment #2)
> So either we should reparent AnimationTimeline during document.open()

I'm guessing reparenting all the AnimationTimelines and AnimationPlayers in a document would require bookkeeping that we'd rather not have. How is that different to:

  nsIGlobalObject* GetParentObject() const {
    return mDocument->GetParentObject();
  }

anyway? Is there something that requires different timing of the switch to the new nsIGlobalObject*?
The basic thing GetParentObject controls is which prototype object is used for creating the JS wrapper.  If we're not consistent about it and don't reparent, we get issues like GC causing a different prototype to be used.

Comment 6

4 years ago
(In reply to Jonathan Watt [:jwatt] from comment #3)
> Step 15 in
> https://html.spec.whatwg.org/multipage/webappapis.html#dom-document-open
> would suggest that maybe the document's timeline (document.timeline) should
> be replaced.
That sounds like one option, not necessarily a bad one.
(Assignee)

Comment 7

4 years ago
Created attachment 8537863 [details] [diff] [review]
patch
Assignee: nobody → jwatt
Attachment #8537863 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #8537863 - Flags: review?(bbirtles)
(Assignee)

Comment 8

4 years ago
Note that this only makes sense after bug 1104435 part 1 ( https://bugzilla.mozilla.org/attachment.cgi?id=8536258&action=diff ) which changes AnimationTimeline::GetParentObject to return nsIGlobalObject*.

Comment 9

4 years ago
Comment on attachment 8537863 [details] [diff] [review]
patch

I would make mParentObject 
nsCOMPtr<nsIGlobalObject> mParentObject and add that to cycle collection.
That would be closer to what we have elsewhere.

nsDocument::ResetToURI change isn't right. That method is called also in some
other cases than in document.open() and mAnimationTimeline shouldn't be reset in those cases, I think.
Attachment #8537863 - Flags: review?(bugs) → review-
(Reporter)

Comment 10

4 years ago
Comment on attachment 8537863 [details] [diff] [review]
patch

smaug knows much better than I do about the specifics here but the general approach of keeping the old timeline around looks fine to me.

In the case where the global disappears, any players still pointing to that timeline will no longer be able to create Promise objects. Does that seem reasonable?

Also, why is the weak pointer necessary?

Assuming you can find the appropriate place to clear the timeline and the weak point is necessary this seems fine to me.
Attachment #8537863 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 11

4 years ago
(In reply to Olli Pettay [:smaug] from comment #9)
> I would make mParentObject 
> nsCOMPtr<nsIGlobalObject> mParentObject and add that to cycle collection.
> That would be closer to what we have elsewhere.

Interesting. Have we given up on trying avoid creating cycles and freeing things as soon as they can be? Anyway, done.

> nsDocument::ResetToURI change isn't right. That method is called also in some
> other cases than in document.open() and mAnimationTimeline shouldn't be
> reset in those cases, I think.

Ah, yeah, CloneDocHelper looks problematic. I've moved it to Reset() instead which looks okay (called on document load and some other things, but at those times resetting the timeline seems okay since it's created on-demand and won't have been created yet).

(In reply to Brian Birtles (:birtles) from comment #10)
> In the case where the global disappears, any players still pointing to that
> timeline will no longer be able to create Promise objects. Does that seem
> reasonable?

I'm not sure when we'd want animations to keep running if the window goes away. I'd say it seems reasonable until someone asks us to support that. :)

> Also, why is the weak pointer necessary?

Just trying to free things when they no longer need to be in memory. (There's a strong ref chain through from the window, to doc, to timeline, so having a strong ref from the timeline back to the window creates a cycle. Anyway, if Olli and the DOM guys have decided that's okay nowadays, I defer to them.)
(Assignee)

Comment 12

4 years ago
Created attachment 8555856 [details] [diff] [review]
patch [r=birtles]
Attachment #8537863 - Attachment is obsolete: true
Attachment #8555856 - Flags: review?(bugs)
(In reply to Jonathan Watt [:jwatt] from comment #11)
> (In reply to Olli Pettay [:smaug] from comment #9)
> > I would make mParentObject 
> > nsCOMPtr<nsIGlobalObject> mParentObject and add that to cycle collection.
> > That would be closer to what we have elsewhere.
> 
> Interesting. Have we given up on trying avoid creating cycles and freeing
> things as soon as they can be? Anyway, done.
Depends.
DOMEventTargetHelper doesn't keep a strong ref, since XHR etc. objects inherit it and necko (which is outside the world cycle collector sees) tends to keep such objects alive too long.

If we need to optimize AnimationTimeline out from CC graph, we could make the object skippable.
Though, looks like there is already strong mDocument, so adding strong mWindow shouldn't be that bad.

Updated

4 years ago
Attachment #8555856 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/76c4ed3a6c8f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.