1017065 - layout/reftests/reftest-sanity/async-scroll-1b.html should be passing in local Linux reftest-ipc

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

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

The test at layout/reftests/reftest-sanity/async-scroll-1b.html currently passes on tbpl when it is run as part of the Linux reftest-ipc (Ripc) suite. However as far as I can tell, this test should be failing. It is a != test, where the rendering is compared to async-scroll-1-ref.html but the two pages should be rendering identically. Also async-scroll-1b.html is identical to async-scroll-1a.html except for the height of the displayport, and that shouldn't affect the rendering at all. async-scroll-1a is a == test.

When I run the reftest-ipc suite locally on my Linux machine (using [1]), the test fails as expected. However on tbpl it passes for unknown reasons. It starts failing when low-res painting is enabled, which is just weird. I'm investigating.

[1] EXTRA_TEST_ARGS="--setpref=layers.async-pan-zoom.enabled=true" ./mach reftest-ipc layout/reftests/reftest-sanity

Comment 1

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

Attachment: Reftest rendering of async-scroll-1b.html

According to my try push at https://tbpl.mozilla.org/?tree=Try&rev=0121b01b8333, the reason the test passes on tbpl is because the bottom (50px?) of async-scroll-1b.html is just black.

Comment 2

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

Attachment: Reftest rendering of async-scroll-1-ref.html

Comment 3

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

I guess this test actually should be passing after all. The displayport is 1000px tall, and the async scroll offset is to 50px, so it makes sense that the bottom 50px should just be black, because it's outside the displayport. I'll leave this open until I figure out why it fails for me locally.

Comment 4

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

Ah, so the problem is that when this test runs, the displayed bottom 50px of the async-scroll-1b.html file is basically garbage. When I run async-scroll-1b.html in isolation, the garbage is just plain gray, and the test passes. When I run async-scroll-1a.html and async-scroll-1b.html together, the garbage that is shown just so happens to be the bottom 50px of the async-scroll-1-ref.html file from the previous reftest, which happens to be exactly what is needed to make the test fail. Presumably the tbpl job is clearing the canvas or something between each test so it passes there.

Comment 5

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

As far as I can tell it's the drawWindow call itself (part of reftest.js) that's putting the garbage in. I don't know if that's something we even want to change.

Comment 6

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

Attachment: Prevent the wrong garbage from failing the reftest

Best I could come up with :(

Comment 7

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8430205 [details] [diff] [review]
Prevent the wrong garbage from failing the reftest

I don't think we should be drawing garbage here at all. I think that indicates a deeper problem.

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> I guess this test actually should be passing after all. The displayport is
> 1000px tall, and the async scroll offset is to 50px, so it makes sense that
> the bottom 50px should just be black, because it's outside the displayport.
> I'll leave this open until I figure out why it fails for me locally.

The displayport is relative to the scrollport (ie display port of 0,0 100x100 would be the rect at 0,0 100x100 of the scrolled contents when our scrollposition is 0,0, but if we scroll to 0,100 then the displayport would contain the scrolled contents at 0,100 100x100). So the bottom 50px is still in the displayport. I think this indicates that the async scroll offset isn't working correctly.

Comment 9

[Timothy Nikkel (:tnikkel)]

Oh, I guess we aren't supposed to redraw from content with the async scroll pos.

Comment 10

[Timothy Nikkel (:tnikkel)]

I think we should be drawing the red from the body bg color where we don't have displayport contents to cover it. Ah, that's what the original test was intended to be testing.

So I'm guessing the bg color item for the body is wrongly getting an empty visible region or something. I think this is because that bg color item will be inside the scroll layer and will be occluded by the green div.

If we do the same test but put the displayport on a scrollable div I think that should work.

Comment 11

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

Is that an r- then?

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8430205 [details] [diff] [review]
Prevent the wrong garbage from failing the reftest

Sure if you'd like. Assuming comment 10 turns out to be accurate.

Comment 13

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

Thanks.

I tried using this as async-scroll-1b.html but it had exactly the same problem:

<!DOCTYPE HTML>
<html reftest-async-scroll>
<body style="background:red; margin:0; overflow:hidden">
 <div reftest-displayport-x="0" reftest-displayport-y="0"
      reftest-displayport-w="800" reftest-displayport-h="1000"
      reftest-async-scroll-x="0" reftest-async-scroll-y="50"
      style="position:absolute; overflow:hidden; height: 1000px">
  <div style="background:lime; top:50px; left:0; width:800px; height:1000px"></div>
  <div style="background:yellow; bottom:-50px; left:50px; width:50px; height:50px"></div>
 </div>
</body>
</html>

Comment 14

[Timothy Nikkel (:tnikkel)]

That definitely sounds like a bug. Is it quick and easy for you to get a display list dump?

Comment 15

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

Attachment: Display list dump from reftest

Here is the output pertaining to async-scroll-1b.html when I run MOZ_DUMP_PAINT_LIST=1 EXTRA_TEST_ARGS="--setpref=layers.async-pan-zoom.enabled=true" ./mach reftest-ipc layout/reftests/reftest-sanity on a build with dump painting enabled.

Comment 16

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

(The version of async-scroll-1b.html that I used is the one in comment 13)

Comment 17

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8430790 [details]
Display list dump from reftest

The vis region looks good there. So my guess wasn't right.

But we are optimizing to color layers, maybe that is confusing something?

Could you put some other content in the scrollable div so it can't be made into a color layer (some text or a small div with a bg color)? Also outside the scrollable div in case the bg color for the whole page creating a color layer is involved too.

Comment 18

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

Attachment: Display list dump from modified test

Attached dump for this test case:

<!DOCTYPE HTML>
<html reftest-async-scroll>
<body style="background:red; margin:0; overflow:hidden">
 <div reftest-displayport-x="0" reftest-displayport-y="0"
      reftest-displayport-w="800" reftest-displayport-h="1000"
      reftest-async-scroll-x="0" reftest-async-scroll-y="50"
      style="position:absolute; overflow:hidden; height: 1000px">
  <div style="background:lime; top:50px; left:0; width:800px; height:1000px">blah</div>
  <div style="background:yellow; bottom:-50px; left:50px; width:50px; height:50px"></div>
 </div>
 foo
</body>
</html>

Comment 19

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8431091 [details]
Display list dump from modified test

Thanks. This dump looks fine, nothing unexpected. And we still draw garbage in the bottom 50 pixels?

Comment 20

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

Yup. Visually in the window that opened the bottom was black, and the canvas that got dumped by the reftest had grey instead of black.

Comment 21

[Timothy Nikkel (:tnikkel)]

As far as I can tell layout is painting the correct things. To verify that the layer contents are correct you could use the paint dumping in html format, that should dump a data uri of layer contents. If that's right then we must going wrong with the async scroll offset and compositing the layers.

Comment 22

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

Attachment: HTML dump

This is the HTML dump of the modified async-scroll-1b.html. Looks like the red and green backgrounds are solid, so it's a problem in the compositor?

Comment 23

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> Created attachment 8432548 [details]
> HTML dump
> 
> This is the HTML dump of the modified async-scroll-1b.html. Looks like the
> red and green backgrounds are solid, so it's a problem in the compositor?

The dump only seems to have data URIs for display items, not the actual layers themselves. So the display items think they are painting the correct thing. But we don't know what actually ends up in the layers. I'm not sure why the layer contents didn't make it into the dump.

Comment 24

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

(In reply to Timothy Nikkel (:tn) from comment #23)
> The dump only seems to have data URIs for display items, not the actual
> layers themselves. So the display items think they are painting the correct
> thing. But we don't know what actually ends up in the layers. I'm not sure
> why the layer contents didn't make it into the dump.

Probably because the layer dumping code crashes a lot and I had to disable some of it (e.g. bug 1019004, bug 946879).

Comment 25

[Timothy Nikkel (:tnikkel)]

Finding out what makes it into the actual layers will determine where to look for the problem.

Comment 26

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

Not sure what fixed it, but running the remote-ipc tests locally passes for me now.