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)

RESOLVED FIXED in Firefox 28

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jaws, Assigned: jrmuizel)

Tracking

(Blocks: 1 bug, {intermittent-failure})

Trunk
mozilla28
x86
Windows 7
intermittent-failure
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 unaffected, firefox27 unaffected, firefox28 fixed, firefox-esr24 unaffected)

Details

(Whiteboard: [Australis:P1])

Attachments

(3 attachments)

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

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

Comment 1

4 years ago
(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

4 years ago
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

4 years ago
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?
Flags: needinfo?(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...
Flags: needinfo?(mconley)

Comment 5

4 years ago
(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

4 years ago
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

4 years ago
(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

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

Comment 9

4 years ago
(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)
Flags: needinfo?(jmuizelaar)

Updated

4 years ago
Blocks: 845874
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
jrmuizel's on PTO - over to roc, who reviewed the patch.
Flags: needinfo?(roc)

Comment 13

4 years ago
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.
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 hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
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.
Flags: needinfo?(roc)

Comment 19

4 years ago
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.
Assignee: nobody → jmuizelaar
Any luck figuring this one out, Jeff? Or should we go ahead and back out bug 845874 from Holly?
I've got jrmuizel's go-ahead to back bug 845874 out from Holly.
(Assignee)

Comment 22

4 years ago
Created attachment 8343329 [details]
layerdump.txt

Here are the layers for the failures
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 23

4 years ago
Created attachment 8343857 [details] [diff] [review]
Backout patch
Try push: https://tbpl.mozilla.org/?tree=Try&rev=18e292f7a91a
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?
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!
Backout landed on holly as https://hg.mozilla.org/projects/holly/rev/9e7f91032d7d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox28: --- → fixed
Resolution: --- → FIXED
status-firefox26: --- → unaffected
status-firefox27: --- → unaffected
status-firefox-esr24: --- → unaffected
Target Milestone: --- → mozilla28
(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.
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.
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?
(Assignee)

Comment 31

4 years ago
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.
So it sounds like we're disabling the test for GPU-accelerated compositing?
(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...
You need to log in before you can comment on or make changes to this bug.