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

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Markus Stange [:mstange]]

Attachment: testcase

Comment 1

[Markus Stange [:mstange]]

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.

Comment 2

[Botond Ballo [:botond]]

Attachment: Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function.

Review commit: https://reviewboard.mozilla.org/r/63288/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63288/

Comment 3

[Botond Ballo [:botond]]

Attachment: Bug 1285619 - Call ResetLayerStateForRecycling() when recycling an OwnLayer.

Review commit: https://reviewboard.mozilla.org/r/63290/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63290/

Comment 4

[Botond Ballo [:botond]]

Attachment: Bug 1285619 - Call ResetLayerStateForRecycling() when recycling an image or color layer.

Review commit: https://reviewboard.mozilla.org/r/63292/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63292/

Comment 5

[Botond Ballo [:botond]]

(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?

Comment 6

[Markus Stange [:mstange]]

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.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8769346 [details]
Bug 1285619 - Call ResetLayerStateForRecycling() when recycling an OwnLayer.

https://reviewboard.mozilla.org/r/63290/#review63780

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8769347 [details]
Bug 1285619 - Call ResetLayerStateForRecycling() when recycling an image or color layer.

https://reviewboard.mozilla.org/r/63292/#review63782

Comment 9

[Botond Ballo [:botond]]

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/

Comment 10

[Botond Ballo [:botond]]

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 11

[Botond Ballo [:botond]]

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/

Comment 12

[Botond Ballo [:botond]]

Updated to address review comment.

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8769345 [details]
Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function.

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

Comment 14

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3aa839a7f52f

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b64e32f1ceee
https://hg.mozilla.org/mozilla-central/rev/2fb6c8087961
https://hg.mozilla.org/mozilla-central/rev/f4b95c6cfe5f

Comment 17

[Botond Ballo [:botond]]

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.

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8769345 [details]
Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function.

This patch fixes a regression. Take it in aurora.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd518331a09d
https://hg.mozilla.org/releases/mozilla-aurora/rev/76fafe444a37
https://hg.mozilla.org/releases/mozilla-aurora/rev/c5fdc568bd3b