Bug 1459997 - Add much more rigorous assertions for retained-dl assumptions. r=miko

This adds an assertion checking for duplicate items whenever we create an item, and when we merge an item into the final list.

I had to disable tracking for the anonymous inner list for nsDisplayPerspective and nsDisplayTransform (and manually forward RemoveFrame to them), as well as skipping the assertion for multi-page (since we can end up duplicating wrap lists, but isn't a problem, since we don't retain these).

MozReview-Commit-ID: 4n6rx9bQNan

--HG--
extra : source : ff93cd94b7c548ef57fa13b7eaf85992a0a60587

layout/generic/nsFrame.cpp

@@ -904,7 +904,7 @@ nsIFrame::AddDisplayItem(nsDisplayItem* aItem)
     items = new DisplayItemArray();
     AddProperty(DisplayItems(), items);
   }
-  MOZ_ASSERT(!items->Contains(aItem));
+  MOZ_DIAGNOSTIC_ASSERT(!items->Contains(aItem));
   items->AppendElement(aItem);
 }
 

layout/generic/nsSimplePageSequenceFrame.cpp

@@ -720,6 +720,8 @@ void
 nsSimplePageSequenceFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
                                             const nsDisplayListSet& aLists)
 {
+  aBuilder->SetInPageSequence(true);
+  aBuilder->SetDisablePartialUpdates(true);
   DisplayBorderBackgroundOutline(aBuilder, aLists);
 
   nsDisplayList content;
@@ -753,6 +755,7 @@ nsSimplePageSequenceFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
                                           ::ComputePageSequenceTransform));
 
   aLists.Content()->AppendToTop(&content);
+  aBuilder->SetInPageSequence(false);
 }
 
 //------------------------------------------------------------------------------

layout/painting/RetainedDisplayListBuilder.cpp

@@ -117,6 +117,11 @@ RetainedDisplayListBuilder::PreProcessDisplayList(RetainedDisplayList* aList,
   aList->mOldItems.SetCapacity(aList->Count());
   MOZ_ASSERT(aList->mOldItems.IsEmpty());
   while (nsDisplayItem* item = aList->RemoveBottom()) {
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+    item->mMergedItem = false;
+    item->mPreProcessedItem = true;
+#endif
+
     if (item->HasDeletedFrame() || !item->CanBeReused()) {
       size_t i = aList->mOldItems.Length();
       aList->mOldItems.AppendElement(OldItemInfo(nullptr));
@@ -275,8 +280,10 @@ public:
     if (!HasModifiedFrame(aNewItem) &&
         HasMatchingItemInOldList(aNewItem, &oldIndex)) {
       nsDisplayItem* oldItem = mOldItems[oldIndex.val].mItem;
+      MOZ_DIAGNOSTIC_ASSERT(oldItem->GetPerFrameKey() == aNewItem->GetPerFrameKey() &&
+                            oldItem->Frame() == aNewItem->Frame());
       if (!mOldItems[oldIndex.val].IsChanged()) {
-        MOZ_ASSERT(!mOldItems[oldIndex.val].IsUsed());
+        MOZ_DIAGNOSTIC_ASSERT(!mOldItems[oldIndex.val].IsUsed());
         if (aNewItem->GetChildren()) {
           Maybe<const ActiveScrolledRoot*> containerASRForChildren;
           if (mBuilder->MergeDisplayLists(aNewItem->GetChildren(),
@@ -355,6 +362,20 @@ public:
                              Span<const MergedListIndex> aDirectPredecessors,
                              const Maybe<MergedListIndex>& aExtraDirectPredecessor) {
     UpdateContainerASR(aItem);
+
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+    nsIFrame::DisplayItemArray* items = aItem->Frame()->GetProperty(nsIFrame::DisplayItems());
+    for (nsDisplayItem* i : *items) {
+      if (i->Frame() == aItem->Frame() &&
+          i->GetPerFrameKey() == aItem->GetPerFrameKey()) {
+        MOZ_DIAGNOSTIC_ASSERT(!i->mMergedItem);
+      }
+    }
+
+    aItem->mMergedItem = true;
+    aItem->mPreProcessedItem = false;
+#endif
+
     mMergedItems.AppendToTop(aItem);
     MergedListIndex newIndex = mMergedDAG.AddNode(aDirectPredecessors, aExtraDirectPredecessor);
     return newIndex;

layout/painting/nsDisplayList.cpp

@@ -124,6 +124,26 @@ SpammyLayoutWarningsEnabled()
 }
 #endif
 
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+void AssertUniqueItem(nsDisplayItem* aItem) {
+  nsIFrame::DisplayItemArray* items = aItem->Frame()->GetProperty(nsIFrame::DisplayItems());
+  if (!items) {
+    return;
+  }
+  for (nsDisplayItem* i : *items) {
+    if (i != aItem &&
+        !i->HasDeletedFrame() &&
+        i->Frame() == aItem->Frame() &&
+        i->GetPerFrameKey() == aItem->GetPerFrameKey()) {
+      if (i->mPreProcessedItem) {
+        continue;
+      }
+      MOZ_DIAGNOSTIC_ASSERT(false, "Duplicate display item!");
+    }
+  }
+}
+#endif
+
 /* static */ bool
 ActiveScrolledRoot::IsAncestor(const ActiveScrolledRoot* aAncestor,
                                const ActiveScrolledRoot* aDescendant)
@@ -996,6 +1016,7 @@ nsDisplayListBuilder::nsDisplayListBuilder(nsIFrame* aReferenceFrame,
       mAllowMergingAndFlattening(true),
       mWillComputePluginGeometry(false),
       mInTransform(false),
+      mInPageSequence(false),
       mIsInChromePresContext(false),
       mSyncDecodeImages(false),
       mIsPaintingToWindow(false),
@@ -3088,7 +3109,8 @@ nsDisplayItem::nsDisplayItem(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame)
 {}
 
 nsDisplayItem::nsDisplayItem(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
-                             const ActiveScrolledRoot* aActiveScrolledRoot)
+                             const ActiveScrolledRoot* aActiveScrolledRoot,
+                             bool aAnonymous)
   : mFrame(aFrame)
   , mActiveScrolledRoot(aActiveScrolledRoot)
   , mAnimatedGeometryRoot(nullptr)
@@ -3102,7 +3124,7 @@ nsDisplayItem::nsDisplayItem(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
 #endif
 {
   MOZ_COUNT_CTOR(nsDisplayItem);
-  if (aBuilder->IsRetainingDisplayList()) {
+  if (aBuilder->IsRetainingDisplayList() && !aAnonymous) {
     mFrame->AddDisplayItem(this);
   }
   mReferenceFrame = aBuilder->FindReferenceFrameFor(aFrame, &mToReferenceFrame);
@@ -6093,17 +6115,18 @@ nsDisplayBoxShadowInner::ComputeVisibility(nsDisplayListBuilder* aBuilder,
 }
 
 nsDisplayWrapList::nsDisplayWrapList(nsDisplayListBuilder* aBuilder,
-                                     nsIFrame* aFrame, nsDisplayList* aList)
+                                     nsIFrame* aFrame, nsDisplayList* aList, bool aAnonymous)
   : nsDisplayWrapList(aBuilder, aFrame, aList,
-                      aBuilder->CurrentActiveScrolledRoot())
+                      aBuilder->CurrentActiveScrolledRoot(), false, 0, aAnonymous)
 {}
 
 nsDisplayWrapList::nsDisplayWrapList(nsDisplayListBuilder* aBuilder,
                                      nsIFrame* aFrame, nsDisplayList* aList,
                                      const ActiveScrolledRoot* aActiveScrolledRoot,
                                      bool aClearClipChain,
-                                     uint32_t aIndex)
+                                     uint32_t aIndex,
-  : nsDisplayItem(aBuilder, aFrame, aActiveScrolledRoot)
+                                     bool aAnonymous)
+  : nsDisplayItem(aBuilder, aFrame, aActiveScrolledRoot, aAnonymous)
   , mFrameActiveScrolledRoot(aBuilder->CurrentActiveScrolledRoot())
   , mOverrideZIndex(0)
   , mIndex(aIndex)
@@ -6146,8 +6169,8 @@ nsDisplayWrapList::nsDisplayWrapList(nsDisplayListBuilder* aBuilder,
 }
 
 nsDisplayWrapList::nsDisplayWrapList(nsDisplayListBuilder* aBuilder,
-                                     nsIFrame* aFrame, nsDisplayItem* aItem)
+                                     nsIFrame* aFrame, nsDisplayItem* aItem, bool aAnonymous)
-  : nsDisplayItem(aBuilder, aFrame)
+  : nsDisplayItem(aBuilder, aFrame, aBuilder->CurrentActiveScrolledRoot(), aAnonymous)
   , mOverrideZIndex(0)
   , mIndex(0)
   , mHasZIndexOverride(false)
@@ -9139,7 +9162,7 @@ nsDisplayPerspective::nsDisplayPerspective(nsDisplayListBuilder* aBuilder,
                                            nsIFrame* aPerspectiveFrame,
                                            nsDisplayList* aList)
   : nsDisplayItem(aBuilder, aPerspectiveFrame)
-  , mList(aBuilder, aPerspectiveFrame, aList)
+  , mList(aBuilder, aPerspectiveFrame, aList, true)
   , mTransformFrame(aTransformFrame)
   , mIndex(aBuilder->AllocatePerspectiveItemIndex())
 {

layout/painting/nsDisplayList.h

@@ -836,6 +836,9 @@ public:
    */
   void SetInTransform(bool aInTransform) { mInTransform = aInTransform; }
 
+  bool IsInPageSequence() const { return mInPageSequence; }
+  void SetInPageSequence(bool aInPage) { mInPageSequence = aInPage; }
+
   /**
    * Return true if we're currently building a display list for a
    * nested presshell.
@@ -1980,6 +1983,7 @@ private:
   // True when we're building a display list that's directly or indirectly
   // under an nsDisplayTransform
   bool                           mInTransform;
+  bool                           mInPageSequence;
   bool                           mIsInChromePresContext;
   bool                           mSyncDecodeImages;
   bool                           mIsPaintingToWindow;
@@ -2025,6 +2029,10 @@ protected:
 
 class nsDisplayWrapList;
 
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+void AssertUniqueItem(nsDisplayItem* aItem);
+#endif
+
 template<typename T, typename... Args>
 MOZ_ALWAYS_INLINE T*
 MakeDisplayItem(nsDisplayListBuilder* aBuilder, Args&&... aArgs)
@@ -2043,6 +2051,14 @@ MakeDisplayItem(nsDisplayListBuilder* aBuilder, Args&&... aArgs)
     }
   }
 
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+  if (aBuilder->IsRetainingDisplayList() &&
+      !aBuilder->IsInPageSequence() &&
+      aBuilder->IsBuilding()) {
+    AssertUniqueItem(item);
+  }
+#endif
+
   return item;
 }
 
@@ -2084,7 +2100,8 @@ public:
   // need to count constructors and destructors.
   nsDisplayItem(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame);
   nsDisplayItem(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
-                const ActiveScrolledRoot* aActiveScrolledRoot);
+                const ActiveScrolledRoot* aActiveScrolledRoot,
+                bool aAnonymous = false);
 
   /**
    * This constructor is only used in rare cases when we need to construct
@@ -2839,12 +2856,18 @@ public:
   // used by RetainedDisplayListBuilder.
   void SetOldListIndex(nsDisplayList* aList, OldListIndex aIndex)
   {
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
     mOldList = reinterpret_cast<uintptr_t>(aList);
+#endif
     mOldListIndex = aIndex;
   }
   OldListIndex GetOldListIndex(nsDisplayList* aList)
   {
-    MOZ_ASSERT(mOldList == reinterpret_cast<uintptr_t>(aList));
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+    if (mOldList != reinterpret_cast<uintptr_t>(aList)) {
+      MOZ_CRASH_UNSAFE_PRINTF("Item found was in the wrong list! type %d", GetPerFrameKey());
+    }
+#endif
     return mOldListIndex;
   }
 
@@ -2883,7 +2906,13 @@ private:
 
 protected:
 
-  mozilla::DebugOnly<uintptr_t> mOldList;
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+public:
+  uintptr_t mOldList = 0;
+  bool mMergedItem = false;
+  bool mPreProcessedItem = false;
+protected:
+#endif
   OldListIndex mOldListIndex;
 
   bool      mForceNotVisible;
@@ -5009,14 +5038,15 @@ public:
    * Takes all the items from aList and puts them in our list.
    */
   nsDisplayWrapList(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
-                    nsDisplayList* aList);
+                    nsDisplayList* aList, bool aAnonymous = false);
   nsDisplayWrapList(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
                     nsDisplayList* aList,
                     const ActiveScrolledRoot* aActiveScrolledRoot,
                     bool aClearClipChain = false,
-                    uint32_t aIndex = 0);
+                    uint32_t aIndex = 0,
+                    bool aAnonymous = false);
   nsDisplayWrapList(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
-                    nsDisplayItem* aItem);
+                    nsDisplayItem* aItem, bool aAnonymous = false);
   nsDisplayWrapList(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame)
     : nsDisplayItem(aBuilder, aFrame)
     , mFrameActiveScrolledRoot(aBuilder->CurrentActiveScrolledRoot())
@@ -6292,10 +6322,10 @@ class nsDisplayTransform: public nsDisplayItem
   public:
     StoreList(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
               nsDisplayList* aList) :
-      nsDisplayWrapList(aBuilder, aFrame, aList) {}
+      nsDisplayWrapList(aBuilder, aFrame, aList, true) {}
     StoreList(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
               nsDisplayItem* aItem) :
-      nsDisplayWrapList(aBuilder, aFrame, aItem) {}
+      nsDisplayWrapList(aBuilder, aFrame, aItem, true) {}
     virtual ~StoreList() {}
 
     virtual void UpdateBounds(nsDisplayListBuilder* aBuilder) override {
@@ -6663,6 +6693,12 @@ public:
       mFrame->Combines3DTransformWithAncestors();
   }
 
+  virtual void RemoveFrame(nsIFrame* aFrame) override
+  {
+    nsDisplayItem::RemoveFrame(aFrame);
+    mStoredList.RemoveFrame(aFrame);
+  }
+
 private:
   void ComputeBounds(nsDisplayListBuilder* aBuilder);
   void SetReferenceFrameToAncestor(nsDisplayListBuilder* aBuilder);
@@ -6837,6 +6873,7 @@ public:
       mTransformFrame = nullptr;
     }
     nsDisplayItem::RemoveFrame(aFrame);
+    mList.RemoveFrame(aFrame);
   }
 
 private: