1192910 - Still scheduling paints when scrolling near the top or the bottom of the page

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Markus Stange [:mstange]]

Scrolling in the center of the scrollable range is fine, but scrolling around the top and bottom is not. That's probably because we're clamping the display port to the scrollable range, so maybe the margins change relative to the scroll position?

Comment 1

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1192910 - When modifying display port margins, don't schedule paints if the actual display port hasn't changed. r?mattwoodrow

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

Comment 2

[Markus Stange [:mstange]]

This makes layout/reftests/scrolling/scroll-behavior-6.html fail.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8703147 [details]
MozReview Request: Bug 1192910 - When modifying display port margins, don't schedule paints if the actual display port hasn't changed. r?mattwoodrow

https://reviewboard.mozilla.org/r/29297/#review26949

Comment 4

[Timothy Nikkel (:tnikkel)]

https://reviewboard.mozilla.org/r/29297/#review27191

::: layout/base/nsLayoutUtils.cpp:1114
(Diff revision 1)
> +  GetDisplayPort(aContent, &oldDisplayPort);

Not sure if this is causing that test to fail, but you'll probably need to check if you had a displayport here, otherwise it'll just leave oldDisplayPort untouched, and so comparing it later doesn't make much sense.

Comment 6

[Botond Ballo [:botond]]

This patch will need rebasing across bug 1241371.

Comment 7

[Botond Ballo [:botond]]

Attachment: Patch rebased across bug 1241371

(In reply to Botond Ballo [:botond] from comment #6)
> This patch will need rebasing across bug 1241371.

I rebased it locally, posting in case it's useful for someone.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Stealing; I forgot there was already a patch here and ended up reinventing it. Since all of {mstange, mattwoodrow, tnikkel, botond} have been involved in this patch already I'm going to assume there are no objections here. I'll get the tests passing and land it.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Getting the scroll-behavior-6 test passing turned out to be a more nontrivial than I had thought. The way reftests work is that they listen for paints on the main thread and update their snapshot based on that. So if we skip paints (which is what this patch does) then the reftest snapshot is potentially stale (which is what was happening). Specifically, the reftest did a smooth-scroll animation with a 500ms timeout to wait for it to finish. The animation did in fact finish just fine, but because it didn't trigger main-thread paints, it didn't update the snapshot in the reftest harness.

I have a fix which seems to work - it basically schedules a paint as part of the APZ flush which happens at the end of every reftest. Conceptually this feels correct, because it's APZ code that's leaving the main thread in an "unpainted" state, and the APZ flush is conceptually supposed to stop that from happening. So extending the APZ flush to cover this scenario seems reasonable.

There's a couple of other test failures that I'm working through as well.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Ok, https://treeherder.mozilla.org/#/jobs?repo=try&revision=a77318030ae1&group_state=expanded is looking good. Patches coming.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Don't schedule a paint if the displayport position didn't change

The main patch. It's equivalent to what Markus had originally, so I kept Markus as the author and r=me.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Schedule a paint after doing an APZ flush

This fixes the tests, by compensating for the fact that we now sometimes skip paints.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a56c6dc4c7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9066ef314419

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out because the OS X 10.10 R-e10s(R-e10s) test permafailed after this push.

Backouts:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c68584f32e4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7eb03c4cb9

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9066ef314419
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=23173671&repo=mozilla-inbound

14:46:24     INFO -  REFTEST TEST-START | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html
14:46:24     INFO -  REFTEST TEST-LOAD | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html | 13243 / 13292 (99%)
14:46:24     INFO -  REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html | failed reftest-no-paint
14:46:24     INFO -  REFTEST TEST-END | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html
14:46:24     INFO -  REFTEST INFO | Saved log: START file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] OnDocumentLoad triggering WaitForTestEnd
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] WaitForTestEnd: Adding listeners
14:46:24     INFO -  REFTEST INFO | Saved log: Initializing canvas snapshot
14:46:24     INFO -  REFTEST INFO | Saved log: DoDrawWindow 0,0,800,1000
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_TO_FIRE_INVALIDATE_EVENT
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: waiting for MozAfterPaint
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] AfterPaintListener in file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] SendUpdateCanvasForEvent with 1 rects
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] Rect: 0 0 800 1000
14:46:24     INFO -  REFTEST INFO | Saved log: Updating canvas for invalidation
14:46:24     INFO -  REFTEST INFO | Saved log: DoDrawWindow 0,0,800,1000
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_TO_FIRE_INVALIDATE_EVENT
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: dispatching MozReftestInvalidate
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] AttrModifiedListener fired
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_REFTEST_WAIT_REMOVAL
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_SPELL_CHECKS
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_APZ_FLUSH
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: done requesting APZ flush
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_APZ_FLUSH
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] AfterPaintListener in file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] SendUpdateCanvasForEvent with 1 rects
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] Rect: 0 -100 800 1536
14:46:24     INFO -  REFTEST INFO | Saved log: Updating canvas for invalidation
14:46:24     INFO -  REFTEST INFO | Saved log: DoDrawWindow 0,0,800,1000
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_APZ_FLUSH
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: apz-repaints-flushed fired
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_TO_FINISH
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: waiting for MozAfterPaint
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] AfterPaintListener in file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] SendUpdateCanvasForEvent with 1 rects
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] Rect: 0 1436 785 2230
14:46:24     INFO -  REFTEST INFO | Saved log: Updating canvas for invalidation
14:46:24     INFO -  REFTEST INFO | Saved log: DoDrawWindow 0,1000,785,0
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_TO_FINISH
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: Completed
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] RecordResult fired
14:46:24     INFO -  REFTEST INFO | Saved log: RecordResult fired
14:46:24     INFO -  REFTEST INFO | Saved log: RecordResult fired

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8abae7731417
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c232821ae5c

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Relanded the patches because the backout didn't fix the issue.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

It looks like the backout *did* fix the issue; the R-e10s failures that came after the backout were some other failure.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Much reftest bustage in the tree, so I missed that. Backed out again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/950506733b03
https://hg.mozilla.org/integration/mozilla-inbound/rev/14838e17cfaf

Comment 19

[Kartikaya Gupta (email:[e-mail])]

I have a patch for bug 1253690 that should take care of this. Will reland this after that's in.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

I put 1253690 on inbound, so I'm going to reland this. Hopefully it'll stick this time...

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a295c8a814b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e9a803d69ed

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2a295c8a814b
https://hg.mozilla.org/mozilla-central/rev/3e9a803d69ed

Comment 23

[Kartikaya Gupta (email:[e-mail])]

This is a perf improvement but not worth uplifting to beta.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8727170 [details] [diff] [review]
Part 1 - Don't schedule a paint if the displayport position didn't change

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: when scrolling near the top or bottom of pages, we do extra repaints for no good reason. Wastes CPU time and power.
[Describe test coverage new/current, TreeHerder]: this code is exercised quite a bit during normal browser usage and automated tests if APZ is enabled.
[Risks and why]: low risk, change is well contained. The biggest risk is that it might expose bugs that were previously not as obvious (which IMO is a good thing) that might look bad. Bug 1255068 is an example of this and so far the only one. (I would like that bug uplifted to 47 as well)
[String/UUID change made/needed]: none

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8727170 [details] [diff] [review]
Part 1 - Don't schedule a paint if the displayport position didn't change

Improves APZ perf, Aurora47+

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e59f963b1a15
https://hg.mozilla.org/releases/mozilla-aurora/rev/bf8eb8bd2dbc

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out from aurora for android assertions like https://treeherder.mozilla.org/logviewer.html#?job_id=2205149&repo=mozilla-aurora

https://hg.mozilla.org/releases/mozilla-aurora/rev/e6bd5cd06759

Comment 28

[Carsten Book [:Tomcat]]

(In reply to Wes Kocher (:KWierso) from comment #27)
> I had to back this out from aurora for android assertions like
> https://treeherder.mozilla.org/logviewer.html#?job_id=2205149&repo=mozilla-
> aurora
> 
> https://hg.mozilla.org/releases/mozilla-aurora/rev/e6bd5cd06759

seems this 2 pushes were innocent so relanded in 
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/9c1b7984c81c
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ede02634231e