890997 - Enable OMTC on OSX 10.6

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Currently this is disabled because of crashes within the driver on tinderbox.

Try run to figure out what they are exactly: https://tbpl.mozilla.org/?tree=Try&rev=80303e04c2fe

Comment 1

[Matt Woodrow (:mattwoodrow)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=25041278&tree=Try&full=1#error1

and

https://tbpl.mozilla.org/php/getParsedLog.php?id=25040973&tree=Try&full=1#error1

Both crashing inside glDrawArrays.

Comment 2

[Matt Woodrow (:mattwoodrow)]

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

Comment 3

[Jeff Muizelaar [:jrmuizel]]

This test /tests/content/html/content/test/test_fullscreen-api.html is crashing.

Comment 4

[Benoit Girard (:BenWa)]

I can reproduce the crash on 10.6 but gdb can't debug XUL. I'm in the process of building lldb now.

I saw the crash while the compositor was compositing and the main thread was trying to dettach views. I'll have a better trace when lldb is ready. All signs point to a threading issue between the compositor and the main thread widget.

Comment 5

[Benoit Girard (:BenWa)]

There's several signature but this ones seems like the most interesting. The main thread is here while the compositor SEGFAULT on 0x0:
#0  0x00007fff81f6a02e in __CFDictionaryCallback ()
#1  0x00007fff81f6a52c in ___CFBasicHashFindBucket1 ()
#2  0x00007fff81f71a8e in CFBasicHashFindBucket ()
#3  0x00007fff81f7195b in CFDictionaryGetValue ()
#4  0x00007fff81fb2c9b in __CFXNotificationPost ()
#5  0x00007fff81f9f548 in _CFXNotificationPostNotification ()
#6  0x00007fff807efa36 in -[NSNotificationCenter postNotificationName:object:userInfo:] ()
#7  0x00007fff899c2d70 in -[NSWindow _reallyDoOrderWindow:relativeTo:findKey:forCounter:force:isModal:] ()
#8  0x00007fff899c27d2 in -[NSWindow orderWindow:relativeTo:] ()
#9  0x00007fff89b74a10 in -[NSWindow _close] ()
#10 0x0000000101db1f86 in nsCocoaWindow::DestroyNativeWindow (this=0x1399ac080) at /Users/mozilla/mozilla/mozilla-central/tree/widget/cocoa/nsCocoaWindow.mm:136

Comment 6

[Benoit Girard (:BenWa)]

Ok the problem is coming from nsCociaWindow::HideWindowChrome. return NS_OK fixes the bug here. The function is saving state and child window, create a new window with the right border settings and reparent everything.

I wonder if 10.6+ has a better way to do this. I bet this code was written way before 10.6 so let's see if we can improve this code at the same time.

Comment 7

[Markus Stange [:mstange]]

The current code was written to work on 10.4 (bug 420491). Starting with 10.5, NSView has a method
- (BOOL)enterFullScreenMode:(NSScreen *)screen withOptions:(NSDictionary *)options
which we can probably use.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Markus, do you have time to work on this? Trying to find the time, but don't want to take BenWa or Jeff off the B2G, then Windows performance (both TART and scrolling), etc., so if you are familiar with the code and can take it on...

Comment 9

[Benoit Girard (:BenWa)]

(In reply to Markus Stange [:mstange] from comment #7)
> The current code was written to work on 10.4 (bug 420491). Starting with
> 10.5, NSView has a method
> - (BOOL)enterFullScreenMode:(NSScreen *)screen withOptions:(NSDictionary
> *)options
> which we can probably use.

I was planning on fixing up HideWindowChrome to use setStyleMask to hide or show the window borders which was added in 10.6+.

Comment 10

[Benoit Girard (:BenWa)]

This test is disabled for 10.7+ but does not fail in the same way when re-enabled.
http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_fullscreen-api.html?force=1#55

Comment 11

[Benoit Girard (:BenWa)]

I tried to reproduce the crash as a user spamming html5 fullscreen video but I can't. We have a few options here:
1) Leave OMTC disabled because of this bug: The cost is very high because it means continuing to support non OMTC OGL layers on mac until we deprecate 10.6
2) mstange can look at using enterFullscreen: I played a bit with this today but there the content wasn't sized correctly, ALT+TAB didn't work, the test didn't finish. I don't think it's really worth spending time given that there doesn't appear to be user impact and 10.6 users using fullscreen is a very small market share that is going away. I don't think it's worth even trading a man day for this issue. I'd rather us spend this time in fixing the fullscreen tests on 10.7+.
3) Disable the test: See 2) for why it's not worth it.

TL;DR I don't think its worth blocking OMTC mac and I don't think it's worth fixing the test either. Let's disable it.

Comment 12

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 13

[Benoit Girard (:BenWa)]

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

Comment 14

[Benoit Girard (:BenWa)]

Looks like we have at least 2 more tests to disable if we go that route. mstange do you want to chime in here?

Comment 15

[Benoit Girard (:BenWa)]

This should block because we don't want to support OMTC and non OMTC layers in the same release for Mac so we need this fixed.

Comment 16

[Markus Stange [:mstange]]

Sorry for the delay, I'll look into it today.

Comment 17

[Markus Stange [:mstange]]

Attachment: wip

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

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Blocking based on comment 15.

Comment 19

[Markus Stange [:mstange]]

Attachment: "invalid context" workaround

The last patch didn't make a difference. Maybe this one will.
https://tbpl.mozilla.org/?tree=Try&rev=bc13a5c28ecc

Comment 20

[Benoit Girard (:BenWa)]

Any time to look into this Markus? We're getting closer to shipping OMTC on mac but we're having some doubt if it's actually ready with all the crashes we see in MakeCurrent.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Do we have any reports showing that the MakeCurrent crash happens in the wild? I've only seen the intermittent orange report so far. Still important, but I don't think we'd pull it from shipping because of that alone.

Comment 22

[Benoit Girard (:BenWa)]

See bug 921323, top OSX crasher.

Comment 23

[Markus Stange [:mstange]]

(In reply to Benoit Girard (:BenWa) from comment #20)
> Any time to look into this Markus?

I looked into this a bit but didn't get far. Attachment 803077 [details] [diff] made the fullscreen change you suggested (using setStyleMask instead of recreating the NSWindow) but it showed exactly the same crashes on tryserver. I think setStyleMasks internally detaches our OpenGL NSView from the window, recreates the window's outer NSView hierarchy and reattaches our NSView. So this change doesn't gain us anything (apart from simpler code).

Then I suspected that the "invalid context" messages I see in the console could have something to do with the crashes. They appear when calling [NSOpenGLContext currentContext] after we called makeCurrentContext on an NSOpenGLContext that's attached to a window that has already been closed. But my workaround for that didn't help with the crashes either, unfortunately. I still think the window closing situation is worth fixing - we really shouldn't try to composite a closed window - but it may not help with the 10.6 situation.

I didn't have any luck reproducing the crashes on my 10.6 machine. I ran the relevant tests in a loop but didn't crash once.

Comment 24

[Markus Stange [:mstange]]

This looks really promising:

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

I've fired off a few more try pushes to check which parts of this are really needed, but it seems like we're almost there now.

Comment 25

[Alex Keybl [:akeybl]]

I've emailed the gfx team to make a final decision on this for FF25.

Comment 26

[Markus Stange [:mstange]]

Attachment: enable

With the patches from bug 886999, bug 914437, bug 923114 and bug 923133, try is green: https://tbpl.mozilla.org/?tree=Try&rev=459e77438691
Note that I didn't have to change anything fullscreen-related after all.

Comment 27

[Benoit Girard (:BenWa)]

Comment on attachment 814907 [details] [diff] [review]
enable

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

Nice!

Comment 28

[Benoit Girard (:BenWa)]

Looks like unfortunately this wont be feasible to uplift to beta. See https://bugzilla.mozilla.org/show_bug.cgi?id=921323#c24 for a omtc crasher bug. Once this is fixed on central we will re-evaluate if we uplift this fix.

Comment 29

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e197b100ba53

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e197b100ba53

Comment 31

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Benoit Girard (:BenWa) from comment #28)
> Looks like unfortunately this wont be feasible to uplift to beta. See
> https://bugzilla.mozilla.org/show_bug.cgi?id=921323#c24 for a omtc crasher
> bug. Once this is fixed on central we will re-evaluate if we uplift this fix.

This is now on central for 2 weeks and we've got FF26 on Beta (in second week of Beta) -- what's the risk/reward of uplifting this to beta now? Should we bother?

Comment 32

[Benoit Girard (:BenWa)]

No, the benefit was not maintaining the code going forward on master. I think we can support it longer on branch however.