Bug 1451384 - Check IsChanged on the old item during merging, since that's the one that might have a deleted frame. r?mstange draft
authorMatt Woodrow <mwoodrow@mozilla.com>
Thu, 05 Apr 2018 12:20:32 +1200
changeset 777576 d007c2ed83b4d6dfefcba026e1a3848fada40f0c
parent 777575 8a1188fac35bbed425e407bd3dbbbad60e8e2e70
push id105247
push usermwoodrow@mozilla.com
push dateThu, 05 Apr 2018 00:21:02 +0000
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
--- 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;
+  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.
+  MOZ_ASSERT(aList->mOldItems.Length() == aList->mDAG.Length());
   return true;
 RetainedDisplayListBuilder::IncrementSubDocPresShellPaintCount(nsDisplayItem* aItem)
   MOZ_ASSERT(aItem->GetType() == DisplayItemType::TYPE_SUBDOCUMENT);
@@ -252,22 +254,23 @@ public:
   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)) {
         if (aNewItem->GetChildren()) {
           Maybe<const ActiveScrolledRoot*> containerASRForChildren;
           if (mBuilder->MergeDisplayLists(aNewItem->GetChildren(),
-                                          mOldItems[oldIndex.val].mItem->GetChildren(),
+                                          oldItem->GetChildren(),
                                           containerASRForChildren)) {
             mResultIsModified = true;
           UpdateASR(aNewItem, containerASRForChildren);
--- a/layout/tools/reftest/runreftest.py
+++ b/layout/tools/reftest/runreftest.py
@@ -234,17 +234,17 @@ class RefTest(object):
         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:
         self.log = getattr(options, 'log', None)