Bug 1974382: Avoid mixing anchors in different child lists. r=layout-anchor-positioning-reviewers,dshin

Anchors with identical names are organized in tree order.
During restyling, the tree undergoes restructuring and the ordering of the frames may go wrong.
With this patch, the adjustments during restyling are recorded and applied only after the tree is ready.

Differential Revision: https://phabricator.services.mozilla.com/D255502

Author: jjjalkanen

layout/base/PresShell.cpp

@@ -1789,6 +1789,8 @@ nsresult PresShell::Initialize() {
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
+  mPresContext->PresShell()->AssertNoAnchorPosAnchorChanges();
+
   if (Element* root = mDocument->GetRootElement()) {
     {
       nsAutoCauseReflowNotifier reflowNotifier(this);
@@ -1863,6 +1865,8 @@ nsresult PresShell::Initialize() {
     mShouldUnsuppressPainting = true;
   }
 
+  mPresContext->PresShell()->MergeAnchorPosAnchorChanges();
+
   return NS_OK;  // XXX this needs to be real. MMP
 }
 
@@ -4519,7 +4523,6 @@ void PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush aFlush) {
 
   // Per our API contract, hold a strong ref to ourselves until we return.
   RefPtr<PresShell> kungFuDeathGrip = this;
-
   /**
    * VERY IMPORTANT: If you add some sort of new flushing to this
    * method, make sure to add the relevant SetNeedLayoutFlush or
@@ -4840,6 +4843,7 @@ MOZ_CAN_RUN_SCRIPT_BOUNDARY void PresShell::ContentWillBeRemoved(
   // Notify the ESM that the content has been removed, so that
   // it can clean up any state related to the content.
 
+  AssertNoAnchorPosAnchorChanges();
   mPresContext->EventStateManager()->ContentRemoved(mDocument, aChild);
 
   nsAutoCauseReflowNotifier crNotifier(this);
@@ -4859,6 +4863,7 @@ MOZ_CAN_RUN_SCRIPT_BOUNDARY void PresShell::ContentWillBeRemoved(
   // display: contents / display: none, because we'd have cleared all the style
   // data from there.
   mPresContext->RestyleManager()->ContentWillBeRemoved(aChild);
+  MergeAnchorPosAnchorChanges();
 }
 
 void PresShell::NotifyCounterStylesAreDirty() {
@@ -10986,6 +10991,7 @@ bool PresShell::ProcessReflowCommands(bool aInterruptible) {
       printf("ProcessReflowCommands: begin incremental reflow\n");
     }
 #endif
+    AssertNoAnchorPosAnchorChanges();
 
     // If reflow is interruptible, then make a note of our deadline.
     const PRIntervalTime deadline =
@@ -11019,6 +11025,8 @@ bool PresShell::ProcessReflowCommands(bool aInterruptible) {
     } while (!interrupted && !mDirtyRoots.IsEmpty() &&
              (!aInterruptible || PR_IntervalNow() < deadline));
 
+    MergeAnchorPosAnchorChanges();
+
     interrupted = !mDirtyRoots.IsEmpty();
 
     overflowTracker.Flush();
@@ -12017,6 +12025,8 @@ nsIFrame* PresShell::GetAbsoluteContainingBlock(nsIFrame* aFrame) {
 nsIFrame* PresShell::GetAnchorPosAnchor(
     const nsAtom* aName, const nsIFrame* aPositionedFrame) const {
   MOZ_ASSERT(aName);
+  MOZ_ASSERT(mLazyAnchorPosAnchorChanges.IsEmpty());
+
   if (const auto& entry = mAnchorPosAnchors.Lookup(aName)) {
     return AnchorPositioningUtils::FindFirstAcceptableAnchor(aPositionedFrame,
                                                              entry.Data());
@@ -12025,41 +12035,10 @@ nsIFrame* PresShell::GetAnchorPosAnchor(
   return nullptr;
 }
 
-void PresShell::AddAnchorPosAnchor(const nsAtom* aName, nsIFrame* aFrame) {
-  MOZ_ASSERT(aName);
-
-  auto& entry = mAnchorPosAnchors.LookupOrInsertWith(
-      aName, []() { return nsTArray<nsIFrame*>(); });
-
-  if (entry.IsEmpty()) {
-    entry.AppendElement(aFrame);
-    return;
-  }
-
-  struct FrameTreeComparator {
-    nsIFrame* mFrame;
-
-    int32_t operator()(nsIFrame* aOther) const {
-      return nsLayoutUtils::CompareTreePosition(mFrame, aOther, nullptr);
-    }
-  };
-
-  FrameTreeComparator cmp{aFrame};
-
-  size_t matchOrInsertionIdx = entry.Length();
-  // If the same element is already in the array,
-  // someone forgot to call RemoveAnchorPosAnchor.
-  if (BinarySearchIf(entry, 0, entry.Length(), cmp, &matchOrInsertionIdx)) {
-    MOZ_ASSERT_UNREACHABLE("Anchor added already");
-    return;
-  }
-
-  *entry.InsertElementAt(matchOrInsertionIdx) = aFrame;
-}
-
-void PresShell::RemoveAnchorPosAnchor(const nsAtom* aName, nsIFrame* aFrame) {
-  MOZ_ASSERT(aName);
-  auto entry = mAnchorPosAnchors.Lookup(aName);
+static void RemoveAnchorPosAnchorFromOrderedCache(
+    const RefPtr<const nsAtom>& aName, nsIFrame* aFrame,
+    nsTHashMap<RefPtr<const nsAtom>, nsTArray<nsIFrame*>>& aAnchorPosAnchors) {
+  auto entry = aAnchorPosAnchors.Lookup(aName);
   if (!entry) {
     return;  // Nothing to remove.
   }
@@ -12069,13 +12048,67 @@ void PresShell::RemoveAnchorPosAnchor(const nsAtom* aName, nsIFrame* aFrame) {
   // XXX: Once the implementation is more complete,
   // we should probably assert here that anchorArray
   // is not empty and aFrame is in it.
+  MOZ_ASSERT(!anchorArray.IsEmpty());
 
   anchorArray.RemoveElement(aFrame);
   if (anchorArray.IsEmpty()) {
     entry.Remove();
   }
 }
 
+void PresShell::RemoveAnchorPosAnchorNow(const RefPtr<const nsAtom>& aName,
+                                         nsIFrame* aFrame) {
+  // nsCSSFrameConstructor will destroy, remove and add frames back,
+  // in no particular order during reflow. Removing the elements from
+  // mLazyAnchorPosAnchorChanges here is necessary and for performance
+  // reasons it would be an advantage to not let mLazyAnchorPosAnchorChanges
+  // grow too much.
+  mLazyAnchorPosAnchorChanges.RemoveElementsBy(
+      [&](const AnchorPosAnchorChange& change) {
+        return change.mFrame == aFrame;
+      });
+
+  RemoveAnchorPosAnchorFromOrderedCache(aName, aFrame, mAnchorPosAnchors);
+}
+
+void PresShell::MergeAnchorPosAnchorChanges() {
+  for (const auto& [name, frame, isRemoval] : mLazyAnchorPosAnchorChanges) {
+    if (isRemoval) {
+      RemoveAnchorPosAnchorFromOrderedCache(name, frame, mAnchorPosAnchors);
+    } else {
+      auto& entry = mAnchorPosAnchors.LookupOrInsertWith(
+          name, []() { return nsTArray<nsIFrame*>(); });
+
+      if (entry.IsEmpty()) {
+        entry.AppendElement(frame);
+        continue;
+      }
+
+      struct FrameTreeComparator {
+        nsIFrame* mFrame;
+
+        int32_t operator()(nsIFrame* aOther) const {
+          return nsLayoutUtils::CompareTreePosition(mFrame, aOther, nullptr);
+        }
+      };
+
+      FrameTreeComparator cmp{frame};
+
+      size_t matchOrInsertionIdx = entry.Length();
+      // If the same element is already in the array,
+      // someone forgot to call RemoveAnchorPosAnchor.
+      if (BinarySearchIf(entry, 0, entry.Length(), cmp, &matchOrInsertionIdx)) {
+        MOZ_ASSERT_UNREACHABLE("Anchor added already");
+        continue;
+      }
+
+      *entry.InsertElementAt(matchOrInsertionIdx) = frame;
+    }
+  }
+
+  mLazyAnchorPosAnchorChanges.Clear();
+}
+
 void PresShell::ActivenessMaybeChanged() {
   if (!mDocument) {
     return;

layout/base/PresShell.h

@@ -751,8 +751,34 @@ class PresShell final : public nsStubDocumentObserver,
   // https://drafts.csswg.org/css-anchor-position-1/#target
   nsIFrame* GetAnchorPosAnchor(const nsAtom* aName,
                                const nsIFrame* aPositionedFrame) const;
-  void AddAnchorPosAnchor(const nsAtom* aName, nsIFrame* aFrame);
-  void RemoveAnchorPosAnchor(const nsAtom* aName, nsIFrame* aFrame);
+
+  /**
+   * When frame tree is reconstructed, we need to avoid ordering the frames
+   * while they may not be part of the same child list.
+   */
+  inline void AddAnchorPosAnchor(const nsAtom* aName, nsIFrame* aFrame) {
+    MOZ_ASSERT(aName);
+    mLazyAnchorPosAnchorChanges.AppendElement(
+        AnchorPosAnchorChange{RefPtr<const nsAtom>(aName), aFrame, false});
+  }
+
+  /**
+   * When frame tree is reconstructed, we need to avoid ordering the frames
+   * while they may not be part of the same child list.
+   */
+  inline void RemoveAnchorPosAnchor(const nsAtom* aName, nsIFrame* aFrame) {
+    MOZ_ASSERT(aName);
+    mLazyAnchorPosAnchorChanges.AppendElement(
+        AnchorPosAnchorChange{RefPtr<const nsAtom>(aName), aFrame, true});
+    ;
+  }
+
+  /**
+   * When frame is about to be destroyed, we need to remove all references to it
+   * without delay.
+   */
+  void RemoveAnchorPosAnchorNow(const RefPtr<const nsAtom>& aName,
+                                nsIFrame* aFrame);
 
 #ifdef MOZ_REFLOW_PERF
   void DumpReflows();
@@ -1808,6 +1834,12 @@ class PresShell final : public nsStubDocumentObserver,
    */
   static Modifiers GetCurrentModifiers() { return sCurrentModifiers; }
 
+  inline void AssertNoAnchorPosAnchorChanges() const {
+    MOZ_ASSERT(mLazyAnchorPosAnchorChanges.IsEmpty());
+  }
+
+  void MergeAnchorPosAnchorChanges();
+
  private:
   ~PresShell();
 
@@ -3197,6 +3229,13 @@ class PresShell final : public nsStubDocumentObserver,
 
   nsTHashMap<RefPtr<const nsAtom>, nsTArray<nsIFrame*>> mAnchorPosAnchors;
 
+  struct AnchorPosAnchorChange {
+    RefPtr<const nsAtom> mName;
+    nsIFrame* mFrame;
+    bool mIsRemoval;
+  };
+  nsTArray<AnchorPosAnchorChange> mLazyAnchorPosAnchorChanges;
+
   // Reflow roots that need to be reflowed.
   DepthOrderedFrameList mDirtyRoots;
 

layout/base/RestyleManager.cpp

@@ -1616,7 +1616,7 @@ void RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList) {
   nsCSSFrameConstructor* frameConstructor = presContext->FrameConstructor();
 
   bool didUpdateCursor = false;
-
+  presContext->PresShell()->AssertNoAnchorPosAnchorChanges();
   for (size_t i = 0; i < aChangeList.Length(); ++i) {
     // Collect and coalesce adjacent siblings for lazy frame construction.
     // Eventually it would be even better to make RecreateFramesForContent
@@ -1928,6 +1928,7 @@ void RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList) {
       frame->UpdateVisibleDescendantsState();
     }
   }
+  presContext->PresShell()->MergeAnchorPosAnchorChanges();
 
   aChangeList.Clear();
   FlushOverflowChangedTracker();
@@ -3276,6 +3277,7 @@ void RestyleManager::DoProcessPendingRestyles(ServoTraversalFlags aFlags) {
     // those into a secondary queue and iterate until there's nothing left.
     ReentrantChangeList newChanges;
     mReentrantChanges = &newChanges;
+    presShell->AssertNoAnchorPosAnchorChanges();
 
     {
       DocumentStyleRootIterator iter(doc->GetServoRestyleRoot());
@@ -3299,6 +3301,8 @@ void RestyleManager::DoProcessPendingRestyles(ServoTraversalFlags aFlags) {
       }
     }
 
+    presShell->MergeAnchorPosAnchorChanges();
+
     doc->ClearServoRestyleRoot();
     ClearSnapshots();
 

layout/generic/nsIFrame.cpp

@@ -887,7 +887,7 @@ void nsIFrame::Destroy(DestroyContext& aContext) {
 
   if (HasAnchorPosName()) {
     for (auto& name : disp->mAnchorName.AsSpan()) {
-      PresShell()->RemoveAnchorPosAnchor(name.AsAtom(), this);
+      PresShell()->RemoveAnchorPosAnchorNow(name.AsAtom(), this);
     }
   }
 

testing/web-platform/meta/css/css-anchor-position/anchor-getComputedStyle-001.html.ini

@@ -1,5 +1,4 @@
 [anchor-getComputedStyle-001.html]
-  expected: CRASH
   [Basic case]
     expected: FAIL
 

testing/web-platform/meta/css/css-anchor-position/anchor-getComputedStyle-002.html.ini

@@ -1,5 +1,4 @@
 [anchor-getComputedStyle-002.html]
-  expected: CRASH
   [getComputedStyle() with fragmented containing block in multicolumn layout]
     expected: FAIL
 

testing/web-platform/meta/css/css-anchor-position/anchor-scope-shadow-names.html.ini

@@ -1,4 +1,3 @@
 [anchor-scope-shadow-names.html]
-  expected: CRASH
   [anchor-scope in ::part() affects slotted-in element]
     expected: FAIL

testing/web-platform/meta/css/css-anchor-position/anchor-scroll-004.html.ini

@@ -1,5 +1,4 @@
 [anchor-scroll-004.html]
-  expected: CRASH
   [Initial position of the targets]
     expected: FAIL
 

testing/web-platform/meta/css/css-anchor-position/anchor-transition-eval.html.ini

@@ -1,5 +1,4 @@
 [anchor-transition-eval.html]
-  expected: CRASH
   [Transition when the result of anchor() changes]
     expected: FAIL
 

testing/web-platform/meta/css/css-anchor-position/position-anchor-target-with-children.html.ini

@@ -0,0 +1,2 @@
+[position-anchor-target-with-children.html]
+  expected: [PASS, FAIL]