Bug 1418945 - Always use the new-list ordering when merging display list since it's guaranteed to be correct. r=miko, a=gchang
authorMatt Woodrow <mwoodrow@mozilla.com>
Wed, 22 Nov 2017 15:10:31 +1300
changeset 445075 51795bc51debe7bd0426bcd8ab6fa2ca75f8c6af
parent 445074 409df5c82beae70a7060bceb2e5072ee78d78c52
child 445076 921cee6d10c2eb419ba142d4aa4d8c17d9c4aedb
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmiko, gchang
bugs1418945
milestone58.0
Bug 1418945 - Always use the new-list ordering when merging display list since it's guaranteed to be correct. r=miko, a=gchang
layout/painting/RetainedDisplayListBuilder.cpp
layout/reftests/display-list/1418945-1-ref.html
layout/reftests/display-list/1418945-1.html
layout/reftests/display-list/reftest.list
--- a/layout/painting/RetainedDisplayListBuilder.cpp
+++ b/layout/painting/RetainedDisplayListBuilder.cpp
@@ -333,24 +333,20 @@ void UpdateASR(nsDisplayItem* aItem,
  *
  * Old List: A, B
  * New List, B, A
  * Invalidations: -
  *
  * This can happen because a prior merge might have changed the ordering
  * for non-intersecting items.
  *
- * We match the B items, and copy A from the old list into the merged list,
- * and then the new B into the merged list.
- * We now get to A in the new list, but A has already been added to the merged
- * list from the old list. This is fine since they must be identical (or we
- * would have invalidated A), so we detect it and just destroy the new
- * instance of A.
+ * We match the B items, but don't copy A since it's also present in the new list
+ * and then add the new B into the merged list. We then add A, and we're done.
  *
- * Merged List: A, B
+ * Merged List: B, A
  */
 void
 RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList* aNewList,
                                               nsDisplayList* aOldList,
                                               nsDisplayList* aOutList,
                                               Maybe<const ActiveScrolledRoot*>& aOutContainerASR)
 {
   nsDisplayList merged(&mBuilder);
@@ -388,97 +384,87 @@ RetainedDisplayListBuilder::MergeDisplay
   for (nsDisplayItem* i = aOldList->GetBottom(); i != nullptr; i = i->GetAbove()) {
     i->SetReused(false);
 
     if (!aNewList->IsEmpty()) {
       oldListLookup.Put({ i->Frame(), i->GetPerFrameKey() }, i);
     }
   }
 
-#ifdef DEBUG
   nsDataHashtable<DisplayItemHashEntry, nsDisplayItem*> newListLookup(aNewList->Count());
   for (nsDisplayItem* i = aNewList->GetBottom(); i != nullptr; i = i->GetAbove()) {
+#ifdef DEBUG
     if (newListLookup.Get({ i->Frame(), i->GetPerFrameKey() }, nullptr)) {
        MOZ_CRASH_UNSAFE_PRINTF("Duplicate display items detected!: %s(0x%p) type=%d key=%d",
                                 i->Name(), i->Frame(),
                                 static_cast<int>(i->GetType()), i->GetPerFrameKey());
     }
+#endif
     newListLookup.Put({ i->Frame(), i->GetPerFrameKey() }, i);
   }
-#endif
 
   while (nsDisplayItem* newItem = aNewList->RemoveBottom()) {
     if (nsDisplayItem* oldItem = oldListLookup.Get({ newItem->Frame(), newItem->GetPerFrameKey() })) {
-      if (oldItem->IsReused()) {
-        // If there's a matching item in the old list, but we've already put it into the
-        // merged list then stick with that one. Merge any child lists, and then delete the
-        // new item. This solves example 2 from above.
-
-        if (oldItem->GetChildren()) {
-          MOZ_ASSERT(newItem->GetChildren());
-          Maybe<const ActiveScrolledRoot*> containerASRForChildren;
-          MergeDisplayLists(newItem->GetChildren(), oldItem->GetChildren(),
-                            oldItem->GetChildren(), containerASRForChildren);
-          UpdateASR(oldItem, containerASRForChildren);
-          oldItem->UpdateBounds(&mBuilder);
-        }
-        if (oldItem->GetType() == DisplayItemType::TYPE_LAYER_EVENT_REGIONS) {
-          MergeLayerEventRegions(oldItem, newItem);
-        }
-        newItem->Destroy(&mBuilder);
-      } else {
-        // The new item has a matching counterpart in the old list, so copy all valid
-        // items from the old list into the merged list until we get to the matched item.
+      // The new item has a matching counterpart in the old list that we haven't yet reached,
+      // so copy all valid items from the old list into the merged list until we get to the
+      // matched item.
+      if (!oldItem->IsReused()) {
         nsDisplayItem* old = nullptr;
         while ((old = aOldList->RemoveBottom()) && !IsSameItem(newItem, old)) {
-          if (!IsAnyAncestorModified(old->FrameForInvalidation())) {
+          if (IsAnyAncestorModified(old->FrameForInvalidation())) {
+            // The old item is invalid, discard it.
+            oldListLookup.Remove({ old->Frame(), old->GetPerFrameKey() });
+            old->Destroy(&mBuilder);
+          } else if (newListLookup.Get({ old->Frame(), old->GetPerFrameKey() })) {
+            // The old item is also in the new list, but we haven't got to it yet.
+            // Mark that we've found it, and we'll deal with it when we get to the new
+            // entry.
+            old->SetReused(true);
+          } else {
             // Recurse into the child list (without a matching new list) to
             // ensure that we find and remove any invalidated items.
             if (old->GetChildren()) {
               nsDisplayList empty(&mBuilder);
               Maybe<const ActiveScrolledRoot*> containerASRForChildren;
               MergeDisplayLists(&empty, old->GetChildren(),
                                 old->GetChildren(), containerASRForChildren);
               UpdateASR(old, containerASRForChildren);
               old->UpdateBounds(&mBuilder);
             }
             ReuseItem(old);
-          } else {
-            oldListLookup.Remove({ old->Frame(), old->GetPerFrameKey() });
-            old->Destroy(&mBuilder);
           }
         }
         MOZ_ASSERT(old && IsSameItem(newItem, old));
         MOZ_ASSERT(old == oldItem);
+      }
 
-        // Recursively merge any child lists, destroy the old item and add
-        // the new one to the list.
-        if (old->GetType() == DisplayItemType::TYPE_LAYER_EVENT_REGIONS &&
-            !IsAnyAncestorModified(old->FrameForInvalidation())) {
-          // Event regions items don't have anything interesting other than
-          // the lists of regions and frames, so we have no need to use the
-          // newer item. Always use the old item instead since we assume it's
-          // likely to have the bigger lists and merging will be quicker.
-          MergeLayerEventRegions(old, newItem);
-          ReuseItem(old);
-          newItem->Destroy(&mBuilder);
-        } else {
-          if (!IsAnyAncestorModified(old->FrameForInvalidation()) &&
-              old->GetChildren()) {
-            MOZ_ASSERT(newItem->GetChildren());
-            Maybe<const ActiveScrolledRoot*> containerASRForChildren;
-            MergeDisplayLists(newItem->GetChildren(), old->GetChildren(),
-                              newItem->GetChildren(), containerASRForChildren);
-            UpdateASR(newItem, containerASRForChildren);
-            newItem->UpdateBounds(&mBuilder);
-          }
+      // Recursively merge any child lists, destroy the old item and add
+      // the new one to the list.
+      if (oldItem->GetType() == DisplayItemType::TYPE_LAYER_EVENT_REGIONS &&
+          !IsAnyAncestorModified(oldItem->FrameForInvalidation())) {
+        // Event regions items don't have anything interesting other than
+        // the lists of regions and frames, so we have no need to use the
+        // newer item. Always use the old item instead since we assume it's
+        // likely to have the bigger lists and merging will be quicker.
+        MergeLayerEventRegions(oldItem, newItem);
+        ReuseItem(oldItem);
+        newItem->Destroy(&mBuilder);
+      } else {
+        if (!IsAnyAncestorModified(oldItem->FrameForInvalidation()) &&
+            oldItem->GetChildren()) {
+          MOZ_ASSERT(newItem->GetChildren());
+          Maybe<const ActiveScrolledRoot*> containerASRForChildren;
+          MergeDisplayLists(newItem->GetChildren(), oldItem->GetChildren(),
+                            newItem->GetChildren(), containerASRForChildren);
+          UpdateASR(newItem, containerASRForChildren);
+          newItem->UpdateBounds(&mBuilder);
+        }
 
-          old->Destroy(&mBuilder);
-          UseItem(newItem);
-        }
+        oldItem->Destroy(&mBuilder);
+        UseItem(newItem);
       }
     } else {
       // If there was no matching item in the old list, then we only need to
       // add the new item to the merged list.
       UseItem(newItem);
     }
   }
 
new file mode 100644
--- /dev/null
+++ b/layout/reftests/display-list/1418945-1-ref.html
@@ -0,0 +1,24 @@
+<html>
+<head>
+<style>
+  div {
+    width: 100px;
+    height: 100px;
+    position:relative;
+  }
+  #first {
+    background-color: red;
+    z-index: 1;
+  }
+  #second {
+    top: -50px;
+    background-color: green;
+    z-index: 1;
+  }
+</style>
+</head>
+<body>
+    <div id="first"></div>
+    <div id="second"></div>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/display-list/1418945-1.html
@@ -0,0 +1,34 @@
+<html class="reftest-wait">
+<head>
+<style>
+  div {
+    width: 100px;
+    height: 100px;
+    position:relative;
+  }
+  #first {
+    background-color: red;
+    z-index: 2;
+  }
+  #second {
+    top: -50px;
+    background-color: green;
+    z-index: 1;
+  }
+</style>
+<script>
+
+function doTest()
+{
+  document.getElementById("first").style.zIndex = 1;
+  document.documentElement.removeAttribute("class");
+}
+
+document.addEventListener("MozReftestInvalidate", doTest);
+</script>
+</head>
+<body>
+    <div id="first"></div>
+    <div id="second"></div>
+</body>
+</html>
--- a/layout/reftests/display-list/reftest.list
+++ b/layout/reftests/display-list/reftest.list
@@ -6,8 +6,9 @@ skip-if(!retainedDisplayList||!asyncPan)
 skip-if(!retainedDisplayList) == retained-dl-remove-for-ancestor-change-1.html retained-dl-remove-for-ancestor-change-1-ref.html
 skip-if(!retainedDisplayList) == retained-dl-scroll-out-of-view-1.html retained-dl-scroll-out-of-view-1-ref.html
 skip-if(!retainedDisplayList) == retained-dl-displayport-1.html retained-dl-displayport-1-ref.html
 skip-if(!retainedDisplayList) == retained-dl-prerender-transform-1.html retained-dl-prerender-transform-1-ref.html
 == retained-dl-wrap-list.html retained-dl-wrap-list-ref.html
 fuzzy(1,235200) == 1413073.html 1413073-ref.html
 == 1416291.html 1416291-ref.html
 == 1417601-1.html 1417601-1-ref.html
+== 1418945-1.html 1418945-1-ref.html