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

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jnicol, Assigned: jnicol)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

()

Attachments

(2 attachments)

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?
Flags: needinfo?(mstange)
The reason this affects android and not desktop is because layers.max-active is 20 on android, but is -1 (infinite) on desktop.
Posted file test case
This test case reproduces the problem when layers.max-active is set to 0
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...
Attachment #8864198 - Flags: review?(mstange) → review+
> 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!)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/62c41325d17e
Force FixedPosition display items to be active. r=mstange
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/62c41325d17e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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 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
Attachment #8864198 - Flags: approval-mozilla-beta?
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?
Flags: needinfo?(jnicol)
This is a regression in 54. We shouldn't let 54 ship without this, the content on affected websites will be completely obscured.
Flags: needinfo?(mstange)
Flags: needinfo?(jnicol)
Comment on attachment 8864198 [details]
Bug 1358185 - Force FixedPosition display items to be active.

Fix a regression. Beta54+. Should be in 54 beta 6.
Attachment #8864198 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee: nobody → jnicol
You need to log in before you can comment on or make changes to this bug.