Last Comment Bug 678002 - [MAC] Titlebar is black when a persona (lightweight theme) is applied on 10.6
: [MAC] Titlebar is black when a persona (lightweight theme) is applied on 10.6
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: mozilla8
Assigned To: Markus Stange [:mstange] [away until June 27]
:
Mentors:
Depends on:
Blocks: 668195 678184
  Show dependency treegraph
 
Reported: 2011-08-10 12:21 PDT by Matthew N. [:MattN] (behind on reviews)
Modified: 2011-08-15 16:24 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Screenshot of black titlebar on Snow Leopard (146.02 KB, image/png)
2011-08-10 12:23 PDT, Matthew N. [:MattN] (behind on reviews)
no flags Details
printf makes it work (1.07 KB, patch)
2011-08-11 06:26 PDT, Markus Stange [:mstange] [away until June 27]
no flags Details | Diff | Review
workaround (1.11 KB, patch)
2011-08-11 06:29 PDT, Markus Stange [:mstange] [away until June 27]
jaas: review+
Details | Diff | Review
better patch (2.88 KB, patch)
2011-08-11 10:34 PDT, Markus Stange [:mstange] [away until June 27]
jaas: review+
Details | Diff | Review

Description Matthew N. [:MattN] (behind on reviews) 2011-08-10 12:21:49 PDT
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] (behind on reviews) 2011-08-10 12:23:21 PDT
Created attachment 552170 [details]
Screenshot of black titlebar on Snow Leopard
Comment 2 Steven Michaud [:smichaud] (Retired) 2011-08-10 12:44:45 PDT
> 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] [away until June 27] 2011-08-10 13:47:36 PDT
Huh, strange. I see this bug in the downloaded nightly but not in my local debug build.
Comment 4 Stefan [:stefanh] 2011-08-10 14:07:31 PDT
(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] [away until June 27] 2011-08-11 06:26:57 PDT
Created attachment 552355 [details] [diff] [review]
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] [away until June 27] 2011-08-11 06:29:12 PDT
Created attachment 552356 [details] [diff] [review]
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) 2011-08-11 07:40:22 PDT
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) 2011-08-11 07:47:38 PDT
> (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] [away until June 27] 2011-08-11 08:22:30 PDT
(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) 2011-08-11 08:30:35 PDT
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) 2011-08-11 08:46:06 PDT
I still think it's worth checking to see if this bug happens in 32-bit mode.
Comment 12 Josh Aas 2011-08-11 09:34:12 PDT
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] [away until June 27] 2011-08-11 10:34:12 PDT
Created attachment 552422 [details] [diff] [review]
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 2011-08-11 10:40:19 PDT
Asa was able to reproduce this on his 10.7 too.
Comment 15 Markus Stange [:mstange] [away until June 27] 2011-08-13 07:28:01 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/bcab3415d9ee
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-14 05:10:23 PDT
http://hg.mozilla.org/mozilla-central/rev/bcab3415d9ee
Comment 17 Matthew N. [:MattN] (behind on reviews) 2011-08-15 16:24:18 PDT
Verified fixed on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a1) Gecko/20110815 Firefox/8.0a1

Note You need to log in before you can comment on or make changes to this bug.