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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Brian Birtles (:birtles)]

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().

Comment 1

[Brian Birtles (:birtles)]

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

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 3

[Jonathan Watt [:jwatt]]

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?

Comment 4

[Jonathan Watt [:jwatt]]

(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*?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 8

[Jonathan Watt [:jwatt]]

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

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[Brian Birtles (:birtles)]

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.

Comment 11

[Jonathan Watt [:jwatt]]

(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.)

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: patch [r=birtles]

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 8555856 [details] [diff] [review]
patch [r=birtles]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76c4ed3a6c8f

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/76c4ed3a6c8f