1358185 - Content incorrectly clipped on fennec on http://www.zpravy.cz/

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jamie Nicol [:jnicol]]

On fennec nightly the page http://www.zpravy.cz/ does not display correctly. None of the main content is shown. When scrolling some content becomes temporarily visible then is hidden again.

I have bisected it to this commit:
https://hg.mozilla.org/mozilla-central/rev/5746c9b9db68fc00cce51b6cd873f403e0caaee6

I don't really understand what's going on here, lots of this code is new to me. Markus?

Comment 1

[Jamie Nicol [:jnicol]]

The reason this affects android and not desktop is because layers.max-active is 20 on android, but is -1 (infinite) on desktop.

Comment 2

[Jamie Nicol [:jnicol]]

Attachment: test case

This test case reproduces the problem when layers.max-active is set to 0

Comment 3

[Jamie Nicol [:jnicol]]

Attachment: Bug 1358185 - Force FixedPosition display items to be active.

Fixed backgrounds were not being clipped correctly when rendered using
inactive layers.

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

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8864198 [details]
Bug 1358185 - Force FixedPosition display items to be active.

https://reviewboard.mozilla.org/r/135862/#review138896

Let's see how long it takes before we notice that this is causing OOM crashes.

In theory, it shouldn't be too hard to apply the clip properly in FrameLayerBuilder even if the item is inactive. But then it would render incorrectly during async scrolling...

Comment 5

[Jamie Nicol [:jnicol]]

> Let's see how long it takes before we notice that this is causing OOM crashes.

Heh :). This might actually help us in some cases. On the site in question we were getting a 2 PaintedLayers per inactive FixedPosition. Because the FixedPositions had a huge clip and a different AGR from the rest of the content, they would finalize the existing PaintedLayerDataNode, then be finalized themselves when the next display item was processed. (I think that's what was going on anyway!)

Comment 6

[Jamie Nicol [:jnicol]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9370e04ccea6bfbfd8b94b9c8ebaa2d97db40af0

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62c41325d17e
Force FixedPosition display items to be active. r=mstange

Comment 8

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/62c41325d17e

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

this shows a performance improvement:
== Change summary for alert #6348 (as of May 03 2017 19:57 UTC) ==

Improvements:

 14%  tscrollx summary linux64 opt      5.01 -> 4.31

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6348

Comment 10

[Jamie Nicol [:jnicol]]

Comment on attachment 8864198 [details]
Bug 1358185 - Force FixedPosition display items to be active.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1298218
[User impact if declined]: On android some sites will be totally obscured by a background image, instead of the image being clipped to the correct size
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: Not particularly
[Why is the change risky/not risky?]: The change makes us take what was already the common code-path all of the time, so is well tested. Layerization changes can occasionally have unforeseen consequences with regards to memory usage, but I believe this change is likely to help decrease usage as described in comment 5.
[String changes made/needed]: N/A

Comment 11

[Gerry Chang [:gchang]]

Hi :jnicol, 
Is this a regression or it's been there for a while? If it's not a new regression in 54, can we let it ride the train?

Comment 12

[Jamie Nicol [:jnicol]]

This is a regression in 54. We shouldn't let 54 ship without this, the content on affected websites will be completely obscured.

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8864198 [details]
Bug 1358185 - Force FixedPosition display items to be active.

Fix a regression. Beta54+. Should be in 54 beta 6.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a849ff78181f