913443 - ContainerState::CreateOrRecyclePaintedLayer can recycle the wrong layer when a PaintedLayer is being scrolled out of view

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If a page contains two ThebesLayers, one vertically above the other, and we scroll down, then when the topmost ThebesLayer scrolls out of view, it will be recycled to render the display items of the second ThebesLayer (since all the display items that previously pulled it off the recycle list are gone), so we'll be forced to reallocate and rerender the remaining visible display items.

Comment 1

[Markus Stange [:mstange]]

Stealing.

Comment 2

[Markus Stange [:mstange]]

Here's my plan:

At the moment, a ThebesLayerData item is assigned a layer on creation. I'd like to change this so that a ThebesLayerData item can be in one of two phases: Phase I without an assigned layer and phase II with a layer. Phase II would be entered as soon as the ThebesLayerData item is assigned a display item for which we know its old layer - at this point, this old layer would be assigned to the ThebesLayerData item. The result of this setup would be that a ThebesLayerData item gets the old layer of the bottommost display item assigned to it which had been assigned to a layer on the previous paint. If the ThebesLayerData item only contains new display items, create a new layer for it. We know that we are done with assigning display items to a ThebesLayerData item when this ThebesLayerData item gets popped in PopThebesLayerData - that's where the new layer would be created.
This setup can create new layers even when there would have been an unused layer available for recycling. But that case can only occur when the old and the new layer share no common display items, so everything would be invalidated anyway.

Implementation notes:
While in phase I, the display items need to be kept somewhere, before they can be streamed to the layer on entering phase II. An array mAssignedDisplayItems on ThebesLayerData should do the trick. Though it looks like we need to store pairs (nsDisplayItem*, LayerState) if we don't want to recompute the layer state for every queued display item when entering phase II.
mNewChildLayers is a bit tricky. We can't add a layer to it on ThebesLayerData creation time because we might not have one at that point. But we also can't just remember the index at which to insert it later when we have the layer, because things can already have shifted underneath us, for example because a ThebesLayerData under us was popped and caused an ImageLayer or ColorLayer to be added to mNewChildLayers. One way to address this would be to make mNewChildLayers instead an array of LayerSlots, where LayerSlot would be
+class LayerSlot {
+  NS_INLINE_DECL_REFCOUNTING(LayerSlot)
+public:
+  LayerSlot(Layer* aLayer) : mLayer(aLayer) {}
+  nsRefPtr<Layer> mLayer;
+};
and each ThebesLayerData item gets a reference to its layer slot, and when it knows its layer it can set it on its slot. That would keep the layer order correct.
Another problem is that FindOpaqueBackgroundColorFor needs to iterate over a ThebesLayerData item's assigned display items, and it needs to be able to do that regardless of whether each ThebesLayerData is in phase I or phase II. So ThebesLayerData should probably expose a common iterator for its display items that encapsulates the phase difference. Or we could keep assigning to ThebesLayerData::mAssignedDisplayItems even during phase II, but that might be expensive.

Does this sound sensible? Did you have something similar in mind? Any other suggestions?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That all sounds good to me.

Comment 4

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05a685c23949

Comment 5

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c60d1b47baf

Comment 6

[Markus Stange [:mstange]]

These patches don't help with bug 1128342, removing the dependency.

Comment 7

[Markus Stange [:mstange]]

Attachment: MozReview Request: bz://913443/mstange

/r/4679 - Bug 913443 - Remove mention of the word ThebesLayer in a comment. r=roc
/r/4681 - Bug 913443 - Remove some #ifdefs. r=roc
/r/4683 - Bug 913443 - Break up CreateOrRecyclePaintedLayer into more parts. r=roc
/r/4685 - Bug 913443 - Extract layer hint calculation. r=roc
/r/4687 - Bug 913443 - Break CreateOrRecyclePaintedLayer up even more. r=roc
/r/4689 - Bug 913443 - Remove unused aItemVisibleRect argument. r=roc
/r/4691 - Bug 913443 - Move IsWidgetLayerManager() check out of UpdateCommonClipCount. r=roc
/r/4693 - Bug 913443 - Change the order of these calls. r=roc
/r/4695 - Bug 913443 - Add a display item buffer for PaintedLayerData so that we can assign items without needing to know the actual Layer. r=roc
/r/4697 - Bug 913443 - Delay PaintedLayer recycling until PopPaintedLayerData(). r=roc
/r/4699 - Bug 913443 - When determining the layer to recycle, only consider layers that have display items in common with the layer we need. r=roc
/r/4701 - Bug 913443 - Recycle PaintedLayers as soon as possible. r=roc
/r/4703 - Bug 913443 - Remove duplicated argument variables. r=roc
/r/4705 - Bug 913443 - Add some tests. r=roc

Pull down these commits:

hg pull review -r fba9fab73e132a6ba1f39f252a8310ab085cf53e

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/4679/#review3821

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/4681/#review3823

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/4683/#review3825

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/4685/#review3827

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/4687/#review3829

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/4689/#review3831

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/4691/#review3833

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/4693/#review3835

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/4695/#review3837

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/4697/#review3839

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/4699/#review3841

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/4701/#review3843

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/4703/#review3845

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/4705/#review3847

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8572836 [details]
MozReview Request: bz://913443/mstange

https://reviewboard.mozilla.org/r/4677/#review3849

This is really good!

Comment 23

[Markus Stange [:mstange]]

Well, that was fun.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a4d7e0a8e55
https://hg.mozilla.org/integration/mozilla-inbound/rev/e73ab1c0c9d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/add34200e05c
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfed9e01504d
https://hg.mozilla.org/integration/mozilla-inbound/rev/396bfdd497cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/66fe954cbf8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa3b47fe9de
https://hg.mozilla.org/integration/mozilla-inbound/rev/c58f78777ab1
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f506b4436e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce500c03c83e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a472533d75f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/04ce9ea7f3d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e30759c8069
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd95d9c12bb

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a4d7e0a8e55
https://hg.mozilla.org/mozilla-central/rev/e73ab1c0c9d1
https://hg.mozilla.org/mozilla-central/rev/add34200e05c
https://hg.mozilla.org/mozilla-central/rev/cfed9e01504d
https://hg.mozilla.org/mozilla-central/rev/396bfdd497cd
https://hg.mozilla.org/mozilla-central/rev/66fe954cbf8c
https://hg.mozilla.org/mozilla-central/rev/eaa3b47fe9de
https://hg.mozilla.org/mozilla-central/rev/c58f78777ab1
https://hg.mozilla.org/mozilla-central/rev/3f506b4436e5
https://hg.mozilla.org/mozilla-central/rev/ce500c03c83e
https://hg.mozilla.org/mozilla-central/rev/a472533d75f3
https://hg.mozilla.org/mozilla-central/rev/04ce9ea7f3d8
https://hg.mozilla.org/mozilla-central/rev/2e30759c8069
https://hg.mozilla.org/mozilla-central/rev/2bd95d9c12bb

Comment 25

[Gregor Wagner [:gwagner]]

Blocking a blocker.

Comment 26

[Gregor Wagner [:gwagner]]

So whats the plan here since you removed the nomination without any explanation? We need this to fix bug 1146137 according to https://bugzilla.mozilla.org/show_bug.cgi?id=1146137#c13

Comment 27

[Bobby Chien]

(In reply to Gregor Wagner [:gwagner] from comment #26)
> So whats the plan here since you removed the nomination without any
> explanation? We need this to fix bug 1146137 according to
> https://bugzilla.mozilla.org/show_bug.cgi?id=1146137#c13

Gregor, I thought it just for mozilla39. I see your point. I noted as 2.2+, could you help for uplift request?

Comment 29

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 913443 - Add some tests. r=roc

Comment 30

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 913443 - Remove mention of the word ThebesLayer in a comment. r=roc

Comment 31

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 913443 - Remove some #ifdefs. r=roc

Comment 32

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 913443 - Break up CreateOrRecyclePaintedLayer into more parts. r=roc

Comment 33

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 913443 - Extract layer hint calculation. r=roc

Comment 34

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 913443 - Break CreateOrRecyclePaintedLayer up even more. r=roc

Comment 35

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 913443 - Remove unused aItemVisibleRect argument. r=roc

Comment 36

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 913443 - Move IsWidgetLayerManager() check out of UpdateCommonClipCount. r=roc

Comment 37

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 913443 - Change the order of these calls. r=roc

Comment 38

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 913443 - Add a display item buffer for PaintedLayerData so that we can assign items without needing to know the actual Layer. r=roc

Comment 39

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 913443 - Delay PaintedLayer recycling until PopPaintedLayerData(). r=roc

Comment 40

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 913443 - When determining the layer to recycle, only consider layers that have display items in common with the layer we need. r=roc

Comment 41

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 913443 - Recycle PaintedLayers as soon as possible. r=roc

Comment 42

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 913443 - Remove duplicated argument variables. r=roc