Rewrite deleted frame handling to keep a list/hashtable of removed frames
authorMatt Woodrow <mwoodrow@mozilla.com>
Fri, 05 May 2017 14:47:31 +1200
changeset 659470 16cbd632337db3b31ad05f248ba20952f5228e1f
parent 659469 387cfd7fbbbb0bd20723e51318cb75478af3cf30
child 659471 ecce903c804ba25b29904e5703e42a51b52871d1
push id78143
push userbmo:ethlin@mozilla.com
push dateWed, 06 Sep 2017 04:00:32 +0000
milestone55.0a1
Rewrite deleted frame handling to keep a list/hashtable of removed frames This seems like it should be more stable than trying to find and mark all display items belonging to that frame. Hopefully the extra hashtable lookup won't be painfully slow. This is means that Miko's work to make merged display items temporary won't need to change DisplayItemData.
layout/base/PresShell.cpp
layout/base/nsLayoutUtils.cpp
layout/generic/nsIFrame.h
layout/painting/FrameLayerBuilder.cpp
layout/painting/nsDisplayList.cpp
layout/painting/nsDisplayList.h
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -2064,16 +2064,26 @@ PresShell::SetIgnoreFrameDestruction(boo
     mDocument->StyleImageLoader()->ClearFrames(mPresContext);
   }
   mIgnoreFrameDestruction = aIgnore;
 }
 
 void
 PresShell::NotifyDestroyingFrame(nsIFrame* aFrame)
 {
+  nsIFrame* displayRoot = nsLayoutUtils::GetDisplayRootFrame(aFrame);
+  if (displayRoot != aFrame) {
+    nsTArray<nsIFrame*>* deletedFrames = displayRoot->Properties().Get(nsIFrame::DeletedFrameList());
+    if (!deletedFrames) {
+      deletedFrames = new nsTArray<nsIFrame*>;
+      displayRoot->Properties().Set(nsIFrame::DeletedFrameList(), deletedFrames);
+    }
+    deletedFrames->AppendElement(aFrame);
+  }
+
   if (!mIgnoreFrameDestruction) {
     mDocument->StyleImageLoader()->DropRequestsForFrame(aFrame);
 
     mFrameConstructor->NotifyDestroyingFrame(aFrame);
 
     for (int32_t idx = mDirtyRoots.Length(); idx; ) {
       --idx;
       if (mDirtyRoots[idx] == aFrame) {
@@ -2108,16 +2118,18 @@ PresShell::NotifyDestroyingFrame(nsIFram
     mFramesToDirty.RemoveEntry(aFrame);
   } else {
     // We must delete this property in situ so that its destructor removes the
     // frame from FrameLayerBuilder::DisplayItemData::mFrameList -- otherwise
     // the DisplayItemData destructor will use the destroyed frame when it
     // tries to remove it from the (array) value of this property.
     mPresContext->PropertyTable()->
       Delete(aFrame, FrameLayerBuilder::LayerManagerDataProperty());
+    mPresContext->PropertyTable()->
+      Delete(aFrame, nsIFrame::CachedDisplayList());
   }
 }
 
 already_AddRefed<nsCaret> PresShell::GetCaret() const
 {
   RefPtr<nsCaret> caret = mCaret;
   return caret.forget();
 }
--- a/layout/base/nsLayoutUtils.cpp
+++ b/layout/base/nsLayoutUtils.cpp
@@ -3510,34 +3510,35 @@ nsLayoutUtils::ExpireDisplayPortOnAsyncS
     }
   }
 }
 
 // TODO: We currently descend into all children even if we don't have an AGR
 // to mark, as child stacking contexts might. It would be nice if we could
 // jump into those immediately rather than walking the entire thing.
 void MarkFramesForDifferentAGR(nsDisplayListBuilder* aBuilder,
-                                nsDisplayList* aList,
-                                AnimatedGeometryRoot* aAGR)
+                               nsTHashtable<nsPtrHashKey<nsIFrame>>& aDeletedFrames,
+                               nsDisplayList* aList,
+                               AnimatedGeometryRoot* aAGR)
 {
   for (nsDisplayItem* i = aList->GetBottom(); i; i = i->GetAbove()) {
-    if (!i->CanBeRecycled()) {
+    if (aDeletedFrames.Contains(i->Frame())) {
       continue;
     }
 
     if (i->GetChildren()) {
       AnimatedGeometryRoot *childAGR = aAGR;
       if (i->Frame()->IsStackingContext()) {
         if (i->Frame()->HasOverrideDirtyRegion()) {
           childAGR = i->Frame()->Properties().Get(nsDisplayListBuilder::DisplayListBuildingRect())->mModifiedAGR;
         } else {
           childAGR = nullptr;
         }
       }
-      MarkFramesForDifferentAGR(aBuilder, i->GetChildren(), childAGR);
+      MarkFramesForDifferentAGR(aBuilder, aDeletedFrames, i->GetChildren(), childAGR);
     }
 
     // TODO: We should be able to check the clipped bounds relative
     // to the common AGR (of both the existing item and the invalidated
     // frame) and determine if they can ever intersect.
     if (aAGR && i->GetAnimatedGeometryRoot()->GetAsyncAGR() != aAGR) {
       aBuilder->MarkFrameForDisplayIfVisible(i->Frame());
     }
@@ -3558,16 +3559,17 @@ bool IsAnyAncestorModified(nsIFrame* aFr
 
 bool IsSameItem(nsDisplayItem* aFirst, nsDisplayItem* aSecond)
 {
   return aFirst->Frame() == aSecond->Frame() &&
          aFirst->GetPerFrameKey() == aSecond->GetPerFrameKey();
 }
 
 void MergeDisplayLists(nsDisplayListBuilder* aBuilder,
+                       nsTHashtable<nsPtrHashKey<nsIFrame>>& aDeletedFrames,
                        nsDisplayList* aNewList,
                        nsDisplayList* aOldList,
                        nsDisplayList* aOutList)
 {
   nsDisplayList merged;
   nsDisplayItem* old;
 
   std::map<std::pair<nsIFrame*, uint32_t>, nsDisplayItem*> newListLookup;
@@ -3581,25 +3583,25 @@ void MergeDisplayLists(nsDisplayListBuil
     oldListLookup[std::make_pair(i->Frame(), i->GetPerFrameKey())] = i;
   }
 
   while (nsDisplayItem* i = aNewList->RemoveBottom()) {
     // If the new item has a matching counterpart in the old list, copy all items
     // up to that one into the merged list, but discard the repeat.
     if (oldListLookup[std::make_pair(i->Frame(), i->GetPerFrameKey())]) {
       while ((old = aOldList->RemoveBottom()) && !IsSameItem(i, old)) {
-        if (old->CanBeRecycled() &&
+        if (!aDeletedFrames.Contains(old->Frame()) &&
             !IsAnyAncestorModified(old->Frame())) {
 
           // If the old item is in the new list (but further forward), then do the sub-list merging
           // now and delete us, and we'll add the new item when we get to it.
           if (nsDisplayItem* newItem = newListLookup[std::make_pair(old->Frame(), old->GetPerFrameKey())]) {
             if (old->GetChildren()) {
               MOZ_ASSERT(newItem->GetChildren());
-              MergeDisplayLists(aBuilder, newItem->GetChildren(), old->GetChildren(), newItem->GetChildren());
+              MergeDisplayLists(aBuilder, aDeletedFrames, newItem->GetChildren(), old->GetChildren(), newItem->GetChildren());
             }
             oldListLookup.erase(std::make_pair(old->Frame(), old->GetPerFrameKey()));
             old->~nsDisplayItem();
           } else {
             merged.AppendToTop(old);
           }
         } else {
           // TODO: Is it going to be safe to call the dtor on a display item that belongs
@@ -3610,27 +3612,27 @@ void MergeDisplayLists(nsDisplayListBuil
       }
 
       // Recursively merge any child lists.
       // TODO: We may need to call UpdateBounds on any non-flattenable nsDisplayWrapLists
       // here. Is there any other cached state that we need to update?
       MOZ_ASSERT(old && IsSameItem(i, old));
       if (old->GetChildren()) {
         MOZ_ASSERT(i->GetChildren());
-        MergeDisplayLists(aBuilder, i->GetChildren(), old->GetChildren(), i->GetChildren());
+        MergeDisplayLists(aBuilder, aDeletedFrames, i->GetChildren(), old->GetChildren(), i->GetChildren());
       }
 
       old->~nsDisplayItem();
     }
 
     merged.AppendToTop(i);
   }
   
   while ((old = aOldList->RemoveBottom())) {
-    if (old->CanBeRecycled() &&
+    if (!aDeletedFrames.Contains(old->Frame()) &&
         !IsAnyAncestorModified(old->Frame())) {
       merged.AppendToTop(old);
     } else {
       old->~nsDisplayItem();
     }
   }
   
   //printf_stderr("Painting --- Merged list:\n");
@@ -3814,16 +3816,17 @@ nsLayoutUtils::PaintFrame(nsRenderingCon
       nsDisplayListBuilder::AutoCurrentScrollParentIdSetter idSetter(&builder, id);
 
       builder.SetVisibleRect(dirtyRect);
 
       bool merged = false;
       if ((aFlags & PaintFrameFlags::PAINT_WIDGET_LAYERS) &&
           aFrame->Properties().Get(nsIFrame::ModifiedFrameList())) {
         nsTArray<nsIFrame*>* modifiedFrames = aFrame->Properties().Get(nsIFrame::ModifiedFrameList());
+        nsTArray<nsIFrame*>* deletedFrames = aFrame->Properties().Get(nsIFrame::DeletedFrameList());
 
         nsTArray<nsIFrame*> stackingContexts;
 
         AnimatedGeometryRoot* modifiedAGR = nullptr;
         nsRect modifiedDirty;
         bool success = true;
         //printf_stderr("Dirty frames: ");
         for (nsIFrame* f : *modifiedFrames) {
@@ -3904,39 +3907,50 @@ nsLayoutUtils::PaintFrame(nsRenderingCon
         //printf_stderr("\n");
 
         modifiedDirty.IntersectRect(modifiedDirty, dirtyRect);
 
         // TODO: Early return if modified Dirty is empty, and we didn't mark any sub-stacking
         // contexts as needing-paint-if-visible.
 
         if (success) {
-          MarkFramesForDifferentAGR(&builder, &list, modifiedAGR);
+          nsTHashtable<nsPtrHashKey<nsIFrame>> deletions;
+          if (deletedFrames) {
+            for (nsIFrame* f : *deletedFrames) {
+              deletions.PutEntry(f);
+            }
+          }
+
+          MarkFramesForDifferentAGR(&builder, deletions, &list, modifiedAGR);
 
           builder.SetDirtyRect(modifiedDirty);
 
           nsDisplayList modifiedDL;
           aFrame->BuildDisplayListForStackingContext(&builder, &modifiedDL);
           //printf_stderr("Painting --- Modified list (dirty %d,%d,%d,%d):\n",
           //      modifiedDirty.x, modifiedDirty.y, modifiedDirty.width, modifiedDirty.height);
           //nsFrame::PrintDisplayList(&builder, modifiedDL);
           
           builder.LeavePresShell(aFrame, &modifiedDL);
           builder.EnterPresShell(aFrame);
 
-          MergeDisplayLists(&builder, &modifiedDL, &list, &list);
+
+          MergeDisplayLists(&builder, deletions, &modifiedDL, &list, &list);
           merged = true;
         }
 
         // TODO: Do we mark frames as modified during displaylist building? If
         // we do this isn't gonna work.
         for (nsIFrame* f : *modifiedFrames) {
           f->SetFrameIsModified(false);
         }
         modifiedFrames->Clear();
+        if (deletedFrames) {
+          deletedFrames->Clear();
+        }
 
         for (nsIFrame* f: stackingContexts) {
           f->SetHasOverrideDirtyRegion(false);
           f->Properties().Delete(nsDisplayListBuilder::DisplayListBuildingRect());
         }
       }
 
       if (!merged) {
--- a/layout/generic/nsIFrame.h
+++ b/layout/generic/nsIFrame.h
@@ -1170,16 +1170,17 @@ public:
 
   NS_DECLARE_FRAME_PROPERTY_SMALL_VALUE(IBaselinePadProperty, nscoord)
   NS_DECLARE_FRAME_PROPERTY_SMALL_VALUE(BBaselinePadProperty, nscoord)
 
   NS_DECLARE_FRAME_PROPERTY_WITH_DTOR(GenConProperty, ContentArray,
                                       DestroyContentArray)
 
   NS_DECLARE_FRAME_PROPERTY_DELETABLE(ModifiedFrameList, nsTArray<nsIFrame*>)
+  NS_DECLARE_FRAME_PROPERTY_DELETABLE(DeletedFrameList, nsTArray<nsIFrame*>)
 
   NS_DECLARE_FRAME_PROPERTY_DELETABLE(CachedDisplayListBuilder, nsDisplayListBuilder)
   NS_DECLARE_FRAME_PROPERTY_DELETABLE(CachedDisplayList, nsDisplayList)
 
   NS_DECLARE_FRAME_PROPERTY_SMALL_VALUE(BidiDataProperty, mozilla::FrameBidiData)
 
   mozilla::FrameBidiData GetBidiData()
   {
--- a/layout/painting/FrameLayerBuilder.cpp
+++ b/layout/painting/FrameLayerBuilder.cpp
@@ -186,16 +186,17 @@ FrameLayerBuilder::DisplayItemData::Remo
   MOZ_RELEASE_ASSERT(array, "Must be already stored on the frame!");
   array->RemoveElement(this);
 }
 
 void
 FrameLayerBuilder::DisplayItemData::EndUpdate()
 {
   MOZ_RELEASE_ASSERT(mLayer);
+  MOZ_ASSERT(!mItem);
   mIsInvalid = false;
   mUsed = false;
 }
 
 void
 FrameLayerBuilder::DisplayItemData::EndUpdate(nsAutoPtr<nsDisplayItemGeometry> aGeometry)
 {
   MOZ_RELEASE_ASSERT(mLayer);
@@ -203,33 +204,37 @@ FrameLayerBuilder::DisplayItemData::EndU
   MOZ_ASSERT(mGeometry || aGeometry);
 
   if (aGeometry) {
     mGeometry = aGeometry;
   }
   mClip = mItem->GetClip();
   mFrameListChanges.Clear();
 
+  mItem = nullptr;
   EndUpdate();
 }
 
 void
 FrameLayerBuilder::DisplayItemData::BeginUpdate(Layer* aLayer, LayerState aState,
                                                 uint32_t aContainerLayerGeneration,
                                                 nsDisplayItem* aItem /* = nullptr */)
 {
   MOZ_RELEASE_ASSERT(mLayer);
   MOZ_RELEASE_ASSERT(aLayer);
   mLayer = aLayer;
   mOptLayer = nullptr;
   mInactiveManager = nullptr;
   mLayerState = aState;
   mContainerLayerGeneration = aContainerLayerGeneration;
   mUsed = true;
-  mItem = aItem;
+
+  if (aLayer->AsPaintedLayer()) {
+    mItem = aItem;
+  }
 
   if (!aItem) {
     return;
   }
 
   // We avoid adding or removing element unnecessarily
   // since we have to modify userdata each time
   AutoTArray<nsIFrame*, 4> copy(mFrameList);
@@ -1952,28 +1957,16 @@ FrameLayerBuilder::RemoveFrameFromLayerM
         nsIntRegion rgn = old.ScaleToOutsidePixels(paintedData->mXScale, paintedData->mYScale, paintedData->mAppUnitsPerDevPixel);
         rgn.MoveBy(-GetTranslationForPaintedLayer(t));
         paintedData->mRegionToInvalidate.Or(paintedData->mRegionToInvalidate, rgn);
         paintedData->mRegionToInvalidate.SimplifyOutward(8);
       }
     }
 
     data->mParent->mDisplayItems.RemoveEntry(data);
-
-    // TODO: It would be nice to remove this item from the underlying list, but that turns out to be really hard.
-    // It's a singly linked list, so we need the pointer to the list, and the pointer to the previous item (if one
-    // exists). We also need to ensure nothing else deletes our item before we get to here, which can happen if the
-    // display list itself (owned by an outer item) gets destroyed. We need to guarantee the right order of iterating
-    // these items for a given frame (since the outer and inner item can belong to the same fame), as well as the ordering
-    // of frame deleted callbacks. Alternatively we could have circular references and have the display item dtors clear
-    // this data out.
-    // Another option is to change how we manage display items entirely, and make them refcounted.
-    if (data->mItem) {
-      data->mItem->MarkFrameDeleted();
-    }
   }
 
   arrayCopy.Clear();
   delete aArray;
   sDestroyedFrame = nullptr;
 }
 
 void
--- a/layout/painting/nsDisplayList.cpp
+++ b/layout/painting/nsDisplayList.cpp
@@ -2596,17 +2596,16 @@ nsDisplayItem::nsDisplayItem(nsDisplayLi
 nsDisplayItem::nsDisplayItem(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
                              const ActiveScrolledRoot* aActiveScrolledRoot)
   : mFrame(aFrame)
   , mClipChain(aBuilder->ClipState().GetCurrentCombinedClipChain(aBuilder))
   , mClip(DisplayItemClipChain::ClipForASR(mClipChain, aActiveScrolledRoot))
   , mActiveScrolledRoot(aActiveScrolledRoot)
   , mAnimatedGeometryRoot(nullptr)
   , mForceNotVisible(aBuilder->IsBuildingInvisibleItems())
-  , mFrameDeleted(false)
 #ifdef MOZ_DUMP_PAINTING
   , mPainted(false)
 #endif
 {
   mReferenceFrame = aBuilder->FindReferenceFrameFor(aFrame, &mToReferenceFrame);
   // This can return the wrong result if the item override ShouldFixToViewport(),
   // the item needs to set it again in its constructor.
   mAnimatedGeometryRoot = aBuilder->FindAnimatedGeometryRootFor(aFrame);
--- a/layout/painting/nsDisplayList.h
+++ b/layout/painting/nsDisplayList.h
@@ -2185,24 +2185,16 @@ public:
    */
   void FuseClipChainUpTo(nsDisplayListBuilder* aBuilder,
                          const ActiveScrolledRoot* aASR);
 
   bool BackfaceIsHidden() {
     return mFrame->BackfaceIsHidden();
   }
 
-  void MarkFrameDeleted() {
-    mFrameDeleted = true;
-  }
-
-  bool CanBeRecycled() {
-    return !mFrameDeleted;
-  }
-
 protected:
   friend class nsDisplayList;
 
   nsDisplayItem() { mAbove = nullptr; }
 
   nsIFrame* mFrame;
   const DisplayItemClipChain* mClipChain;
   const DisplayItemClip* mClip;
@@ -2215,17 +2207,16 @@ protected:
   // This is the rectangle that needs to be painted.
   // Display item construction sets this to the dirty rect.
   // nsDisplayList::ComputeVisibility sets this to the visible region
   // of the item by intersecting the current visible region with the bounds
   // of the item. Paint implementations can use this to limit their drawing.
   // Guaranteed to be contained in GetBounds().
   nsRect    mVisibleRect;
   bool      mForceNotVisible;
-  bool      mFrameDeleted;
 #ifdef MOZ_DUMP_PAINTING
   // True if this frame has been painted.
   bool      mPainted;
 #endif
 };
 
 /**
  * Manages a singly-linked list of display list items.