OpenTTD

Tasklist

FS#6012 - [OSX] Crashes clicking screen resolution dropdown

Attached to Project: OpenTTD
Opened by Will Glynn (willglynn) - Friday, 09 May 2014, 18:28 GMT
Last edited by frosch (frosch) - Thursday, 05 June 2014, 17:38 GMT
Type Bug
Category Core
Status Closed
Assigned To No-one
Operating System Mac OS X
Severity High
Priority Normal
Reported Version 1.4.1-RC1
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Steps to reproduce:

1. Download, extract OpenTTD
2. Launch OpenTTD
3. Click "Game Options"
4. See "Screen resolution: other", click dropdown

Expected results: dropdown, including the ability to run fullscreen at 1440x900. (I'm on a 15" Retina MacBook Pro running 2880x1800.)
Actual results: crash to desktop

OSX gives a backtrace:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0 libsystem_kernel.dylib 0x97f82952 __pthread_kill + 10
1 libsystem_pthread.dylib 0x90790167 pthread_kill + 101
2 libsystem_c.dylib 0x935a229c abort + 155
3 org.openttd.openttd 0x0022dea8 HandleCrash(int) + 520
4 libsystem_platform.dylib 0x965d3deb _sigtramp + 43
5 org.openttd.openttd 0x003d8389 ShowDropDownListAt(Window*, AutoDeleteSmallVector<DropDownListItem const*, 4u> const*, int, int, OTTD_Rect, Colours, bool, bool) + 793
6 org.openttd.openttd 0x003d8826 ShowDropDownList(Window*, AutoDeleteSmallVector<DropDownListItem const*, 4u> const*, int, int, unsigned int, bool, bool) + 438
7 org.openttd.openttd 0x00304ee8 GameOptionsWindow::OnClick(OTTD_Point, int, int) + 152
8 org.openttd.openttd 0x003dfe16 HandleMouseEvents() + 7078
9 org.openttd.openttd 0x003e5805 QZ_GameLoop() + 2725
10 org.openttd.openttd 0x003e14cd -[OTTDMain launchGameEngine:] + 45
11 com.apple.Foundation 0x9bfeb732 __57-[NSNotificationCenter addObserver:selector:name:object:]_block_invoke + 49
12 com.apple.CoreFoundation 0x963b85a4 __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 20
13 com.apple.CoreFoundation 0x9629a05b _CFXNotificationPost + 3435
14 com.apple.Foundation 0x9bfda21f -[NSNotificationCenter postNotificationName:object:userInfo:] + 92
15 com.apple.Foundation 0x9bff77de -[NSNotificationCenter postNotificationName:object:] + 55
16 org.openttd.openttd 0x003e15a4 -[OTTDMain applicationDidFinishLaunching:] + 148
17 com.apple.Foundation 0x9bfeb732 __57-[NSNotificationCenter addObserver:selector:name:object:]_block_invoke + 49
18 com.apple.CoreFoundation 0x963b85a4 __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 20
19 com.apple.CoreFoundation 0x9629a05b _CFXNotificationPost + 3435
20 com.apple.Foundation 0x9bfda21f -[NSNotificationCenter postNotificationName:object:userInfo:] + 92
21 com.apple.AppKit 0x940435b5 -[NSApplication _postDidFinishNotification] + 367
22 com.apple.AppKit 0x94043255 -[NSApplication _sendFinishLaunchingNotification] + 239
23 com.apple.AppKit 0x9403fb84 -[NSApplication(NSAppleEventHandling) _handleAEOpenEvent:] + 840
24 com.apple.AppKit 0x9403f471 -[NSApplication(NSAppleEventHandling) _handleCoreEvent:withReplyEvent:] + 277
25 libobjc.A.dylib 0x9902c304 -[NSObject performSelector:withObject:withObject:] + 77
26 com.apple.Foundation 0x9bffa47a __76-[NSAppleEventManager setEventHandler:andSelector:forEventClass:andEventID:]_block_invoke + 121
27 com.apple.Foundation 0x9bff9fb1 -[NSAppleEventManager dispatchRawAppleEvent:withRawReply:handlerRefCon:] + 430
28 com.apple.Foundation 0x9bff9dbb _NSAppleEventManagerGenericHandler + 218
29 com.apple.AE 0x907f6b15 aeDispatchAppleEvent(AEDesc const*, AEDesc*, unsigned long, unsigned char*) + 387
30 com.apple.AE 0x907c5ed6 dispatchEventAndSendReply(AEDesc const*, AEDesc*) + 44
31 com.apple.AE 0x907c5dce aeProcessAppleEvent + 318
32 com.apple.HIToolbox 0x9a29d591 AEProcessAppleEvent + 55
33 com.apple.AppKit 0x9403b188 _DPSNextEvent + 1089
34 com.apple.AppKit 0x9403a8b0 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 119
35 com.apple.AppKit 0x9402d19c -[NSApplication run] + 727

crash.* are attached.
This task depends upon

Closed by  frosch (frosch)
Thursday, 05 June 2014, 17:38 GMT
Reason for closing:  Fixed
Additional comments about closing:  in r26629
Comment by Remko Bijker (Rubidium) - Sunday, 11 May 2014, 10:24 GMT
  • Field changed: Summary (OpenTTD 1.4.1-RC1 crashes when clicking screen resolution dropdown → [OSX] Crashes clicking screen resolution dropdown)
  • Field changed: Reported Version (other → 1.4.1-RC1)
Given the crash log, it's a floating point error. This is usually division by zero. There are two divisions in the function that crashes.

One divides a number by the list length, another by the average height. The average height is the list height divided by the list length, so it looks to me like the OS X specific parts of OpenTTD do not fill the drop down data correctly.

One big caveat is that there is effectively no active OS X developer, so it might take a long time for anyone attempting to fix this bug.
Comment by Will Glynn (willglynn) - Monday, 12 May 2014, 16:05 GMT
I'm toying with this a bit but I feel like I'm missing some development infrastructure. (There's no XCode project and make-via-XCode isn't playing nice with my Homebrew-supplied LZMA/LZO libraries.) How are the current OSX binaries produced?
Comment by Will Glynn (willglynn) - Monday, 12 May 2014, 16:29 GMT
I got it built and running in a debugger. My binary doesn't crash.
Comment by Will Glynn (willglynn) - Tuesday, 13 May 2014, 02:26 GMT
I can report that the latest prebuilt nightly (openttd-trunk-r26585-macosx-universal) also does not crash. I'm traveling in the morning and don't really have time to bisect this tonight, but I thought that worth noting.
Comment by frosch (frosch) - Tuesday, 20 May 2014, 17:42 GMT
 FS#6020  reports this issue for 1.4.0
Comment by fonsinchen (fonsinchen) - Tuesday, 20 May 2014, 17:49 GMT
This seems similar to  FS#5889  and, surprisingly,  FS#5885 .  FS#5885  is also a division by 0, at a similar place, from a dropdown list, caused by bad font selection that makes the height of the entries 0.
Comment by James (ChooChoo5) - Thursday, 22 May 2014, 07:42 GMT
With V1.4.0, running on OS X 10.6.8, as well as this issue, the 'fullscreen' button does not function - the error 'fullscreen mode failed' appears.
Comment by Will Glynn (willglynn) - Thursday, 29 May 2014, 13:23 GMT
I just downloaded the openttd-1.4.1-RC2-macosx-universal and openttd-trunk-r26620-macosx-universal builds. 1.4.1-RC2 has this crash bug, r26620 does not.
Comment by Will Glynn (willglynn) - Thursday, 29 May 2014, 14:11 GMT
The crash is at: http://vcs.openttd.org/svn/browser/branches/1.4/src/widgets/dropdown.cpp#L144
invoked from: http://vcs.openttd.org/svn/browser/branches/1.4/src/settings_gui.cpp#L442

_num_resolutions is 0 by the time we get here. The Allegro driver guards against this so it seems like a legal (but uncommon) condition which should not result in the game crashing.

Poking around further, QZ_UpdateVideoModes() gets called twice on startup: I get 13 modes the first time and 0 the second. The first call is from VideoDriver_Cocoa::Start(), the second from VideoDriver_Cocoa::ChangeResolution(). I don't know why it's failing (or why trunk works fine) but it seems reasonable to use the existing resolution list if subsequent calls are empty, so that's what this patch does. This fixes the issue although, again, the game should probably not crash if _num_resolutions is 0 for any other reason.
Comment by Will Glynn (willglynn) - Tuesday, 03 June 2014, 15:00 GMT
As expected, openttd-1.4.1-macosx-universal has this crash bug.

A little more digging: QZ_ListModes() is first called with device_depth=32, then called again with device_depth=8. OSX returns no resolutions at 8bpp, so QZ_ListModes returns an empty set. I don't claim to understand the graphics code (especially since the OSX bits use way-deprecated APIs) but I still think my patch is a reasonable workaround.
Comment by fonsinchen (fonsinchen) - Wednesday, 04 June 2014, 19:49 GMT
Some OS don't do 8bpp anymore. Apparently the game is running in some supported video mode when you click on the dropdown, or else you wouldn't see anything at all. Probably that is a 32bpp mode. The bug is then that the wrong pixel depth is given to the query to retrieve the list of resolutions. Or, more generally, we need a way to cleanly sort out the 8bpp vs. 32bpp mess when switching video modes. Currently the chosen mode depends on various factors: the user's choice, the modes supported by OS and driver, pixel depth of loaded base sets and pixel depth of loaded newgrfs, off the top of my head. Those things can change independent of each other at runtime, leading to new modes being chosen. The result probably feels pretty chaotic to anyone not really involved in that and under certain conditions it can just fail to choose the right mode.
Comment by frosch (frosch) - Wednesday, 04 June 2014, 20:22 GMT
Trunk always uses 32bpp since r26522. 8bpp is only used when enabled via a config-file-only option. That may be, why this seems to only apply to 1.4.

There is also an option to use 32bpp specifically only for full screen, which is used in the win32 video driver (win32_v.cpp:288).

So, I guess the fix should be:
- Do not crash for empty list, but display an error.
- If it is worth the effort, make the osx video driver behave like the win32 driver.

(just in case: r26522 won't make it into the 1.4 branch)
Comment by Will Glynn (willglynn) - Wednesday, 04 June 2014, 20:26 GMT
It certainly does feel chaotic.

I note that QZ_CanDisplay8bpp() says:

/* 8bpp modes are deprecated starting in 10.5. CoreGraphics will return them
* as available in the display list, but many features (e.g. palette animation)
* will be broken. */
if (MacOSVersionIsAtLeast(10, 5, 0)) return false;

My Mac (running 10.9) doesn't list any 8bpp modes, so the resolution list is empty, and clicking it causes a crash-to-desktop. This can be reproduced 100% of the time and should be fixed somehow.

Relatedly: is there an official position about support for ancient versions of OSX? The game has code for 10.3 (over 10 years old) and build support for universal binaries (PowerPC Macs haven't been sold in 8 years, Rosetta was removed 3 years ago). It links against Carbon, which is 32-bit only and deprecated entirely as of 10.8. It's my opinion that the OSX port needs modernizing but I don't have any inclination to try if supporting way past end-of-life Apple targets is a requirement.

The most recent changelog entry that dropped support for something OSX-related was 8 years ago:

0.5.0-RC2 (2006-12-31)
------------------------------------------------------------------------
- General Removed support for OSX older than 10.3.9. Either upgrade, or use 0.4.8 (compatible with OSX 10.2)

10.3.9 was released April 15, 2005, 625 days prior.
Comment by Ingo von Borstel (planetmaker) - Wednesday, 04 June 2014, 20:59 GMT
The crash with a zero-length resolution list surely needs to be fixed, independent of a long-needed update of the OSX port.

And agreed, the OSX port grew quite a bit chaotic and is a bit in a state of negligience. If you're prepared to take a sound stab at issues and walk that stony path, that will be very welcome indeed! We talked a bit in #openttd.dev and can probably agree to remove support for some ancient OSX versions meanwhile - they are really ancient. (Even my ancient laptop runs meanwhile 10.6 which isn't new anymore either and already unsupported). Alternatively, or additionally we might also provide different OSX builds for different OSX versions, if needed.
If possible, please make it small steps so that it can be followed and reviewed somewhat :)

Alas, there's one very big stumbling stone with upping the SDK we use: our compile farm. That will need updating or rather re-creation at the same time or we're left without any builds at all: our OSX builds are created by a linux-to-OSX cross-compiler: https://devs.openttd.org/~truebrain/compile-farm/apple-darwin9.txt
An alternative might be, to get a native OSX instance which we have root access to so that a bamboo build node can be installed on it. But prices for those are usually way out-of-proportion compared to our total server hosting costs, so that it's very hard to justify that expense.
Comment by fonsinchen (fonsinchen) - Wednesday, 04 June 2014, 21:14 GMT
It might be appropriate to have two video drivers for Mac OS, targetting different APIs. Like that we could keep support for older versions around while not preventing support for new versions.
Comment by Will Glynn (willglynn) - Wednesday, 04 June 2014, 21:21 GMT
Well, I do know that Travis-CI has OSX support: http://blog.travis-ci.com/2014-05-13-multi-os-feature-available/

Travis is free for open-source projects, though I'm not sure how best to hook it into the OpenTTD build infrastructure – I've only ever used Travis with GitHub projects. Still, it's definitely possible to get access to actual OSX machines, compile stuff, and ship out build artifacts without having a dedicated server.

While I'm here, I'll ask an even more inflammatory question: is the build process negotiable? I would love to use CMake to generate build scripts appropriate for each platform (Makefiles, build.ninja, Visual Studio projects, Xcode projects, etc.) in part because it would make my life better (i.e. keeping multiple out-of-tree builds with different settings).
Comment by Will Glynn (willglynn) - Thursday, 05 June 2014, 14:18 GMT
I got CMake generating an Xcode project on my machine. It seems to work, including settingsgen/strgen. The CMake Makefile and ninja generators work too, all building from the same source tree. And, of course, once the Makefile/ninja/Xcode project is created, it'll regenerate itself automatically as CMakeLists.txt changes. My CMake setup doesn't have all the smarts of config.lib at this time – in particular it doesn't set the proper compile-time flags on platforms other than OSX – but that can be added if desired.

I found that some of the things that seemed complicated before were easy to re-do. Take endianness for example. CMake's include(TestBigEndian) / test_big_endian() does what you think, but better than you might expect: it builds a program with two constant uint16 arrays and greps the resulting binary for the test patterns. While OpenTTD's endianness determination relies on being able to *run* an executable, CMake's standard test relies only on being able to *compile* it, thus determining target endianness reliably even when it differs from the host.

One of the features enabled by having a normal Xcode project is the ability to click a button and run the whole project through the Clang static analyzer. Sure enough, it found this crash bug.
Comment by frosch (frosch) - Thursday, 05 June 2014, 17:37 GMT
The original issue (the crash) has been fixed in r26629 in an back-portable way.

There seeem to be some other topics in this task:
* OSX not supporting 8bpp modes -> 1.5 will address that by disabling 8bpp by default. 1.4 will get the error message from r26629. I don't think it's worth the effort to bother more about 8bpp on OSX.
* New OSX video drivers for this year's API. Maybe join IRC and directly discuss this matter with planetmaker and fonsinchen.
* Using cmake instead of configure. This sounds like it will heavily affect openttd.org's compile farm, infrastructure, releases and OS distributions. I don't see anyone having the time in the near future to deal with all this hassle, esp. since the current system works.
Comment by Ingo von Borstel (planetmaker) - Thursday, 05 June 2014, 17:55 GMT
While IRC might make some discussions easier, alternatively create a new issue (or issues as needed) here for the rework of the OSX video drivers.
As to cmake: that changes many things and breaks it for many people, so it's not something to consider lightly. What could be done much easier, is adding an XCode project file similar to the MSVC project files. But it shouldn't change for each commit and be rather universal for XCode than special to your setup.

Loading...