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

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: roc, Assigned: mstange)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(14 attachments, 1 obsolete attachment)

39 bytes, text/x-review-board-request
roc
: review+
Details
39 bytes, text/x-review-board-request
roc
: review+
Details
39 bytes, text/x-review-board-request
roc
: review+
Details
39 bytes, text/x-review-board-request
roc
: review+
Details
39 bytes, text/x-review-board-request
roc
: review+
Details
39 bytes, text/x-review-board-request
roc
: review+
Details
39 bytes, text/x-review-board-request
roc
: review+
Details
39 bytes, text/x-review-board-request
roc
: review+
Details
39 bytes, text/x-review-board-request
roc
: review+
Details
39 bytes, text/x-review-board-request
roc
: review+
Details
39 bytes, text/x-review-board-request
roc
: review+
Details
39 bytes, text/x-review-board-request
roc
: review+
Details
39 bytes, text/x-review-board-request
roc
: review+
Details
39 bytes, text/x-review-board-request
roc
: review+
Details
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.
Assignee

Comment 1

5 years ago
Stealing.
Assignee: roc → mstange
Status: NEW → ASSIGNED
Assignee

Comment 2

5 years ago
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?
Flags: needinfo?(roc)
Assignee

Updated

5 years ago
Blocks: 852588
That all sounds good to me.
Flags: needinfo?(roc)
Depends on: 1128342
Assignee

Comment 4

4 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05a685c23949
Summary: ContainerState::CreateOrRecycleThebesLayer can recycle the wrong layer when a lThebesLayer is being scrolled out of view → ContainerState::CreateOrRecycleThebesLayer can recycle the wrong layer when a PaintedLayer is being scrolled out of view
Assignee

Updated

4 years ago
Summary: ContainerState::CreateOrRecycleThebesLayer can recycle the wrong layer when a PaintedLayer is being scrolled out of view → ContainerState::CreateOrRecyclePaintedLayer can recycle the wrong layer when a PaintedLayer is being scrolled out of view
Assignee

Comment 6

4 years ago
These patches don't help with bug 1128342, removing the dependency.
No longer depends on: 1128342
Assignee

Comment 7

4 years ago
/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
Attachment #8572836 - Flags: review?(roc)
Comment on attachment 8572836 [details]
MozReview Request: bz://913443/mstange

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

This is really good!
Attachment #8572836 - Flags: review?(roc) → review+
Assignee

Updated

4 years ago
Depends on: 1139863
Assignee

Updated

4 years ago
No longer depends on: 1139863
Assignee

Updated

4 years ago
Depends on: 1141595
Assignee

Updated

4 years ago
Blocks: 1146137
No longer depends on: 1146137
Assignee

Updated

4 years ago
Depends on: 1147083
Assignee

Updated

4 years ago
No longer depends on: 1147083
Assignee

Updated

4 years ago
Depends on: 1147538
Blocking a blocker.
blocking-b2g: --- → 2.2?

Updated

4 years ago
blocking-b2g: 2.2? → ---
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
Flags: needinfo?(khu)
Flags: needinfo?(bchien)

Comment 27

4 years ago
(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?
blocking-b2g: --- → 2.2+
Flags: needinfo?(khu)
Flags: needinfo?(bchien)
Flags: needinfo?(anygregor)
Flags: needinfo?(anygregor)
Assignee

Comment 28

4 years ago
Attachment #8572836 - Attachment is obsolete: true
Attachment #8618020 - Flags: review+
Attachment #8618021 - Flags: review+
Attachment #8618022 - Flags: review+
Attachment #8618023 - Flags: review+
Attachment #8618024 - Flags: review+
Attachment #8618025 - Flags: review+
Attachment #8618026 - Flags: review+
Attachment #8618027 - Flags: review+
Attachment #8618028 - Flags: review+
Attachment #8618029 - Flags: review+
Attachment #8618030 - Flags: review+
Attachment #8618031 - Flags: review+
Attachment #8618032 - Flags: review+
Attachment #8618033 - Flags: review+
You need to log in before you can comment on or make changes to this bug.