1209852 - Fix and re-enable scroll-inactive-layers-2.html

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

In order to get bug 1143856 landed without spending an infinite amount of time dealing with regressions, I disabled the scroll-inactive-layers-2.html test which was failing intermittently. I need to debug the problem and re-enable the test.

Comment 1

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

Attachment: Failure log

Here's a log with layout.display-list.dump and layers.dump-decisions set to true of when this failed for me locally.

Comment 2

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

Attachment: Successful log

And here's a success log

Comment 3

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

From what I understand of the no-paint checks, the failing run should still be passing because all of the reftest-no-paint divs are inside inactive layer managers and so they should never get the NS_FRAME_PAINTED_THEBES flag set on them. Not really sure what's going on here but I'll see if I can capture this in rr.

Comment 4

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

Attachment: Failure log

Failure log for which I have an rr trace

Comment 5

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

Attachment: Backtrace to setting the painted bit

It looks like it's correctly inside a BasicLayerManager so I'm not really sure why the cdi->mInactiveLayerManager is null.

Comment 6

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

markus, any ideas on where to go from here? I have an rr trace for it but I don't have a mental model of what the code is supposed to be doing so it's hard for me to say where things go wrong.

Comment 7

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

Working backwards a bit more, the LayerState value for the display item 0x2aaac750d970 is LAYER_NONE, so in the FrameLayerBuilder::AddPaintedDisplayItem function it never goes into the block that assigns tempManager.

Comment 8

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

Discussed with mstange on IRC, apparently I was barking up the wrong tree. The problem here is actually the invalidation; for some reason the entire page is getting repainted on the scroll rather than just the 50-pixel area that is exposed.

I ran it again with MOZ_REFTEST_VERBOSE=1 and nglayout.debug.invalidation=true and it looks like there's a race condition between the APZ flush request from the reftest harness and the paint for the scroll offset change. If the scroll offset change happens first everything is fine because the APZ flush request becomes a no-op. However if the APZ flush request gets processed first the invalid area expands and the entire page gets repainted. I'm not yet sure if this is just an artifact of the harness or an actual bug.

Comment 9

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

Attachment: fail.log

Here's a log of what I saw. Note that since the harness runs the same page twice (because it's an == of the test page with itself). In this run the first load of the page is bad because the "apz-repaints-flushed fired" shows up before the nonempty SendUpdateCanvasForEvent (i.e. before the paint happens). The second load of the page is fine.

Comment 10

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

It happens even without the forced APZ flush from the harness, because APZ might trigger a repaint on its own. The relevant part of the log seems to be this:

 0:04.69 ---- PAINT START ----PresShell(0x7ff306a7e500), nsView(0x7ff306b02000), nsIWidget(0x7ff30b7afc80)
 0:04.69 Invalidating changes of the visible region bounds of the scrolled contents
 0:04.69 Invalidating layer 0x7ff306b06500: < (x=0, y=0, w=786, h=100); (x=0, y=1100, w=786, h=1121); >
 0:04.71 ---- PAINT END ----
 0:04.71 Ending ProcessPendingUpdates
 0:04.71 REFTEST INFO | [CONTENT] AfterPaintListener in file:///home/kats/zspace/B2G-aries/gecko/layout/reftests/invalidation/scroll-inactive-layers-2.html
 0:04.71 REFTEST INFO | [CONTENT] SendUpdateCanvasForEvent with 3 rects
 0:04.71 REFTEST INFO | [CONTENT] Rect: 0 -100 786 0
 0:04.71 REFTEST INFO | [CONTENT] Rect: 0 0 800 1050
 0:04.71 REFTEST INFO | [CONTENT] Rect: 0 1050 786 2121

Note that the invalidated area is (x=0, y=0, w=786, h=100); (x=0, y=1100, w=786, h=1121) which is above and below the visible area, presumably this is from the displayport. However the AfterPaint event has three rects, the second one of which includes the visible area. So I guess something is causing that area to get painted too, but the invaldation reporting isn't reporting it.

Comment 11

[Markus Stange [:mstange]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> (because it's an == of the test page with itself).

Oops. It should probably be an != with about:blank, like the more recent invalidation tests I added.

Comment 12

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

Attachment: Patch

After some more tracing through the code and help from Markus the problem turned out to be that the initial displayport (as set by InitializeRootDisplayport) has zero margins and then after scrolling APZ expands the displayport to be the entire page. This change in displayport area causes an invalidation and repaint. Forcing the displayport to be a fixed size (which is what this patch does) makes the test pass. I will file a follow-up bug about the extra invalidation/repaint on growing the displayport since that seems like something we should avoid as well.

Comment 13

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

Attachment: Reenable test

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b3fd3c319bb

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b27e3f1780d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba7cdf401f27

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b27e3f1780d
https://hg.mozilla.org/mozilla-central/rev/ba7cdf401f27