Enable OMTC on OSX 10.6

RESOLVED FIXED in Firefox 27

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mattwoodrow, Assigned: mstange)

Tracking

unspecified
mozilla27
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 unaffected, firefox25+ wontfix, firefox26+ wontfix, firefox27 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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
This test /tests/content/html/content/test/test_fullscreen-api.html is crashing.
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.
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
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.
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.
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...
Flags: needinfo?(mstange)
(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+.
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
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.
Created attachment 796893 [details] [diff] [review]
patch
Assignee: nobody → bgirard
Looks like we have at least 2 more tests to disable if we go that route. mstange do you want to chime in here?
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.
tracking-firefox25: --- → ?
tracking-firefox26: --- → ?
Sorry for the delay, I'll look into it today.
Flags: needinfo?(mstange)
Blocking based on comment 15.
status-firefox24: --- → unaffected
status-firefox25: --- → affected
status-firefox26: --- → affected
tracking-firefox25: ? → +
tracking-firefox26: ? → +
Created attachment 803757 [details] [diff] [review]
"invalid context" workaround

The last patch didn't make a difference. Maybe this one will.
https://tbpl.mozilla.org/?tree=Try&rev=bc13a5c28ecc
Attachment #803077 - Attachment is obsolete: true

Updated

5 years ago
Blocks: 756601
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.
(Reporter)

Comment 21

5 years ago
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.
See bug 921323, top OSX crasher.
(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.
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.
Assignee: bgirard → mstange
Status: NEW → ASSIGNED
I've emailed the gfx team to make a final decision on this for FF25.

Updated

5 years ago
Blocks: 924403
Created attachment 814907 [details] [diff] [review]
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.
Attachment #814907 - Flags: review?(bgirard)
Attachment #796893 - Attachment is obsolete: true
Attachment #803757 - Attachment is obsolete: true
Comment on attachment 814907 [details] [diff] [review]
enable

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

Nice!
Attachment #814907 - Flags: review?(bgirard) → review+
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.

Updated

5 years ago
status-firefox25: affected → wontfix
https://hg.mozilla.org/mozilla-central/rev/e197b100ba53
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(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?
Flags: needinfo?(bgirard)
No, the benefit was not maintaining the code going forward on master. I think we can support it longer on branch however.
Flags: needinfo?(bgirard)
status-firefox26: affected → wontfix
status-firefox27: --- → fixed
You need to log in before you can comment on or make changes to this bug.