678002 - [MAC] Titlebar is black when a persona (lightweight theme) is applied on 10.6

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Matthew N. [:MattN]]

Build ID: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a1) Gecko/20110810 Firefox/8.0a1 
Built from: http://hg.mozilla.org/mozilla-central/rev/04dfb49d3a3d

Starting with today's nightly build (on Snow Leopard 10.6.8) the window titlebar is black instead of showing the persona image in it.  The titlebar is normal with the the default theme.

STR:
1) Go to getpersonas.com and 'wear' a persona

Expected result:
The persona image is shown on all of the standard toolbars and in the titlebar

Actual result:
The persona image shows on the toolbars but the titlebar background is black (with black text). See screenshot.

I suspect this is caused by fixes for OS X Lion (eg. bug 668195 or bug 667476).

Comment 1

[Matthew N. [:MattN]]

Attachment: Screenshot of black titlebar on Snow Leopard

Comment 2

[Steven Michaud [:smichaud] (Retired)]

> I suspect this is caused by fixes for OS X Lion (eg. bug 668195 or bug 667476).

You're probably right.

Comment 3

[Markus Stange [:mstange]]

Huh, strange. I see this bug in the downloaded nightly but not in my local debug build.

Comment 4

[Stefan [:stefanh]]

(In reply to Markus Stange from comment #3)
> Huh, strange. I see this bug in the downloaded nightly but not in my local
> debug build.

I noticed that too, my SeaMonkey debug build doesn't exibit this (built with your inbound patches, before they where pushed to m-c) but the latest nightly does.

Comment 5

[Markus Stange [:mstange]]

Attachment: printf makes it work

I have no idea what's going on here. I can reproduce the bug in an opt build, and adding a printf at this place makes it go away. I'd love to know why.

I've looked at the disassembly of TitlebarDrawCallback in gdb before and after the patch, and the call to printf is really the only difference. Three additional instructions:
+	lea    0x815a5d(%rip),%rdi
+	xor    %eax,%eax
+	callq  0x1023369c8 <dyld_stub_printf>

The bug also goes away if I removed the call to setCurrentContext, but that of course causes other problems. It looks like setCurrentContext somehow leaves the world in a broken state, and the call to printf fixes things again.

Comment 6

[Markus Stange [:mstange]]

Attachment: workaround

printf isn't the only magical antidote; filling the background with a color works, too, and has no bad side-effects. Should we land this as a temporary workaround? Finding the real bug here would be better, of course, but at the moment I have no idea where to look for it.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Since this bug is so fiddly, I wonder if it only occurs in 64-bit mode.

(Markus, the assembler code you quoted in comment #5 is, of course, 32-bit assembler.)

Comment 8

[Steven Michaud [:smichaud] (Retired)]

> (Markus, the assembler code you quoted in comment #5 is, of course,
> 32-bit assembler.)

Actually, now that I look more closely, it actually has both 64-bit
assembler (the references to %rip and %rdi) and 32-bit assembler (the
references to %eax).  How odd.

I can't say for sure this is a problem, though.

Comment 9

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #8)
> I can't say for sure this is a problem, though.

I don't think that's the problem. According to https://github.com/yasm/yasm/wiki/AMD64 %eax just maps to the 32 least significant bits of %rax.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

It also leaves the 32 most significant bits of %rax unchanged/uninitialized -- but again that might not be a problem.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

I still think it's worth checking to see if this bug happens in 32-bit mode.

Comment 12

[Josh Aas]

Comment on attachment 552356 [details] [diff] [review]
workaround

Review of attachment 552356 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with this workaround for now if you want to do it.

Comment 13

[Markus Stange [:mstange]]

Attachment: better patch

OK, this looks better!

Before bug 668195 we didn't call +[NSGraphicsContext setCurrentContext:] in drawsContentsIntoWindowFrame mode. This patch restores that situation, and magically the bug goes away. It also fixes bug 678184.
Only NSRectFill really looks at [NSGraphicsContext currentContext], so I'm limiting the setCurrentContext call to that case.

Comment 14

[Staś Małolepszy :stas]

Asa was able to reproduce this on his 10.7 too.

Comment 15

[Markus Stange [:mstange]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bcab3415d9ee

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/bcab3415d9ee

Comment 17

[Matthew N. [:MattN]]

Verified fixed on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a1) Gecko/20110815 Firefox/8.0a1