Backed out changesets b232989d707c and 551e3cbe82ba (bug 1500864) for causing topcrash bug 1514528.
authorRyan VanderMeulen <ryanvm@gmail.com>
Sun, 16 Dec 2018 13:11:32 -0500
changeset 507967 36a2f27ecc47
parent 507966 5ca841fa4f30
child 507981 b990112ccff1
child 507987 323a4c385e45
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1500864, 1514528
milestone66.0a1
backs outb232989d707c
551e3cbe82ba
first release with
nightly linux32
36a2f27ecc47 / 66.0a1 / 20181216181156 / files
nightly linux64
36a2f27ecc47 / 66.0a1 / 20181216181156 / files
nightly mac
36a2f27ecc47 / 66.0a1 / 20181216181156 / files
nightly win32
36a2f27ecc47 / 66.0a1 / 20181216181156 / files
nightly win64
36a2f27ecc47 / 66.0a1 / 20181216181156 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out changesets b232989d707c and 551e3cbe82ba (bug 1500864) for causing topcrash bug 1514528.
layout/painting/RetainedDisplayListBuilder.cpp
layout/painting/RetainedDisplayListBuilder.h
--- a/layout/painting/RetainedDisplayListBuilder.cpp
+++ b/layout/painting/RetainedDisplayListBuilder.cpp
@@ -8,17 +8,16 @@
 #include "RetainedDisplayListBuilder.h"
 
 #include "DisplayListChecker.h"
 #include "gfxPrefs.h"
 #include "nsPlaceholderFrame.h"
 #include "nsSubDocumentFrame.h"
 #include "nsViewManager.h"
 #include "nsCanvasFrame.h"
-#include "mozilla/AutoRestore.h"
 
 /**
  * Code for doing display list building for a modified subset of the window,
  * and then merging it into the existing display list (for the full window).
  *
  * The approach primarily hinges on the observation that the ‘true’ ordering of
  * display items is represented by a DAG (only items that intersect in 2d space
  * have a defined ordering). Our display list is just one of a many possible
@@ -313,35 +312,18 @@ class MergeState {
             std::move(*reinterpret_cast<DirectedAcyclicGraph<OldListUnits>*>(
                 &aOldList.mDAG))),
         mOuterKey(aOuterKey),
         mResultIsModified(false) {
     mMergedDAG.EnsureCapacityFor(mOldDAG);
     MOZ_RELEASE_ASSERT(mOldItems.Length() == mOldDAG.Length());
   }
 
-  // Items within an opacity:0 container (excluding plugins and hit test info)
-  // aren't needed, so we can strip them out from both the old and new lists.
-  // Do this silently, so that these changes don't result in us reporting that
-  // the display list changed. Both FrameLayerBuilder and
-  // WebRenderCommandBuilder also do this removal, so it's functionally correct
-  // to report that the display list is identical.
-  bool ShouldSilentlyDiscardItem(nsDisplayItem* aItem) {
-    return mBuilder->mForEventsAndPluginsOnly &&
-           (aItem->GetType() != DisplayItemType::TYPE_COMPOSITOR_HITTEST_INFO &&
-            aItem->GetType() != DisplayItemType::TYPE_PLUGIN);
-  }
-
-  Maybe<MergedListIndex> ProcessItemFromNewList(
+  MergedListIndex ProcessItemFromNewList(
       nsDisplayItem* aNewItem, const Maybe<MergedListIndex>& aPreviousItem) {
-    if (ShouldSilentlyDiscardItem(aNewItem)) {
-      aNewItem->Destroy(mBuilder->Builder());
-      return aPreviousItem;
-    }
-
     OldListIndex oldIndex;
     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()) {
@@ -357,17 +339,18 @@ class MergeState {
           // rect across from the new item.
           oldItem->SetBuildingRect(aNewItem->GetBuildingRect());
         }
 
         if (aNewItem->GetChildren()) {
           Maybe<const ActiveScrolledRoot*> containerASRForChildren;
           if (mBuilder->MergeDisplayLists(
                   aNewItem->GetChildren(), oldItem->GetChildren(),
-                  destItem->GetChildren(), containerASRForChildren, aNewItem)) {
+                  destItem->GetChildren(), containerASRForChildren,
+                  aNewItem->GetPerFrameKey())) {
             destItem->InvalidateCachedChildInfo();
             mResultIsModified = true;
           }
           UpdateASR(destItem, containerASRForChildren);
           destItem->UpdateBounds(mBuilder->Builder());
         }
 
         AutoTArray<MergedListIndex, 2> directPredecessors =
@@ -375,22 +358,22 @@ class MergeState {
         MergedListIndex newIndex = AddNewNode(
             destItem, Some(oldIndex), directPredecessors, aPreviousItem);
         mOldItems[oldIndex.val].AddedMatchToMergedList(mBuilder, newIndex);
         if (destItem == aNewItem) {
           oldItem->Destroy(mBuilder->Builder());
         } else {
           aNewItem->Destroy(mBuilder->Builder());
         }
-        return Some(newIndex);
+        return newIndex;
       }
     }
     mResultIsModified = true;
-    return Some(AddNewNode(aNewItem, Nothing(), Span<MergedListIndex>(),
-                           aPreviousItem));
+    return AddNewNode(aNewItem, Nothing(), Span<MergedListIndex>(),
+                      aPreviousItem);
   }
 
   bool ShouldUseNewItem(nsDisplayItem* aNewItem) {
     // Generally we want to use the old item when the frame isn't marked as
     // modified so that any cached information on the item (or referencing the
     // item) gets retained. Quite a few FrameLayerBuilder performance
     // improvements benefit by this. Sometimes, however, we can end up where the
     // new item paints something different from the old item, even though we
@@ -517,30 +500,26 @@ class MergeState {
     MergedListIndex newIndex =
         mMergedDAG.AddNode(aDirectPredecessors, aExtraDirectPredecessor);
     return newIndex;
   }
 
   void ProcessOldNode(OldListIndex aNode,
                       nsTArray<MergedListIndex>&& aDirectPredecessors) {
     nsDisplayItem* item = mOldItems[aNode.val].mItem;
-    if (ShouldSilentlyDiscardItem(item)) {
-      // If the item should be discarded, then just silently drop it and
-      // don't mark the display list as being modified.
-      mOldItems[aNode.val].Discard(mBuilder, std::move(aDirectPredecessors));
-    } else if (mOldItems[aNode.val].IsChanged() || HasModifiedFrame(item)) {
+    if (mOldItems[aNode.val].IsChanged() || HasModifiedFrame(item)) {
       mOldItems[aNode.val].Discard(mBuilder, std::move(aDirectPredecessors));
       mResultIsModified = true;
     } else {
       if (item->GetChildren()) {
         Maybe<const ActiveScrolledRoot*> containerASRForChildren;
         nsDisplayList empty;
-        if (mBuilder->MergeDisplayLists(&empty, item->GetChildren(),
-                                        item->GetChildren(),
-                                        containerASRForChildren, item)) {
+        if (mBuilder->MergeDisplayLists(
+                &empty, item->GetChildren(), item->GetChildren(),
+                containerASRForChildren, item->GetPerFrameKey())) {
           item->InvalidateCachedChildInfo();
           mResultIsModified = true;
         }
         UpdateASR(item, containerASRForChildren);
         item->UpdateBounds(mBuilder->Builder());
       }
       if (item->GetType() == DisplayItemType::TYPE_SUBDOCUMENT) {
         mBuilder->IncrementSubDocPresShellPaintCount(item);
@@ -646,29 +625,23 @@ class MergeState {
  *
  * Once we've merged a list, we then retain the DAG (as part of the
  * RetainedDisplayList object) to use for future merges.
  */
 bool RetainedDisplayListBuilder::MergeDisplayLists(
     nsDisplayList* aNewList, RetainedDisplayList* aOldList,
     RetainedDisplayList* aOutList,
     mozilla::Maybe<const mozilla::ActiveScrolledRoot*>& aOutContainerASR,
-    nsDisplayItem* aOuterItem) {
-  MergeState merge(this, *aOldList,
-                   aOuterItem ? aOuterItem->GetPerFrameKey() : 0);
-
-  AutoRestore<bool> restoreForEventAndPluginsOnly(mForEventsAndPluginsOnly);
-  if (aOuterItem && aOuterItem->GetType() == DisplayItemType::TYPE_OPACITY &&
-      static_cast<nsDisplayOpacity*>(aOuterItem)->ForEventsAndPluginsOnly()) {
-    mForEventsAndPluginsOnly = true;
-  }
+    uint32_t aOuterKey) {
+  MergeState merge(this, *aOldList, aOuterKey);
 
   Maybe<MergedListIndex> previousItemIndex;
   while (nsDisplayItem* item = aNewList->RemoveBottom()) {
-    previousItemIndex = merge.ProcessItemFromNewList(item, previousItemIndex);
+    previousItemIndex =
+        Some(merge.ProcessItemFromNewList(item, previousItemIndex));
   }
 
   *aOutList = merge.Finalize();
   aOutContainerASR = merge.mContainerASR;
   return merge.mResultIsModified;
 }
 
 static void TakeAndAddModifiedAndFramesWithPropsFromRootFrame(
@@ -1285,16 +1258,21 @@ auto RetainedDisplayListBuilder::Attempt
                                             mBuilder.RootReferenceFrame());
     }
   }
 
   modifiedDirty.IntersectRect(
       modifiedDirty,
       mBuilder.RootReferenceFrame()->GetVisualOverflowRectRelativeToSelf());
 
+  PartialUpdateResult result = PartialUpdateResult::NoChange;
+  if (!modifiedDirty.IsEmpty() || !framesWithProps.IsEmpty()) {
+    result = PartialUpdateResult::Updated;
+  }
+
   mBuilder.SetDirtyRect(modifiedDirty);
   mBuilder.SetPartialUpdate(true);
 
   nsDisplayList modifiedDL;
   mBuilder.RootReferenceFrame()->BuildDisplayListForStackingContext(
       &mBuilder, &modifiedDL);
   if (!modifiedDL.IsEmpty()) {
     nsLayoutUtils::AddExtraBackgroundItems(
@@ -1326,17 +1304,16 @@ auto RetainedDisplayListBuilder::Attempt
   // display list merging to prune unused items (for example, items that
   // are not visible anymore) from the old list.
   // TODO: Optimization opportunity. In this case, MergeDisplayLists()
   // unnecessarily creates a hashtable of the old items.
   // TODO: Ideally we could skip this if result is NoChange, but currently when
   // we call RestoreState on nsDisplayWrapList it resets the clip to the base
   // clip, and we need the UpdateBounds call (within MergeDisplayLists) to
   // move it to the correct inner clip.
-  PartialUpdateResult result = PartialUpdateResult::NoChange;
   Maybe<const ActiveScrolledRoot*> dummy;
   if (MergeDisplayLists(&modifiedDL, &mList, &mList, dummy)) {
     result = PartialUpdateResult::Updated;
   }
 
   // printf_stderr("Painting --- Merged list:\n");
   // nsFrame::PrintDisplayList(&mBuilder, mList);
 
--- a/layout/painting/RetainedDisplayListBuilder.h
+++ b/layout/painting/RetainedDisplayListBuilder.h
@@ -90,18 +90,17 @@ RetainedDisplayListData* GetRetainedDisp
  * Returns RetainedDisplayListData property for the given |aRootFrame|. Creates
  * and sets a new RetainedDisplayListData property if it is not already set.
  */
 RetainedDisplayListData* GetOrSetRetainedDisplayListData(nsIFrame* aRootFrame);
 
 struct RetainedDisplayListBuilder {
   RetainedDisplayListBuilder(nsIFrame* aReferenceFrame,
                              nsDisplayListBuilderMode aMode, bool aBuildCaret)
-      : mBuilder(aReferenceFrame, aMode, aBuildCaret, true),
-        mForEventsAndPluginsOnly(false) {}
+      : mBuilder(aReferenceFrame, aMode, aBuildCaret, true) {}
   ~RetainedDisplayListBuilder() { mList.DeleteAll(&mBuilder); }
 
   nsDisplayListBuilder* Builder() { return &mBuilder; }
 
   nsDisplayList* List() { return &mList; }
 
   enum class PartialUpdateResult { Failed, NoChange, Updated };
 
@@ -118,30 +117,21 @@ struct RetainedDisplayListBuilder {
 
   NS_DECLARE_FRAME_PROPERTY_DELETABLE(Cached, RetainedDisplayListBuilder)
 
  private:
   bool PreProcessDisplayList(RetainedDisplayList* aList,
                              AnimatedGeometryRoot* aAGR,
                              uint32_t aCallerKey = 0,
                              uint32_t aNestingDepth = 0);
-
-  /**
-   * Merges items from aNewList into non-invalidated items from aOutList and
-   * stores the result in aOutList.
-   *
-   * aOuterItem is a pointer to an item that owns one of the lists, if
-   * available. If both lists are populated, then both outer items must not be
-   * invalidated, and identical, so either can be passed here.
-   */
   bool MergeDisplayLists(
       nsDisplayList* aNewList, RetainedDisplayList* aOldList,
       RetainedDisplayList* aOutList,
       mozilla::Maybe<const mozilla::ActiveScrolledRoot*>& aOutContainerASR,
-      nsDisplayItem* aOuterItem = nullptr);
+      uint32_t aOuterKey = 0);
 
   bool ComputeRebuildRegion(nsTArray<nsIFrame*>& aModifiedFrames,
                             nsRect* aOutDirty,
                             AnimatedGeometryRoot** aOutModifiedAGR,
                             nsTArray<nsIFrame*>& aOutFramesWithProps);
   bool ProcessFrame(nsIFrame* aFrame, nsDisplayListBuilder& aBuilder,
                     nsIFrame* aStopAtFrame,
                     nsTArray<nsIFrame*>& aOutFramesWithProps,
@@ -150,15 +140,11 @@ struct RetainedDisplayListBuilder {
 
   void IncrementSubDocPresShellPaintCount(nsDisplayItem* aItem);
 
   friend class MergeState;
 
   nsDisplayListBuilder mBuilder;
   RetainedDisplayList mList;
   WeakFrame mPreviousCaret;
-
-  // True if we're currently within an opacity:0 container, and only
-  // plugin and hit test items should be considered.
-  bool mForEventsAndPluginsOnly;
 };
 
 #endif  // RETAINEDDISPLAYLISTBUILDER_H_