Bug 1451384 - Check IsChanged on the old item during merging, since that's the one that might have a deleted frame. r=mstange
authorMatt Woodrow <mwoodrow@mozilla.com>
Thu, 05 Apr 2018 12:20:32 +1200
changeset 411905 a9c5a53970bf7d432833b283779f8b6485a3e57c
parent 411904 edaa920ba38b2509cdbbcab820cbf7c5712165bd
child 411906 aecbdcadaf96d10b3fe771f89030120bcd160a9a
push id62201
push usermwoodrow@mozilla.com
push dateThu, 05 Apr 2018 11:20:03 +0000
treeherderautoland@a9c5a53970bf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmstange
bugs1451384
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1451384 - Check IsChanged on the old item during merging, since that's the one that might have a deleted frame. r=mstange This happens when an nsIFrame* that builds an nsDisplayWrapList is deleted, but then the memory is immediately reused for another frame that builds the same type display item, within the same display list. PreProcessDisplayLists chooses not to descend into the nsDisplayWrapList for the deleted frame, and so mOldItems remains uninitialized for the old sublist. When adding the new instance, IsChanged returns false, since the pointers are the same, and we're checking HasDeletedFrame on the new instance (where it's never true), instead of the old. We then recurse into MergeDisplayLists, with an uninitialized mOldItems array, and crash. I haven't added a test because I haven't yet figured out how to create a minimal testcase, and the test would rely on implementation details of the frame allocator to remain unchanged to be useful. MozReview-Commit-ID: pHimEvfAND
layout/painting/RetainedDisplayListBuilder.cpp
layout/tools/reftest/runreftest.py
--- a/layout/painting/RetainedDisplayListBuilder.cpp
+++ b/layout/painting/RetainedDisplayListBuilder.cpp
@@ -122,16 +122,17 @@ RetainedDisplayListBuilder::PreProcessDi
       aList->mDAG.mDirectPredecessorList.Length() >
       (aList->mDAG.mNodesInfo.Length() * kMaxEdgeRatio)) {
     return false;
 
   }
 
   nsDisplayList saved;
   aList->mOldItems.SetCapacity(aList->Count());
+  MOZ_ASSERT(aList->mOldItems.IsEmpty());
   size_t i = 0;
   while (nsDisplayItem* item = aList->RemoveBottom()) {
     if (item->HasDeletedFrame() || !item->CanBeReused()) {
       // If we haven't yet initialized the DAG, then we can
       // just drop this item. Otherwise we need to keep it
       // around to preserve indices, and merging will
       // get rid of it.
       if (initializeDAG) {
@@ -171,16 +172,17 @@ RetainedDisplayListBuilder::PreProcessDi
       mBuilder.MarkFrameForDisplayIfVisible(f, mBuilder.RootReferenceFrame());
     }
 
     // TODO: This is here because we sometimes reuse the previous display list
     // completely. For optimization, we could only restore the state for reused
     // display items.
     item->RestoreState();
   }
+  MOZ_ASSERT(aList->mOldItems.Length() == aList->mDAG.Length());
   aList->RestoreState();
   return true;
 }
 
 void
 RetainedDisplayListBuilder::IncrementSubDocPresShellPaintCount(nsDisplayItem* aItem)
 {
   MOZ_ASSERT(aItem->GetType() == DisplayItemType::TYPE_SUBDOCUMENT);
@@ -252,22 +254,23 @@ public:
   {
     mOldKeyLookup.SwapElements(aOldList.mKeyLookup);
     mMergedDAG.EnsureCapacityFor(mOldDAG);
   }
 
   MergedListIndex ProcessItemFromNewList(nsDisplayItem* aNewItem, const Maybe<MergedListIndex>& aPreviousItem) {
     OldListIndex oldIndex;
     if (mOldKeyLookup.Get({ aNewItem->Frame(), aNewItem->GetPerFrameKey() }, &oldIndex.val)) {
-      if (!IsChanged(aNewItem)) {
+      nsDisplayItem* oldItem = mOldItems[oldIndex.val].mItem;
+      if (oldItem && !IsChanged(oldItem)) {
         MOZ_ASSERT(!mOldItems[oldIndex.val].IsUsed());
         if (aNewItem->GetChildren()) {
           Maybe<const ActiveScrolledRoot*> containerASRForChildren;
           if (mBuilder->MergeDisplayLists(aNewItem->GetChildren(),
-                                          mOldItems[oldIndex.val].mItem->GetChildren(),
+                                          oldItem->GetChildren(),
                                           aNewItem->GetChildren(),
                                           containerASRForChildren)) {
             mResultIsModified = true;
 
           }
           UpdateASR(aNewItem, containerASRForChildren);
           aNewItem->UpdateBounds(mBuilder->Builder());
         }
--- a/layout/tools/reftest/runreftest.py
+++ b/layout/tools/reftest/runreftest.py
@@ -234,17 +234,17 @@ class RefTest(object):
         update_mozinfo()
         self.lastTestSeen = None
         self.haveDumpedScreen = False
         self.resolver = self.resolver_cls()
         self.log = None
         self.outputHandler = None
         self.testDumpFile = os.path.join(tempfile.gettempdir(), 'reftests.json')
 
-        self.run_by_manifest = True
+        self.run_by_manifest = False
         if suite in ('crashtest', 'jstestbrowser'):
             self.run_by_manifest = False
 
     def _populate_logger(self, options):
         if self.log:
             return
 
         self.log = getattr(options, 'log', None)