942250 - Holly branch mochitest-other: Windows 7 debug perma-orange: test_leaf_layers_partition_browser_window.xul | Leaf layers should form a non-overlapping partition of the browser window (non-maximized)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

+++ This bug was initially created as a clone of Bug #886281 +++

https://tbpl.mozilla.org/?tree=Holly&rev=1cddaa735901

Comment 1

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from bug 886281 comment #49)
> (In reply to :Gijs Kruitbosch from comment #48)
> > I'm re-running mochitest-other on the various merges to narrow the
> > regression range.
> 
> 
> Regression range:
> https://hg.mozilla.org/projects/holly/pushloghtml?startID=3&endID=4

Suspects: bug 939607 and bug 919144.

Comment 2

[:Gijs (out for now; he/him)]

Renewed try pushes without messing with build stuff (sigh):

Without bug 939607: https://tbpl.mozilla.org/?tree=Try&rev=3e8ef3e44f9d

Without bug 939607 and without bug 919144: https://tbpl.mozilla.org/?tree=Try&rev=9b7455657b39

Comment 3

[:Gijs (out for now; he/him)]

Sigh. All of that went orange, so so much for the suspicions. Guess we get to start bisecting.

This is the easy one, as Mike merged once, then merged some more and pushed, so I've pushed the earlier merge to try:

https://tbpl.mozilla.org/?tree=Try&rev=a428b22afc2c


This would roughly halve the regression range. Mike, do you have any other ideas based on any changes you had to make for this merge?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Looking at the log, these oranges were introduced during one of the more straight-forward merges. I didn't have to back anything out, so I honestly have no idea...grr...

Comment 5

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> https://tbpl.mozilla.org/?tree=Try&rev=a428b22afc2c

This is green. So it's somewhere in the upper half of https://hg.mozilla.org/projects/holly/pushloghtml?startID=3&endID=4, specifically, between d316d1dd062e and 3d0fce244e6b inclusive, which is something like:

http://hg.mozilla.org/mozilla-central/pushloghtml?tochange=d316d1dd062e&fromchange=5236947f9090

What confuses me though is that I thought we needed to re-backout the Australis changes on every merge, but that doesn't seem to be what's being done.

Comment 6

[:Gijs (out for now; he/him)]

Backed out up to 3b1d81c4c118:

https://tbpl.mozilla.org/?tree=Try&rev=d5ef9677532f

Backed out up to 5fd1b6c4ad77:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=1f0e6ff69710

Backed all of the inbound merge out:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=61863dc012ff


This had better not end up being the fault of all that build system stuff...

Comment 7

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #6)
> Backed out up to 3b1d81c4c118:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=d5ef9677532f
> 
> Backed out up to 5fd1b6c4ad77:
> 
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=1f0e6ff69710
> 
> Backed all of the inbound merge out:
> 
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=61863dc012ff
> 
> 
> This had better not end up being the fault of all that build system stuff...

 Bottom one is green, top two are orange. So that leaves:

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?tochange=5fd1b6c4ad77&fromchange=e4b59fdbc9c2

My money is on http://hg.mozilla.org/integration/mozilla-inbound/rev/d0255c4b2adc .

Comment 8

[:Gijs (out for now; he/him)]

Try push of the merge without the changeset I suspect: https://tbpl.mozilla.org/?tree=Try&rev=40ac5ab9faed

Comment 9

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #8)
> Try push of the merge without the changeset I suspect:
> https://tbpl.mozilla.org/?tree=Try&rev=40ac5ab9faed

Green. Alright, Jeff, do you have any idea how that patch broke this test on Holly? (mc-sans-Australis-aka-our-future-Aurora)

Comment 10

[Legacy TBPL/Treeherder Robot]

Gijs
https://tbpl.mozilla.org/php/getParsedLog.php?id=31004830&tree=Holly
Windows 7 32-bit holly debug test mochitest-other on 2013-11-23 10:28:22
revision: 2b90f521f14d
slave: t-w732-ix-079

15317 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_leaf_layers_partition_browser_window.xul | Leaf layers should form a non-overlapping partition of the browser window (non-maximized). [Set MOZ_DUMP_PAINT_LIST=1 env var to investigate.]
Return code: 1
Return code: 1
Return code: 1

Comment 11

[Legacy TBPL/Treeherder Robot]

mconley
https://tbpl.mozilla.org/php/getParsedLog.php?id=31012895&tree=Holly
Windows 7 32-bit holly debug test mochitest-other on 2013-11-23 16:11:16
revision: f8d8d9949a22
slave: t-w732-ix-073

15317 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_leaf_layers_partition_browser_window.xul | Leaf layers should form a non-overlapping partition of the browser window (non-maximized). [Set MOZ_DUMP_PAINT_LIST=1 env var to investigate.]
Return code: 1
Return code: 1
Return code: 1

Comment 12

[Mike Conley (:mconley) (:⚙️)]

jrmuizel's on PTO - over to roc, who reviewed the patch.

Comment 13

[:Gijs (out for now; he/him)]

Note: this is meant to be Aurora on Monday 9th. That's 1.5 weeks from now. We should probably at least have a potential/idea for a fix by Monday (2nd), or we should either back this out or disable the test to ensure we don't have last-minute shenanigans as holly hits Aurora.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

I just spoke to Milan - Jeff is back this Friday, and working with him once he gets back rather than spinning up someone new is probably the most efficient use of the Gfx team's time.

Comment 15

[Legacy TBPL/Treeherder Robot]

jwein%mozilla.com
https://tbpl.mozilla.org/php/getParsedLog.php?id=31174039&tree=Holly
Windows 7 32-bit holly debug test mochitest-other on 2013-11-27 10:00:35
revision: b1c32805287e
slave: t-w732-ix-086

15317 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_leaf_layers_partition_browser_window.xul | Leaf layers should form a non-overlapping partition of the browser window (non-maximized). [Set MOZ_DUMP_PAINT_LIST=1 env var to investigate.]
Return code: 1
Return code: 1
Return code: 1

Comment 16

[Legacy TBPL/Treeherder Robot]

mconley
https://tbpl.mozilla.org/php/getParsedLog.php?id=31272673&tree=Holly
WINNT 6.2 holly debug test mochitest-other on 2013-11-29 12:13:15
revision: 95ff4a81aa97
slave: t-w864-ix-053

14760 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_leaf_layers_partition_browser_window.xul | Leaf layers should form a non-overlapping partition of the browser window (non-maximized). [Set MOZ_DUMP_PAINT_LIST=1 env var to investigate.]
Return code: 1
Return code: 1
Return code: 1

Comment 17

[Legacy TBPL/Treeherder Robot]

mconley
https://tbpl.mozilla.org/php/getParsedLog.php?id=31274967&tree=Holly
Windows 7 32-bit holly debug test mochitest-other on 2013-11-29 13:27:17
revision: aeb2c05f2ab4
slave: t-w732-ix-096

15317 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_leaf_layers_partition_browser_window.xul | Leaf layers should form a non-overlapping partition of the browser window (non-maximized). [Set MOZ_DUMP_PAINT_LIST=1 env var to investigate.]
Return code: 1
Return code: 1
Return code: 1

Comment 18

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

This test tests that layers for a regular browser window don't overlap, which allows some optimizations. It's certainly conceivable that bug 845874 changed the way regions work so we simplify more aggressively in a way that makes layer visible regions overlap.

I think backing out bug 845874 on holly/Aurora would be reasonable.

Comment 19

[:Gijs (out for now; he/him)]

Attachment: MOZ_DUMP_PAINT_LIST output

Jeff said he was looking at this today. I'd just checked if I could reproduce; this is the MOZ_DUMP_PAINT_LIST output on my own win7 machine.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Any luck figuring this one out, Jeff? Or should we go ahead and back out bug 845874 from Holly?

Comment 21

[Mike Conley (:mconley) (:⚙️)]

I've got jrmuizel's go-ahead to back bug 845874 out from Holly.

Comment 22

[Jeff Muizelaar [:jrmuizel]]

Attachment: layerdump.txt

Here are the layers for the failures

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Attachment: Backout patch

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=18e292f7a91a

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Uh, try push hit this:

c:\Program Files (x86)\Windows Kits\8.0\include\shared\sal.h(2961) : error C2143: syntax error : missing ')' before '('
c:\Program Files (x86)\Windows Kits\8.0\include\shared\sal.h(2961) : error C2081: '_In_function_class_' : name in formal parameter list illegal
c:\Program Files (x86)\Windows Kits\8.0\include\shared\sal.h(2961) : error C2143: syntax error : missing ')' before 'type'
c:\Program Files (x86)\Windows Kits\8.0\include\shared\sal.h(2961) : error C2091: function returns function
c:\Program Files (x86)\Windows Kits\8.0\include\shared\sal.h(2961) : error C2055: expected formal parameter list, not a type list
c:\Program Files (x86)\Windows Kits\8.0\include\shared\sal.h(2961) : error C2059: syntax error : ')'
c:\Program Files (x86)\Windows Kits\8.0\include\shared\sal.h(2961) : error C2059: syntax error : ';'
c:\Program Files (x86)\Windows Kits\8.0\include\shared\sal.h(2961) : error C2059: syntax error : ')'
make[6]: *** [utypes.obj] Error 2
make[5]: *** [all-recursive] Error 2

I retriggered - hopefully that's just an intermittent red?

Comment 26

[Mike Conley (:mconley) (:⚙️)]

This was indeed a random red.

According to releng, the try machine that red'd out on us is faulty, and is being taken behind the barn and shot.

I will land the back-out patch on Holly. Thanks Jeff!

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Backout landed on holly as https://hg.mozilla.org/projects/holly/rev/9e7f91032d7d

Comment 28

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

(In reply to :Gijs Kruitbosch from comment #19)
> Created attachment 8341121 [details]
> MOZ_DUMP_PAINT_LIST output
> 
> Jeff said he was looking at this today. I'd just checked if I could
> reproduce; this is the MOZ_DUMP_PAINT_LIST output on my own win7 machine.

Thanks for this!

 0:23.54 D3D10LayerManager (0xb0ff488)
 0:23.54   D3D10ContainerLayer (0x1006ec50) [visible=< (x=0, y=0, w=948, h=739); >] [metrics={ viewport=(x=0,000000, y=0,000000, w=948,000000, h=739,000000) viewportScroll=(x=0,000000, y=0,000000) displayport=(x=0,000000, y=0,000000, w=0,000000, h=0,000000) scrollId=0 }]
 0:23.54     D3D10ThebesLayer (0xb3f7a80) [visible=< (x=0, y=0, w=948, h=30); (x=4, y=30, w=278, h=19); (x=0, y=49, w=948, h=38); >] [valid=< (x=0, y=0, w=948, h=30); (x=4, y=30, w=278, h=19); (x=0, y=49, w=948, h=38); >]
 0:23.54     D3D10ContainerLayer (0x130bc450) [clip=(x=0, y=0, w=948, h=739)] [visible=< (x=0, y=87, w=1, h=651); >]
 0:23.54       D3D10ThebesLayer (0x130b57c8) [clip=(x=0, y=0, w=0, h=0)] [not visible]
 0:23.54       D3D10ColorLayer (0x1045a748) [visible=< (x=0, y=87, w=1, h=651); >] [color=rgba(26, 26, 26, 0,4)]
 0:23.54     D3D10ContainerLayer (0xfe5d438) [clip=(x=1, y=87, w=946, h=651)] [visible=< (x=1, y=87, w=946, h=651); >] [opaqueContent]
 0:23.54       D3D10ThebesLayer (0xbe053b0) [visible=< (x=1, y=87, w=946, h=651); >] [opaqueContent] [valid=< (x=1, y=87, w=946, h=651); >]
 0:23.54     D3D10ContainerLayer (0x130bc228) [clip=(x=0, y=0, w=948, h=739)] [visible=< (x=947, y=87, w=1, h=651); >]
 0:23.54       D3D10ThebesLayer (0x130b59a0) [clip=(x=0, y=0, w=0, h=0)] [not visible]
 0:23.54       D3D10ColorLayer (0x10459988) [visible=< (x=947, y=87, w=1, h=651); >] [color=rgba(26, 26, 26, 0,4)]
 0:23.54     D3D10ContainerLayer (0x130bb538) [clip=(x=0, y=0, w=948, h=739)] [visible=< (x=0, y=738, w=948, h=1); >]
 0:23.54       D3D10ThebesLayer (0x130b5b78) [visible=< (x=0, y=738, w=948, h=1); >] [valid=< (x=0, y=738, w=948, h=1); >]

It seems to me that the rectangles (x=0, y=30, w=4, h=19) and (x=282, y=30, w=726, h=19) are not present in any of the visible regions. You might expect them to be in ThebesLayer 0xb3f7a80. I believe that's what's causing the test failure; as far as I can tell, the leaf layers aren't overlapping. It's entirely possible that with the old regions, 0xb3f7a80's visible region was being fluffed out to a rectangle and masking this issue.

I guess it makes sense that 0xb3f7a80 doesn't have those rectangles; they would correspond to transparent rectangles on the left side and right side of the tab titles.

Comment 29

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

The first thing to note is that the test failure doesn't indicate a significant performance problem. D3D10 (and any accelerated backend) doesn't care about whether the leaf layers partition the window, because the double-buffering optimization that cares about the partitioning only matters for BasicLayers. AFAIK we only have a transparent chrome window (and these test failures) when Aero Glass is enabled, and would we actually be using BasicLayers on a system with Aero Glass? Even if we do, it would only be for a small minority of users. So I think we could disable the test when GPU accelerated compositiong is enabled. That would resolve this bug.

Another option would be to improve the BasicLayers double-buffering optimization so that if a container layer's children are non-overlapping but don't completely cover the container, draw the children without double-buffering and explicitly clear the parts of the container layer that aren't covered. Then alter nsDOMWindowUtils::LeafLayersPartitionWindow to drop the test that the window is completely covered by the leaf layers, making this test green.

Comment 30

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

Take your pick. Disabling the test for GPU-accelerated compositing is probably the best thing to do if we believe that BasicLayers Aero Glass users are rare. Do we have any data on that?

Comment 31

[Jeff Muizelaar [:jrmuizel]]

The number of people using HW accel layers on Windows 7 is 72% on Vista it's like 25%. I expect most people on those OSs will be using Glass. So that's your number.

These numbers should also improve in FF26 as we loosened the blacklist some.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

So it sounds like we're disabling the test for GPU-accelerated compositing?

Comment 33

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

(In reply to Jeff Muizelaar [:jrmuizel] from comment #31)
> The number of people using HW accel layers on Windows 7 is 72% on Vista it's
> like 25%. I expect most people on those OSs will be using Glass. So that's
> your number.

Those numbers are lower than I expected :-(.

We could fix this for real following the second para of comment #29 but I see we've backed out y-x regions on Holly already...