In order to correctly merge retained display lists, we need to preserve the original display lists before they are ran through FrameLayerBuilder. This bug tracks the progress and stores WIP patches. There are multiple places in FLB code where the list is mutated, but the biggest offender seems to be ContainerState::ProcessDisplayItems(), called by FrameLayerBuilder::BuildContainerLayerFor(). Merging performed in ProcessDisplayItems() is especially harmful, since it both removes the items from the list and destroys/mutates the original version. Despite the relatively simple merging code, this also seems to be rather difficult to fix with the current nsDisplayList implementation, which tightly couples the nsDisplayList linked list nodes and the nsDisplayItems contained (through inheritance) in them.

This is a very WIP patch that refactors nsDisplayList (and all the code where display item based iteration is used) to use nsDisplayLinks instead of nsDisplayItems directly. The display list processing is done using a temporary std::list, which works around some memory management problems. There will still be double-frees since the merging items copies items between the display lists.

Another approach to merging could be to create a wrapper class for each merged item. The wrapper class would hold a pointer to the display list owned by the merged item (subclass of nsDisplayWrapList which has mList). These wrapper items would then be flattened when the new merged item is processed. A graph to illustrate this idea: https://pastebin.mozilla.org/9019947 (Thanks Matt). I am currently trying to implement this version as well, because it would most likely result in far fewer code changes. There are a couple of problems with this approach too, such as limiting merging only to subclasses of nsDisplayWrapList, and having to deal with incomplete type information.

Another approach to immutable merging and flattening in FLB. Unfortunately, this approach breaks some reftests that require nsDisplayOpacity flattening. This flattening mutates the items though, so we probably want to get rid of at some point anyway. Here is a list of broken reftests (most with 1-2 pixel difference): layout/reftests/box-shadow/boxshadow-opacity.html layout/reftests/bugs/759036-1.html layout/reftests/bugs/797797-1.html layout/reftests/bugs/797797-2.html layout/reftests/bugs/991046-1.html layout/reftests/text/475092-pos.html

It seems that preventing FLB from mutating display items would require a lot of refactoring. The current solution is to work around this by storing the pre-FLB display item state, and restoring it before merging the old and modified display lists together. The first version of this is currently in the WIP repo: https://hg.mozilla.org/users/mwoodrow_mozilla.com/retained-dl/. Implementing saved display item state also made it possible to restore nsDisplayOpacity flattening and fix the broken reftests. We probably still want to fix the underlying bug causing non-flattening and flattening nsDisplayOpacities to result in different rendering result.

This work currently resides in the graphics project repository. Most of the obvious FLB display item state changes are reverted by duplicating the state when it changes and restoring it when the item is reused. This made it possible to fix the regressions with nsDisplayOpacity.

Blocks what was a qf:p1 bug.

I talked to Matt, retained display lists are unlikely to land in 57. Marking accordingly.

Comment on attachment 8901605 [details] Bug 1359584 - Part 1: Move mDisableSubpixelAA to nsDisplayItem https://reviewboard.mozilla.org/r/173002/#review179078

Comment on attachment 8901607 [details] Bug 1359584 - Part 3: Improve nsDisplayItem const correctness and fix surrounding whitespace https://reviewboard.mozilla.org/r/173006/#review179080

Comment on attachment 8901608 [details] Bug 1359584 - Part 4: Refactor FrameLayerBuilder::ProcessDisplayItems() to be recursive https://reviewboard.mozilla.org/r/173008/#review179082

Comment on attachment 8901609 [details] Bug 1359584 - Part 5: Change nsDisplayItem merging logic to non-destructive https://reviewboard.mozilla.org/r/173010/#review179450 ::: gfx/layers/wr/WebRenderLayerManager.cpp:207 (Diff revision 1) > + nsTArray<nsDisplayItem*>& aMergedItems) > +{ > + nsDisplayItem* merged = nullptr; > + > + // Clone the last item in the aMergedItems list and merge the items into it. > + for (nsDisplayItem* item : Reversed(aMergedItems)) { Does this put the items in the right order? Or does the order not matter? I think the code would be a little more readable if it looked like this: size_t lastElementIndex = aMergedItems.Length() - 1; nsDisplayItem* lastItem = aMergedItems[lastElementIndex]; aMergedItems.RemoveElementAt(lastElementIndex); nsDisplayItem* merged = lastItem->Clone(aBuilder); MOZ_ASSERT(merged); aBuilder->AddTemporaryItem(merged); merged->MergeDisplayListFromItem(lastItem); for (nsDisplayItem* item : aMergedItems) { merged->MergeDisplayListFromItem(aBuilder, item); } ... but now that I see the code in this form, I'm not that sure anymore that I prefer it. At least this form makes it obvious that we're calling MergeDisplayListFromItem even with the display item that we cloned from. Is that intentional? If it is, maybe Clone() should merge the original display item in automatically? ::: layout/painting/nsDisplayList.h:4009 (Diff revision 1) > + > + /** > + * Creates a new nsDisplayWrapList that holds a pointer to the display list > + * owned by the given nsDisplayItem. > + */ > + virtual void MergeDisplayListFromItem(nsDisplayListBuilder* aBuilder, These methods, and the CanMerge methods, are a bit long to put them inline into nsDisplayList.h, I think. I'd be happier if they were defined in nsDisplayList.cpp. nsDisplayList.h is already rather long, and it gets (transitively) included into many .cpp files. ::: layout/painting/nsDisplayList.cpp:6037 (Diff revision 1) > { > return true; > } > > +static bool > +CopyItemsWithOpacity(nsDisplayList* aList, I don't really understand this function. It seems to gather all descendants of aList into an array, but it doesn't actually do any copying, as far as I can tell (there are no calls to i->Clone()). ::: layout/painting/nsDisplayList.cpp:6067 (Diff revision 1) > + > + return true; > +} > + > bool > nsDisplayOpacity::ShouldFlattenAway(nsDisplayListBuilder* aBuilder) This part seems like it could have gone in a separate patch. ::: layout/painting/nsDisplayList.cpp (Diff revision 1) > return Nothing(); > } > > -/* If UNIFIED_CONTINUATIONS is defined, we can merge two display lists that > - * share the same underlying content. Otherwise, doing so results in graphical > - * glitches. Do you know if the reverse is also true? E.g. will there be graphical glitches if UNIFIED_CONTINUATIONS is defined and continuations are *not* merged? Maybe we should remove all traces of UNIFIED_CONTINUATIONS now, if this patch is removing a crucial part of it. I don't know if there's any value in keeping UNIFIED_CONTINUATIONS around; maybe Matt can comment on that.

Matt, see the last sentence of comment 16 - do you have any opinion on what to do with UNIFIED_CONTINUATIONS?

I don't know of any plans to try spec or implement UNIFIED_CONTINUATIONS properly, so ripping it out seems reasonable. There's really not much code there, and we can always find it again in the history if need be.

Comment on attachment 8901609 [details] Bug 1359584 - Part 5: Change nsDisplayItem merging logic to non-destructive https://reviewboard.mozilla.org/r/173010/#review179450 > Does this put the items in the right order? Or does the order not matter? > > I think the code would be a little more readable if it looked like this: > > size_t lastElementIndex = aMergedItems.Length() - 1; > nsDisplayItem* lastItem = aMergedItems[lastElementIndex]; > aMergedItems.RemoveElementAt(lastElementIndex); > > nsDisplayItem* merged = lastItem->Clone(aBuilder); > MOZ_ASSERT(merged); > aBuilder->AddTemporaryItem(merged); > merged->MergeDisplayListFromItem(lastItem); > > for (nsDisplayItem* item : aMergedItems) { > merged->MergeDisplayListFromItem(aBuilder, item); > } > > ... but now that I see the code in this form, I'm not that sure anymore that I prefer it. At least this form makes it obvious that we're calling MergeDisplayListFromItem even with the display item that we cloned from. Is that intentional? If it is, maybe Clone() should merge the original display item in automatically? It is unclear whether the order matters here, I replicated the previous behavior (merging with the item above) to avoid hard to find bugs/regressions. MergeDisplayListFromItem() creates a new nsDisplayWrapList item that contains a pointer to the display list of the merged item, this item is then added to the local list. Clone() calls the derived class copy-constructor to ensure that we create a new temporary item with, for example, right bounds/frames. Merge() copies, again bounds/frames/etc, from the merged item to the previously created temporary item. Although all three are currently only used for merging, I think it would be nice to keep these separated for clarity. > These methods, and the CanMerge methods, are a bit long to put them inline into nsDisplayList.h, I think. I'd be happier if they were defined in nsDisplayList.cpp. nsDisplayList.h is already rather long, and it gets (transitively) included into many .cpp files. Makes sense, I will move them there. > I don't really understand this function. It seems to gather all descendants of aList into an array, but it doesn't actually do any copying, as far as I can tell (there are no calls to i->Clone()). Maybe a comment is needed here. The item pointers are copied to the given temporary array, after which opacity is applied to them. The behavior is similar to for-loop that was used before, but nested nsDisplayWrapLists are supported. > Do you know if the reverse is also true? E.g. will there be graphical glitches if UNIFIED_CONTINUATIONS is defined and continuations are *not* merged? > > Maybe we should remove all traces of UNIFIED_CONTINUATIONS now, if this patch is removing a crucial part of it. I don't know if there's any value in keeping UNIFIED_CONTINUATIONS around; maybe Matt can comment on that. Comment 18

Comment on attachment 8901609 [details] Bug 1359584 - Part 5: Change nsDisplayItem merging logic to non-destructive https://reviewboard.mozilla.org/r/173010/#review179450 > Maybe a comment is needed here. The item pointers are copied to the given temporary array, after which opacity is applied to them. The behavior is similar to for-loop that was used before, but nested nsDisplayWrapLists are supported. But ApplyOpacity mutates display items, doesn't it? Don't you have to clone these items before you call ApplyOpacity? Or somehow revert the applied opacity afterwards? I also don't understand why the change to check nested display items is necessary. And I think it's even incorrect in some cases; for example you could have an nsDisplayFilter for a feFlood SVG filter which generates a color. The opacity would need to apply to this color as well, not just to the items that are contained inside the nsDisplayFilter.

(In reply to Markus Stange [:mstange] from comment #25) > Comment on attachment 8901609 [details] > Bug 1359584 - Part 5: Change nsDisplayItem merging logic to non-destructive > > https://reviewboard.mozilla.org/r/173010/#review179450 > > > Maybe a comment is needed here. The item pointers are copied to the given temporary array, after which opacity is applied to them. The behavior is similar to for-loop that was used before, but nested nsDisplayWrapLists are supported. > > But ApplyOpacity mutates display items, doesn't it? Don't you have to clone > these items before you call ApplyOpacity? Or somehow revert the applied > opacity afterwards? Yes, this is correct and unfortunate. In the retained-dl branch we are currently storing the initial state of the item and restoring it if we reuse the item. Hopefully we can fix this properly in the future. > I also don't understand why the change to check nested display items is > necessary. This is needed to handle the case where we perform both merging and flattening of nsDisplayOpacity. Consider a case where we merge three consecutive nsDisplayOpacities, each with one child. After merging we have a new temporary nsDisplayOpacity item with a child display list that contains three nsDisplayWrapLists, each pointing to the display list of the merged (original) nsDisplayOpacities. When we then flatten the new temporary item, we have to collect the original display items from the nested lists. Some reftests rely on this behavior. > And I think it's even incorrect in some cases; for example you > could have an nsDisplayFilter for a feFlood SVG filter which generates a > color. The opacity would need to apply to this color as well, not just to > the items that are contained inside the nsDisplayFilter. I think you are right. Can you think of cases that break if we only descend into nsDisplayWrapLists?

Comment on attachment 8901609 [details] Bug 1359584 - Part 5: Change nsDisplayItem merging logic to non-destructive https://reviewboard.mozilla.org/r/173010/#review182722 ::: layout/painting/nsDisplayList.h:4007 (Diff revision 2) > nsDisplayItem::Destroy(aBuilder); > } > + > + /** > + * Creates a new nsDisplayWrapList that holds a pointer to the display list > + * owned by the given nsDisplayItem. Please also say that aItem's contents will be inserted at the *bottom* of this item's contents. ::: layout/painting/nsDisplayList.h:4192 (Diff revision 2) > #endif > > + virtual nsDisplayWrapList* Clone(nsDisplayListBuilder* aBuilder) const override > + { > + MOZ_COUNT_CTOR(nsDisplayOpacity); > + return new (aBuilder) nsDisplayOpacity(*this); I'm trying to understand the full implications of this copy constructor. This is using the default copy constructor, correct? So all fields will simply be copy constructed from the fields of the other instance. So the default copy constructor of the nsDisplayWrapList will copy-construct mList and mListPtr, among others. So mListPtr of the new item will usually point to mList of the original item. What does copy-constructing mList do? It will copy-construct mSentinel and mTop. So if you copy-construct an empty list, mTop will point to mSentinel of the original list, and IsEmpty() of the new list will return false. Am I right so far? Or am I missing something? I'm not sure if this all makes an actual difference, because we probably won't actually end up using the bad data. But it would be nice if all this had more well-defined and intentional behavior. I think we need explicit copy constructors for nsDisplayList and nsDisplayWrapList + its subclasses, at least. ::: layout/painting/nsDisplayList.cpp:5733 (Diff revision 2) > + > + // Set the display list pointer of the new wrapper item to the display list > + // of the wrapped item. > + wrapper->mListPtr = wrappedItem->mListPtr; > + > + mListPtr->AppendNewToBottom(wrapper); Let's use AppendToBottom instead of AppendNewToBottom here. You're already relying on wrapper being non-null here. It looks like nsDisplayListBuilder uses infallible allocation. I'm not sure why nsDisplayList even has mechanisms for these null checks.

(In reply to Miko Mynttinen [:miko] from comment #26) > (In reply to Markus Stange [:mstange] from comment #25) > > Comment on attachment 8901609 [details] > > Bug 1359584 - Part 5: Change nsDisplayItem merging logic to non-destructive > > > > https://reviewboard.mozilla.org/r/173010/#review179450 > > > > > Maybe a comment is needed here. The item pointers are copied to the given temporary array, after which opacity is applied to them. The behavior is similar to for-loop that was used before, but nested nsDisplayWrapLists are supported. > > > > But ApplyOpacity mutates display items, doesn't it? Don't you have to clone > > these items before you call ApplyOpacity? Or somehow revert the applied > > opacity afterwards? > Yes, this is correct and unfortunate. In the retained-dl branch we are > currently storing the initial state of the item and restoring it if we reuse > the item. Hopefully we can fix this properly in the future. Ok, please at least add a comment about this. > > I also don't understand why the change to check nested display items is > > necessary. > This is needed to handle the case where we perform both merging and > flattening of nsDisplayOpacity. > > Consider a case where we merge three consecutive nsDisplayOpacities, each > with one child. After merging we have a new temporary nsDisplayOpacity item > with a child display list that contains three nsDisplayWrapLists, each > pointing to the display list of the merged (original) nsDisplayOpacities. > When we then flatten the new temporary item, we have to collect the original > display items from the nested lists. I see. In today's world, if you have an nsDisplayOpacity that wraps an nsDisplayWrapList that contains an nsDisplayText, we already hit a case where we can't flatten the opacity. Is that right? > Some reftests rely on this behavior. In what way? Could they be fuzzed? I'd prefer the simplicity of just not flattening opacity if there are nsDisplayWrapLists inside. > > > And I think it's even incorrect in some cases; for example you > > could have an nsDisplayFilter for a feFlood SVG filter which generates a > > color. The opacity would need to apply to this color as well, not just to > > the items that are contained inside the nsDisplayFilter. > I think you are right. Can you think of cases that break if we only descend > into nsDisplayWrapLists? I can't, but how would you differentiate nsDisplayWrapList items from items that use a subclass of nsDisplayWrapList? Using a virtual method like CanFlattenOpacityToContents? Please also add a test for the feFlood case which fails with your current patch.

Comment on attachment 8901609 [details] Bug 1359584 - Part 5: Change nsDisplayItem merging logic to non-destructive https://reviewboard.mozilla.org/r/173010/#review182740 ::: gfx/layers/wr/WebRenderLayerManager.cpp:201 (Diff revision 2) > aTarget.GetLayerDataMutable(index)->Initialize(aTarget, aLayer, descendants); > return descendants + 1; > } > > +static nsDisplayItem* > +MergeItems(nsDisplayListBuilder* aBuilder, Please share this function with the one in FrameLayerBuilder. You could make it a method on nsDisplayListBuilder, for example.

(In reply to Markus Stange [:mstange] from comment #28) > In today's world, if you have an nsDisplayOpacity that wraps an > nsDisplayWrapList that contains an nsDisplayText, we already hit a case > where we can't flatten the opacity. Is that right? Yeah, for example we don't flatten in this testcase: data:text/html,<div style="opacity:0.5"><div style="position:relative;z-index:2">Text

(In reply to Markus Stange [:mstange] from comment #27) > Please also say that aItem's contents will be inserted at the *bottom* of > this item's contents. Done. > Let's use AppendToBottom instead of AppendNewToBottom here. You're already > relying on wrapper being non-null here. Done. (In reply to Markus Stange [:mstange] from comment #28) In the retained-dl branch we are > > currently storing the initial state of the item and restoring it if we reuse > > the item. Hopefully we can fix this properly in the future. > > Ok, please at least add a comment about this. I think a comment regarding this might be a bit misleading since none of this logic is in the tree yet. > In today's world, if you have an nsDisplayOpacity that wraps an > nsDisplayWrapList that contains an nsDisplayText, we already hit a case > where we can't flatten the opacity. Is that right? As per IRC discussion, I've made the sublist descending more explicitly only for wraplists. Matt preferred this flattening logic to what we had before. > Please also add a test for the feFlood case which fails with your current > patch. In this case, we bail out before applying opacity since feFlood filter fails the type check after iterating through the children. (In reply to Markus Stange [:mstange] from comment #29) > Please share this function with the one in FrameLayerBuilder. You could make > it a method on nsDisplayListBuilder, for example. Done.

Comment on attachment 8901606 [details] Bug 1359584 - Part 2: Use display list through a pointer in nsDisplayWrapList https://reviewboard.mozilla.org/r/173004/#review184494

Comment on attachment 8901609 [details] Bug 1359584 - Part 5: Change nsDisplayItem merging logic to non-destructive https://reviewboard.mozilla.org/r/173010/#review184496 ::: layout/painting/nsDisplayList.h:3986 (Diff revision 3) > mListPtr = &mList; > } > + > + /** > + * A custom copy-constructor that does not copy mList, as this would mutate > + * the other item. Much better, thanks. ::: layout/painting/nsDisplayList.cpp:881 (Diff revision 3) > + if (!merged) { > + // Create the temporary item. > + merged = item->Clone(this); > + MOZ_ASSERT(merged); > + > + this->AddTemporaryItem(merged); We usually drop the "this->" :) ::: layout/painting/nsDisplayList.cpp:892 (Diff revision 3) > + } > + > + // Create nsDisplayWrapList that points to the internal display list of the > + // item we are merging. This nsDisplayWrapList is added to the display list > + // of the temporary item. > + merged->MergeDisplayListFromItem(this, item); I wonder if it would be better to do this work in the caller, since this is the only caller. You could have something like: mergedAsWrapList = merged->AsWrapList(); ... if (mergedAsWrapList) { MOZ_ASSERT(item->AsWrapList()); // We also need to merge the child list from item // into mergedAsWrapList. and then the code from MergeDisplayListFromItem here } This would make it clearer what kinds of display items are created during this function.

Comment on attachment 8901609 [details] Bug 1359584 - Part 5: Change nsDisplayItem merging logic to non-destructive https://reviewboard.mozilla.org/r/173010/#review182722 > I'm trying to understand the full implications of this copy constructor. > > This is using the default copy constructor, correct? So all fields will simply be copy constructed from the fields of the other instance. > So the default copy constructor of the nsDisplayWrapList will copy-construct mList and mListPtr, among others. So mListPtr of the new item will usually point to mList of the original item. > What does copy-constructing mList do? It will copy-construct mSentinel and mTop. So if you copy-construct an empty list, mTop will point to mSentinel of the original list, and IsEmpty() of the new list will return false. > Am I right so far? Or am I missing something? > > I'm not sure if this all makes an actual difference, because we probably won't actually end up using the bad data. But it would be nice if all this had more well-defined and intentional behavior. I think we need explicit copy constructors for nsDisplayList and nsDisplayWrapList + its subclasses, at least. Replying to my own comment: This is using the default copy constructor of nsDisplayOpacity. That default copy constructor first calls the copy constructor of nsDisplayWrapList. nsDisplayWrapList has a custom copy constructor which does *not* copy-construct mList.

Pushed by mikokm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ee9226d2253b Part 1: Move mDisableSubpixelAA to nsDisplayItem r=mstange https://hg.mozilla.org/integration/autoland/rev/475a8b4fd94b Part 2: Use display list through a pointer in nsDisplayWrapList r=mstange https://hg.mozilla.org/integration/autoland/rev/0b9b457392d5 Part 3: Improve nsDisplayItem const correctness and fix surrounding whitespace r=mstange https://hg.mozilla.org/integration/autoland/rev/575c0a07f75d Part 4: Refactor FrameLayerBuilder::ProcessDisplayItems() to be recursive r=mstange https://hg.mozilla.org/integration/autoland/rev/7bdf8f58bb8b Part 5: Change nsDisplayItem merging logic to non-destructive r=mstange

https://hg.mozilla.org/mozilla-central/rev/ee9226d2253b https://hg.mozilla.org/mozilla-central/rev/475a8b4fd94b https://hg.mozilla.org/mozilla-central/rev/0b9b457392d5 https://hg.mozilla.org/mozilla-central/rev/575c0a07f75d https://hg.mozilla.org/mozilla-central/rev/7bdf8f58bb8b