The default bug view has changed. See this FAQ.

Large amount of over painting on some pages on fennec

RESOLVED FIXED in mozilla17

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: cwiiis)

Tracking

unspecified
mozilla17
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 639428 [details]
A somewhat simplified test case
(Reporter)

Updated

5 years ago
Blocks: 771374
http://people.mozilla.com/~cjones/grid.html or
http://people.mozilla.com/~dsherk/grid.html
(Reporter)

Comment 3

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

Comment 4

5 years ago
Created attachment 650362 [details]
Another much simplied test case

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

Comment 5

5 years ago
(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
(Reporter)

Comment 6

5 years ago
Created attachment 650706 [details]
A somewhat simplifed test case that reproduces on beta

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

Comment 7

5 years ago
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?
(Assignee)

Comment 8

5 years ago
(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?
(Assignee)

Comment 9

5 years ago
Created attachment 654418 [details] [diff] [review]
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...)
Attachment #654418 - Flags: review?(roc)
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.
(Assignee)

Comment 11

5 years ago
Created attachment 654548 [details] [diff] [review]
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(?)
Attachment #654418 - Attachment is obsolete: true
Attachment #654418 - Flags: review?(roc)
Attachment #654548 - Flags: review?(roc)
Comment on attachment 654548 [details] [diff] [review]
Invalidate only the bounds of newly visible frames (v2)

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

Super.
Attachment #654548 - Flags: review?(roc) → review+
(Assignee)

Comment 13

5 years ago
Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/8bc22ea75c01
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/8bc22ea75c01
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Assignee)

Updated

5 years ago
Depends on: 786740
(Assignee)

Updated

5 years ago
No longer depends on: 786740
You need to log in before you can comment on or make changes to this bug.