From 86fdc8655de6f934c2b338cf85af315c7997411d Mon Sep 17 00:00:00 2001
From: Daniel Holbert <dholbert@cs.stanford.edu>
Date: Thu, 23 Mar 2023 04:41:12 +0000
Subject: [PATCH] Bug 1822907 part 2: When handling a content-visibility
 change, don't insert already-completed animations into the timeline's
 sampling-order list. r=hiro

We have an invariant that the mAnimationOrder LinkedList is a subset of the
mAnimations hashset (omitting any animations that are hidden due to
content-visibility).  This patch corrects one case where we were incorrectly
inserting an animation into the linked list when it wasn't present in the
hashset (because the animation had completed).

This patch also adds some documentation to mention this invariant, and some
assertions to enforce it in several places.

Differential Revision: https://phabricator.services.mozilla.com/D173333
---
 dom/animation/AnimationTimeline.cpp           |  8 +++-
 dom/animation/AnimationTimeline.h             |  1 +
 ...nt-visibility-transition-finished-001.html | 43 +++++++++++++++++++
 3 files changed, 51 insertions(+), 1 deletion(-)
 create mode 100644 testing/web-platform/tests/css/css-contain/content-visibility/crashtests/content-visibility-transition-finished-001.html

diff --git a/dom/animation/AnimationTimeline.cpp b/dom/animation/AnimationTimeline.cpp
index 5d6dde51b301..8be055468357 100644
--- a/dom/animation/AnimationTimeline.cpp
+++ b/dom/animation/AnimationTimeline.cpp
@@ -46,6 +46,8 @@ bool AnimationTimeline::Tick() {
   for (Animation* animation = mAnimationOrder.getFirst(); animation;
        animation =
            static_cast<LinkedListElement<Animation>*>(animation)->getNext()) {
+    MOZ_ASSERT(mAnimations.Contains(animation),
+               "The sampling order list should be a subset of the hashset");
     MOZ_ASSERT(!animation->IsHiddenByContentVisibility(),
                "The sampling order list should not contain any animations "
                "that are hidden by content-visibility");
@@ -91,6 +93,8 @@ void AnimationTimeline::NotifyAnimationUpdated(Animation& aAnimation) {
 void AnimationTimeline::RemoveAnimation(Animation* aAnimation) {
   MOZ_ASSERT(!aAnimation->GetTimeline() || aAnimation->GetTimeline() == this);
   if (static_cast<LinkedListElement<Animation>*>(aAnimation)->isInList()) {
+    MOZ_ASSERT(mAnimations.Contains(aAnimation),
+               "The sampling order list should be a subset of the hashset");
     static_cast<LinkedListElement<Animation>*>(aAnimation)->remove();
   }
   mAnimations.Remove(aAnimation);
@@ -100,7 +104,9 @@ void AnimationTimeline::NotifyAnimationContentVisibilityChanged(
     Animation* aAnimation, bool aIsVisible) {
   bool inList =
       static_cast<LinkedListElement<Animation>*>(aAnimation)->isInList();
-  if (aIsVisible && !inList) {
+  MOZ_ASSERT(!inList || mAnimations.Contains(aAnimation),
+             "The sampling order list should be a subset of the hashset");
+  if (aIsVisible && !inList && mAnimations.Contains(aAnimation)) {
     mAnimationOrder.insertBack(aAnimation);
   } else if (!aIsVisible && inList) {
     static_cast<LinkedListElement<Animation>*>(aAnimation)->remove();
diff --git a/dom/animation/AnimationTimeline.h b/dom/animation/AnimationTimeline.h
index a8c876590dd9..48a9d769cfaf 100644
--- a/dom/animation/AnimationTimeline.h
+++ b/dom/animation/AnimationTimeline.h
@@ -125,6 +125,7 @@ class AnimationTimeline : public nsISupports, public nsWrapperCache {
   // We store them in (a) a hashset for quick lookup, and (b) a LinkedList
   // to maintain a fixed sampling order. Animations that are hidden by
   // `content-visibility` are not sampled and will only be in the hashset.
+  // The LinkedList should always be a subset of the hashset.
   //
   // The hashset keeps a strong reference to each animation since
   // dealing with addref/release with LinkedList is difficult.
diff --git a/testing/web-platform/tests/css/css-contain/content-visibility/crashtests/content-visibility-transition-finished-001.html b/testing/web-platform/tests/css/css-contain/content-visibility/crashtests/content-visibility-transition-finished-001.html
new file mode 100644
index 000000000000..ef7fb001ed86
--- /dev/null
+++ b/testing/web-platform/tests/css/css-contain/content-visibility/crashtests/content-visibility-transition-finished-001.html
@@ -0,0 +1,43 @@
+<!DOCTYPE html>
+<html class="test-wait">
+<meta charset="utf-8">
+<link rel="author" title="Daniel Holbert" href="mailto:dholbert@mozilla.com">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1822907">
+<style>
+  #inner {
+    /* Extremely short so that we can just do a double-rAF and
+     * expect that this transition will have completed: */
+    transition: opacity 0.01s;
+  }
+</style>
+<script>
+  function setup() {
+    // Flush style, in case the document hasn't been styled yet:
+    let origOpacity = getComputedStyle(inner).opacity;
+
+    // Trigger the (extremely short) transition, and proceed to next step
+    // when it finishes.
+    inner.addEventListener("transitionend", handleTransitionEnd);
+    inner.style.opacity = "0.8";
+  }
+  function handleTransitionEnd(e) {
+    // Hide & unhide the subtree that contained the transition:
+    foo.style.contentVisibility = "hidden";
+    document.documentElement.offsetHeight; // flush layout
+    foo.style.contentVisibility = "";
+
+    // Double-rAF to see if we crash when animations are resampled.
+    requestAnimationFrame(() => { requestAnimationFrame(finish) });
+  }
+  function finish() {
+    // If we get here, we've succeeded; hooray!
+    document.documentElement.removeAttribute("class");
+  }
+</script>
+<body onload="setup()">
+  <div id="foo">
+    Hello
+    <div id="inner">Inner</div>
+  </div>
+</body>
+</html>