1195523 - Switch AnimationCommon::mElementCollections & AnimationCollection struct to be LinkedList<>-based, not PRCList-based

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Comment 1

[Daniel Holbert [:dholbert]]

[d'oh, hit enter while typing bug summary]

Ever since nsTransitionManager was introduced, ages ago, we've used a PRClist to manage a list of transitions/animations. (nowadays in AnimationCommon::mElementCollections).

This requires ugly boilerplate macros and static_casts to initialize/clean-up/iterate over the list.

Nowadays, we have the MFBT templated LinkedList class. We should use that here instead, for cleaner more readable code.  (Like PRCList, it's a doubly-linked list. It's not circular, but I'm pretty sure we don't actually care about having this particular list be circular.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: strawman

Here's a strawman patch -- just manually ported all of the PRCList stuff to LinkedList & got it to compile.  Firefox starts fine, and a few animations in the UI played just fine too. Haven't sanity-checked this beyond that yet.

Try run: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Ever since nsTransitionManager was introduced, ages ago, we've used a
> PRClist to manage a list of transitions/animations.

For reference, that was in this cset from 2009:
 http://hg.mozilla.org/mozilla-central/rev/ff00a422b6f1#l24.153

> (nowadays in AnimationCommon::mElementCollections).

This was introduced in this cset from 2011:
 http://hg.mozilla.org/mozilla-central/rev/548241dd0c12#l2.151

And then the MFBT LinkedList class was added in 2012, in bug 715405.)

(This ^^ backstory is the answer to the question that prompted me to file this bug: "why are we using PRCList instead of e.g. LinkedList here?")

Comment 4

[Daniel Holbert [:dholbert]]

(My Try run hit some issues from something observing the refreshdriver longer than it should. Hopefully that's not too tough to investigate/solve though.)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8648977 [details] [diff] [review]
strawman

>   ~AnimationCollection()
>   {
>     MOZ_ASSERT(mCalledPropertyDtor,
>                "must call destructor through element property dtor");
>     MOZ_COUNT_DTOR(AnimationCollection);
>-    PR_REMOVE_LINK(this);
>     mManager->ElementCollectionRemoved();
>   }

Brian correctly guessed that the problem is here ^^.  AnimationCollection's superclass (LinkedListElement) will remove us from the list in its destructor, but we need to be removed *before* the call to ElementCollectionRemoved here, or else we're left with the manager (or something) observing the refresh driver.

Local test runs seem to get past the Try run's issues, with that changed.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Try run: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound

Er, clearly I copypasted the URL from the wrong treeherder tab here ^^^. :)  I meant to post this link, for my first (broken) Try run:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b4febb2e621

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8648977 [details] [diff] [review]
strawman

Fixed 2 more issues in initial patch:

>-  PR_INSERT_BEFORE(aCollection, &mElementCollections);
>+  mElementCollections.insertFront(aCollection);

I'd mistakenly read INSERT_BEFORE...mElementCollections as meaning "insert at the front of the list" here. But mElementCollections is actually a pointer to the end of the list, so this is insertion at the end. So, I've changed this locally to insertBack, which fixed some mochitest failures.

> CommonAnimationManager::RemoveAllElementCollections()
> {
>-  while (!PR_CLIST_IS_EMPTY(&mElementCollections)) {
>-    AnimationCollection* head =
>-      static_cast<AnimationCollection*>(PR_LIST_HEAD(&mElementCollections));
>-    head->Destroy();
>-  }
>+ while (AnimationCollection* head = mElementCollections.popFirst()) {
>+   head->Destroy();
>+ }

My "popFirst" usage here slightly tweaked the semantics (it ended up removing the AnimationCollection from mElementCollections, *before* we hit the AnimationCollection destructor, instead of waiting for it to be removed *in* its destructor).

I've changed this locally to use getFirst instead of popFirst, so that I'm not changing the order in which things happen.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Comment 9

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8cfb70924ce

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 8651226 [details] [diff] [review]
fix v2

>diff --git a/layout/style/AnimationCommon.cpp b/layout/style/AnimationCommon.cpp
>--- a/layout/style/AnimationCommon.cpp
>+++ b/layout/style/AnimationCommon.cpp
>@@ -77,27 +76,25 @@ CommonAnimationManager::AddElementCollec
>   if (!mIsObservingRefreshDriver) {
>     NS_ASSERTION(aCollection->mNeedsRefreshes,
>       "Added data which doesn't need refreshing?");
>     // We need to observe the refresh driver.
>     mPresContext->RefreshDriver()->AddRefreshObserver(this, Flush_Style);
>     mIsObservingRefreshDriver = true;
>   }
> 
>-  PR_INSERT_BEFORE(aCollection, &mElementCollections);
>+  mElementCollections.insertBack(aCollection);
> }

If I understand this right, the old code would leave us with:

  aCollection -> mElementCollections

But this new code will leave us with:

  mElementCollections -> aCollection

I guess that will effect things like the order we dispatch events and mutation observer records. Given that the current order isn't really sensible in anyway (it's based on when an element newly transitioned from having zero animations to 1 or more animations, I believe), this is probably ok. I'm currently working on both these areas to make us dispatch these things in a more sensible order.
 
So, looks good!

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Brian Birtles (:birtles, busy 22~30 Aug) from comment #10)
> If I understand this right, the old code would leave us with:
> 
>   aCollection -> mElementCollections
> 
> But this new code will leave us with:
> 
>   mElementCollections -> aCollection

If you want to change the order, that's probably best done in a separate patch for regression-hunting, though.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles, busy 22~30 Aug) from comment #10)
> If I understand this right, the old code would leave us with:
> 
>   aCollection -> mElementCollections
> 
> But this new code will leave us with:
> 
>   mElementCollections -> aCollection

I'm not sure I follow.

I think in the old code, mElementCollections is just a sentinel/dummy node at the end of the list -- not a node with useful data. Note that in our iteration, we iterate until " l != &mElementCollections;" -- i.e. we intentionally never visit that node.

So I don't think this patch is actually changing behavior -- it's still appending at the end of the list. In the old code, you do that by inserting before the sentinel node. In the new code, you do that with .insertBack().

Birtles, does this make sense, and can you clarify if you think I'm misunderstanding something? I agree with dbaron's sentiment - I very much do not want to change behavior in this refactoring patch, even in a way that seems correct.

Comment 13

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] (vacation 8/27-9/13) from comment #12)
> (In reply to Brian Birtles (:birtles, busy 22~30 Aug) from comment #10)
> > If I understand this right, the old code would leave us with:
> > 
> >   aCollection -> mElementCollections
> > 
> > But this new code will leave us with:
> > 
> >   mElementCollections -> aCollection
> 
> I'm not sure I follow.
> 
> I think in the old code, mElementCollections is just a sentinel/dummy node
> at the end of the list -- not a node with useful data. Note that in our
> iteration, we iterate until " l != &mElementCollections;" -- i.e. we
> intentionally never visit that node.
> 
> So I don't think this patch is actually changing behavior -- it's still
> appending at the end of the list. In the old code, you do that by inserting
> before the sentinel node. In the new code, you do that with .insertBack().

Yes, you're right. I checked this over again and I agree it should be fine as you have it. Sorry for the confusion.

Comment 14

[Daniel Holbert [:dholbert]]

No worries - thanks for double-checking & for the review! Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/793be272a336

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/793be272a336