593275 - Reftests aren't testing accelerated layer managers properly since bug 130078

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Reftests are still calling drawWindow on a child window. Post bug-130078, that window has no widget, so this code quietly turns off USE_WIDGET_LAYERS:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5284

We need to fix this. Now, before we turn on any accelerated layer managers by default.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: WIP

This kinda works but mysteriously, reftests hang when we load a XUL document.

Comment 2

[Joe Drew (not getting mail)]

That's expected behaviour because we don't load remote xul anymore. Run make reftest and the content pref in question will be set for you.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah, thanks for the tip!

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This actually seems to work! I'm sending it to the tryserver to see if it shows any new test failures.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This patch makes us always take snapshots of the toplevel window ('window' for reftest.js). That way, we get to use the retained layers.

Because the toplevel window is not affected by a zoomed subdocument, this actually simplifies the code a lot. No matter what the zoom factor of the subdocument, the area we need to paint is exactly the bounds of the destination canvas. For incremental updates, we do need to map the coordinates of the MozAfterPaint event up to coordinates in the toplevel window. We could set the MozAfterPaint listener on the root window to get around that, I suppose.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #5)
> Because the toplevel window is not affected by a zoomed subdocument, this
> actually simplifies the code a lot. No matter what the zoom factor of the
> subdocument, the area we need to paint is exactly the bounds of the destination
> canvas. For incremental updates, we do need to map the coordinates of the
> MozAfterPaint event up to coordinates in the toplevel window. We could set the
> MozAfterPaint listener on the root window to get around that, I suppose.

I'm puzzled why UpdateCurrentCanvasForEvent doesn't need to adjust the coordinates by the scrollX/scrollY as well.  (Or maybe you didn't hit any scrolled tests yet.)

That said, I wouldn't be puzzled if you had the MozAfterPaint listener on the toplevel window, which does seem like it might simplify things even more.


Also, could you move that rgb(255,255,255) comment to where we currently pass rgb(255,255,255) instead of removing it?  Or is it no longer correct?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Or are the MozAfterPaint events in the scrolled window already relative to its scrolled position?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #6)
> I'm puzzled why UpdateCurrentCanvasForEvent doesn't need to adjust the
> coordinates by the scrollX/scrollY as well.  (Or maybe you didn't hit any
> scrolled tests yet.)

It probably does, but it didn't before.

> That said, I wouldn't be puzzled if you had the MozAfterPaint listener on the
> toplevel window, which does seem like it might simplify things even more.

I'll try that.

> Also, could you move that rgb(255,255,255) comment to where we currently pass
> rgb(255,255,255) instead of removing it?  Or is it no longer correct?

It might be correct, but it probably isn't and it doesn't seem worth testing.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: v2

Does MozAfterPaint listening on the toplevel. Still seems to work.

Comment 10

[Joe Drew (not getting mail)]

This does indeed cause us to use the hardware-accelerated layer manager, at least on Mac.

Comment 11

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/301a576c3fc1

Comment 12

[Joe Drew (not getting mail)]

Backed out for bustage on Linux, at least.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: update drawWindow flags dynamically

I managed to reproduce the problem on my machine. (At least I think it's the same problem.) When I run reftests on a small-size screen --- with compiz disabled --- the tests start running at the desired window size (800x1000), BUT when we get to test-async.xul the window gets resized from outside to fit on screen! For the first few tests the window hasn't actually appeared so I think we're able to get going before the window manager is showing the window, and I guess the window manager shrinks our window to fit on screen before allowing it to display. Anyway, with the current code we don't notice that the window size has changed so we proceed with USE_WIDGET_LAYERS even though that won't work anymore.

This patch applies on top of the previous patch and simply recomputes the flags on every single drawWindow. It fixes the bug for me; we dynamically switch off USE_WIDGET_LAYERS at the right time.

This a bit scary, though, because there's no guarantee that the window size will remain unchanged between drawing a test and drawing its reference. If the window size does change between test and reference, we might render the test and reference with different settings for USE_WIDGET_LAYERS, so the test might fail spuriously. We could perhaps fix that by making the current test expected-random whenever we detect a flags change. However I haven't seen this happen in practice so I have no way to test that approach.

Interesting note: with compiz enabled, the window manager makes no attempt to make my windows fit on screen. Maybe we should enable compiz on all our test boxes, at least the GL ones!

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2 v2

Marks the test expected-random if drawWindow flags change during the test.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Linux reftests pass on try with this patch.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

However, all Linux tryserver machines aren't quite able to get the reftest window big enough. They all reduce the window size to height 974. Do we have a bug on file about that?

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 472140 [details] [diff] [review]
Part 2 v2

Why does test_async.xul cause the window to be resized?  Can we fix that?

And shouldn't we make the test fail if it's doing something that causes it never to be tested, rather than just hiding the failure by marking it expected-random?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #17)
> Why does test_async.xul cause the window to be resized?  Can we fix that?

I don't think test-async.xul is specifically causing it. That test doesn't really do anything other than test reftest-wait. It's probably just the first reftest-wait test that triggers it, or maybe the first XUL test, or maybe even just the test that runs a certain amount of time into the run.

> And shouldn't we make the test fail if it's doing something that causes it
> never to be tested, rather than just hiding the failure by marking it
> expected-random?

Not if it's not the test's fault, I think.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW, my tryserver push had these parts 1 and 2 on top of the patch to enable D3D layers, and there are three test failures that look real:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1283611007.1283612477.19724.gz#err0

I *presume* that with D3D layers disabled on trunk, we should be still OK to land parts 1 and 2.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #19)
> BTW, my tryserver push had these parts 1 and 2 on top of the patch to enable
> D3D layers, and there are three test failures that look real:
> 
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1283611007.1283612477.19724.gz#err0

Filed bug 593618 on the first two (I'm optimistically assuming same root problem); third failure is visually identical (one tone delta), added it to the list in 591155 for now.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 472140 [details] [diff] [review]
Part 2 v2

I don't think it's acceptable for tests to be randomly disabled.  I think if we run a set of reftests they should all get run, and any failure not marked as known in the manifest should be reported.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

An alternative approach that would cause that would be that if you're unable to run both test and reference with DRAW_USE_WIDGET_LAYERS, and you have a mismatched pair, you rerun without the flag whichever was previously run with it.

That said, it would be good to get an approach for testing this that doesn't depend on window sizes.  Can we force rendering to a layer that's larger than the widget?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe. We could probably even use cjones' SetDisplayPort API to do it on the layout side. We would probably also have to do something on the layer manager side (in particular for GL/D3D) to ensure that the backbuffer is the right size. I'm not sure how hard that would be.

(In reply to comment #22)
> An alternative approach that would cause that would be that if you're unable to
> run both test and reference with DRAW_USE_WIDGET_LAYERS, and you have a
> mismatched pair, you rerun without the flag whichever was previously run with
> it.

We could do that. I'm not sure how much to care about this since we can (and should) also change the test machine configurations to be have a slightly taller screen size. So in the end the only usage of either of these two approaches is for developers with unrealistically small screens, and running reftests accelerated on mobile devices.

Another immediate option is to just take the first version of the patch, in comment #13. In my testing, and on try, tests pass with it. If something changes and the size change happens at a bad time, that will show up as a test failure. Is that OK with you?

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #23)
> Another immediate option is to just take the first version of the patch, in
> comment #13. In my testing, and on try, tests pass with it. If something
> changes and the size change happens at a bad time, that will show up as a test
> failure. Is that OK with you?

That's ok with me, although I think we should try to find a solution that doesn't depend on screen size.

Comment 25

[Joe Drew (not getting mail)]

Comment on attachment 472138 [details] [diff] [review]
Part 2: update drawWindow flags dynamically

Given Rob and David's latest comments on this bug, let's see what David has to say about this patch.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 472138 [details] [diff] [review]
Part 2: update drawWindow flags dynamically

I'm fine with the reftest.js part, but don't go regenerating yacc-generated parsers as part of the same patch.

Comment 27

[Joe Drew (not getting mail)]

Re-landed along with part 2 v1:
http://hg.mozilla.org/mozilla-central/rev/f64155b51b37
http://hg.mozilla.org/mozilla-central/rev/c81b6eb6605a

Comment 28

[Armen [:armenzg]]

(In reply to comment #16)
> However, all Linux tryserver machines aren't quite able to get the reftest
> window big enough. They all reduce the window size to height 974. Do we have a
> bug on file about that?

No, I don't think so. I don't even think we manage the screen resolution with puppet on the Fedora slaves. Please file a bug when you know that what we need is a bigger screen size or to enable compiz enabled.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Filed bug 593874, thanks!