771154 - Large amount of over painting on some pages on fennec

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

You can see this problem by turning on paint flashing.
It happens on cnn.com and reddit comment threads. It does not happen on http://people.mozilla.org/~jmuizelaar/

I'll try to make a reduced test case.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: A somewhat simplified test case

Comment 2

[Doug Sherk (:drs) (inactive)]

http://people.mozilla.com/~cjones/grid.html or
http://people.mozilla.com/~dsherk/grid.html

Comment 3

[Jeff Muizelaar [:jrmuizel]]

On the attached test case we end up calling SetInvalidateAllThebesContent() the same happens on cnn.com

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: Another much simplied test case

In this case adding position: relative causes us to repaint everything.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> Created attachment 650362 [details]
> Another much simplied test case
> 
> In this case adding position: relative causes us to repaint everything.

Turns out this case is a recent regression for which I have filed bug 781516

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: A somewhat simplifed test case that reproduces on beta

This test case also has problems with position:relative and happens on beta.

Comment 7

[Chris Lord [:cwiiis]]

The situations where everything is invalidated in this test case seem to be due to the underlying frame changing when merging clip items. We had this same problem with scroll layers in bug 729534. I'll attempt a similar solution, but this does feel like a very delicate situation to me.

Perhaps we should always swap frames when merging? Or perhaps merged frames should share DisplayListItemDataEntry's and the invalid region should be stored there instead of on the frame?

Comment 8

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #7)
> The situations where everything is invalidated in this test case seem to be
> due to the underlying frame changing when merging clip items. We had this
> same problem with scroll layers in bug 729534. I'll attempt a similar
> solution, but this does feel like a very delicate situation to me.

This diagnosis wasn't correct (or, not the whole story) - when I scroll down on this page, at some point some frames appear and cause a full invalidation. Sometimes they disappear shortly after, which I assume is due to the display-port changing size and causing them to be visible only momentarily.

I suppose the real issue here is that any new merged frame becoming visible triggers an entire invalidation of that layer. In this situation, instead of invalidating the entire layer, could we not just invalidate the frame bounds?

Comment 9

[Chris Lord [:cwiiis]]

Attachment: Invalidate only the bounds of newly visible frames

This patch changes the behaviour of a missing ThebesLayerInvalidation property from meaning "invalidate the entire layer" to "invalidate the area of this frame".

I'm not sure if I've done this correctly, or if this is even a valid thing to do, but it does fix a fair few of the bad invalidation cases and it seems to pass tests: https://tbpl.mozilla.org/?tree=Try&rev=64b4d0d9296f (this was looking like it'd end up all green when I last saw it, it's taking a ridiculously long time to complete...)

Comment 10

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

Comment on attachment 654418 [details] [diff] [review]
Invalidate only the bounds of newly visible frames

Review of attachment 654418 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/FrameLayerBuilder.cpp
@@ +2165,3 @@
>    } else {
>      // The region was deleted to indicate that everything should be
>      // invalidated.

"or the frame has just become visible".

@@ +2178,5 @@
> +  }
> +
> +  aState.AddInvalidThebesContent(invalidRegion.
> +    ScaleToOutsidePixels(scaleParameters.mXScale, scaleParameters.mYScale,
> +                         aState.GetAppUnitsPerDevPixel()));

I'm afraid this is going to break invalidation when zooming. See bug 776836.

We might need to add a flag to RefCountedRegion to indicate that the region is infinite and everything should be invalidated, and set that flag where we currently delete the region, and here in BuildContainerLayerFor take the SetInvalidateAllThebesContent path when that flag is set.

Comment 11

[Chris Lord [:cwiiis]]

Attachment: Invalidate only the bounds of newly visible frames (v2)

This adds an mIsInfinite to RefCountedRegion, which gets set when invalidating untrusted geometry (instead of deleting the region). This should mean that the clear-frame-rect code is only executed for newly visible frames(?)

Comment 12

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

Comment on attachment 654548 [details] [diff] [review]
Invalidate only the bounds of newly visible frames (v2)

Review of attachment 654548 [details] [diff] [review]:
-----------------------------------------------------------------

Super.

Comment 13

[Chris Lord [:cwiiis]]

Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/8bc22ea75c01

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8bc22ea75c01