Closed Bug 1285619 Opened 4 years ago Closed 4 years ago

We are accumulating ancestor mask layers for PaintedLayers that get optimized to ColorLayers / ImageLayers

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
e10s + ---
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: mstange, Assigned: botond)

References

Details

(Keywords: regression)

Attachments

(4 files)

Attached file testcase
No description provided.
STR:
 1. Make sure APZ and e10s are on.
 2. Turn on display list dumping.
 3. Scroll the scrollframe in the testcase.
 4. Scroll the root scroll frame in the testcase.

You'll see the ColorLayer accumulate a new AncestorMaskLayer with each paint.

What's happening here is that we're reusing an existing ColorLayer and not clearing its existing AncestorMaskLayers, and then in ContainerState::SetupScrollingMetadata we add a new ancestor mask layer to the layer's existing ancestor mask layers.

There are multiple pieces in the code that make use of existing layers and reset their state; in bug 1267438 we missed two of them:
 - ownLayer at http://searchfox.org/mozilla-central/rev/f43c9e0ffa92e72dbdbcbf57eecf04a43d46da63/layout/base/FrameLayerBuilder.cpp#4085
 - ImageLayer / ColorLayer optimized from a PaintedLayer: http://searchfox.org/mozilla-central/rev/f43c9e0ffa92e72dbdbcbf57eecf04a43d46da63/layout/base/FrameLayerBuilder.cpp#3197

It would be nice to have all of those pieces of code go through a generic ResetLayerStateForRecycling function.
Blocks: 1267438
Keywords: regression
Summary: Are we accumulating ancestor mask layers for PaintedLayers that get optimized to ColorLayers / ImageLayers? → We are accumulating ancestor mask layers for PaintedLayers that get optimized to ColorLayers / ImageLayers
(In reply to Markus Stange [:mstange] from comment #1)
> There are multiple pieces in the code that make use of existing layers and
> reset their state; in bug 1267438 we missed two of them:
>  - ownLayer at
> http://searchfox.org/mozilla-central/rev/
> f43c9e0ffa92e72dbdbcbf57eecf04a43d46da63/layout/base/FrameLayerBuilder.
> cpp#4085
>  - ImageLayer / ColorLayer optimized from a PaintedLayer:
> http://searchfox.org/mozilla-central/rev/
> f43c9e0ffa92e72dbdbcbf57eecf04a43d46da63/layout/base/FrameLayerBuilder.
> cpp#3197

I chose call sites slightly upstream from those, because they seemed more appropriate and consistent. Let me know if you agree.

ClearExtraDumpInfo() seems like a candidate for calling in ResetLayerStateForRecycling() too - should I add that?
Assignee: nobody → botond
Priority: -- → P2
Comment on attachment 8769345 [details]
Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function.

https://reviewboard.mozilla.org/r/63288/#review63692

Sorry for taking so long here.

Fundamentally, I think the concept of "recycling a layer" should not be part of of the layers API. It's really just something that consumers of the API do with layers, and it's up to them to decide which parts of a layer's state should be reset and which parts should persist.
So I think this function should live in FrameLayerBuilder, not on Layer. Maybe just a static function in FrameLayerBuilder.cpp.
Attachment #8769345 - Flags: review?(mstange)
Comment on attachment 8769346 [details]
Bug 1285619 - Call ResetLayerStateForRecycling() when recycling an OwnLayer.

https://reviewboard.mozilla.org/r/63290/#review63780
Attachment #8769346 - Flags: review?(mstange) → review+
Comment on attachment 8769347 [details]
Bug 1285619 - Call ResetLayerStateForRecycling() when recycling an image or color layer.

https://reviewboard.mozilla.org/r/63292/#review63782
Attachment #8769347 - Flags: review?(mstange) → review+
Comment on attachment 8769345 [details]
Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63288/diff/1-2/
Attachment #8769345 - Attachment description: Bug 1285619 - Introduce Layer::ResetLayerStateForRecycling(). → Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function.
Attachment #8769345 - Flags: review?(mstange)
Comment on attachment 8769346 [details]
Bug 1285619 - Call ResetLayerStateForRecycling() when recycling an OwnLayer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63290/diff/1-2/
Comment on attachment 8769347 [details]
Bug 1285619 - Call ResetLayerStateForRecycling() when recycling an image or color layer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63292/diff/1-2/
Updated to address review comment.
Attachment #8769345 - Flags: review?(mstange) → review+
Comment on attachment 8769345 [details]
Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function.

https://reviewboard.mozilla.org/r/63288/#review63808
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b64e32f1ceee
Introduce a ResetLayerStateForRecycling() helper function. r=mstange
https://hg.mozilla.org/integration/autoland/rev/2fb6c8087961
Call ResetLayerStateForRecycling() when recycling an OwnLayer. r=mstange
https://hg.mozilla.org/integration/autoland/rev/f4b95c6cfe5f
Call ResetLayerStateForRecycling() when recycling an image or color layer. r=mstange
Comment on attachment 8769345 [details]
Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function.

Let's uplift this to avoid shipping 49 with this regression.

This uplift request applies to all three patches in this bug.

Approval Request Comment
[Feature/regressing bug #]:
  Bug 1267438

[User impact if declined]:
  On pages with certain structures, during scrolling we keep
  adding a new layer to the layer tree on every paint instead 
  of reusing an existing layer. This can result in excessive 
  memory usage and rendering problems.

[Describe test coverage new/current, TreeHerder]:
  Tested manually, and patch has been on m-c since 206-07-26.

[Risks and why]:
  Low risk. This is a small and well-understood change.

[String/UUID change made/needed]:
  None.
Attachment #8769345 - Flags: approval-mozilla-aurora?
Comment on attachment 8769345 [details]
Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function.

This patch fixes a regression. Take it in aurora.
Attachment #8769345 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.