Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mac OS X 10.10 Yosemite crash fix #324

Merged
merged 2 commits into from Oct 13, 2014
Merged

Mac OS X 10.10 Yosemite crash fix #324

merged 2 commits into from Oct 13, 2014

Conversation

jberney
Copy link
Contributor

@jberney jberney commented Oct 8, 2014

The Yosemite crash happens when we call CFRunLoopAddObserver. It doesn't seem to like runLoopRef. Replacing the first argument to this call with CFRunLoopGetCurrent() avoids the crash, but the app then hangs because CFRunLoopWakeUp doesn't cause observerRef (NameHALThread) to fire, so the semaphore is not posted. Commenting the semaphore wait line allows the game to boot in Yosemite, and seems to play totally normally. This doesn't really fix our problem (we are trying to name the HAL thread, and a catastrophic error occurs), but it shows us what the problem is.

We are obtaining runLoopRef using an API call that was deprecated years ago. However, the Apple-approved replacement code doesn't produce a runLoopRef that avoids the crash, either. So I'm not sure how we get the correct runLoopRef so we can name the thread.

This is my first time looking at the code base, but I don't see the value of naming the thread. In the interest of keeping Stepmania compatible with the upcoming operating system, I propose we just remove the thread-naming logic.

Jonathan Berney and others added 2 commits October 7, 2014 19:48
The Yosemite crash happens when we call CFRunLoopAddObserver. It doesn't seem to like runLoopRef. Replacing the first argument to this call with CFRunLoopGetCurrent() avoids the crash, but the app then hangs because CFRunLoopWakeUp doesn't cause observerRef (NameHALThread) to fire, so the semaphore is not posted. Commenting the semaphore wait line allows the game to boot in Yosemite, and seems to play totally normally. This doesn't really fix our problem (we are trying to name the HAL thread, and a catastrophic error occurs), but it shows us what the problem is.

We are obtaining runLoopRef using an API call that was deprecated years ago. However, the Apple-approved replacement code doesn't produce a runLoopRef that avoids the crash, either. So I'm not sure how we get the correct runLoopRef so we can name the thread.

This is my first time looking at the code base, but I don't see the value of naming the thread. In the interest of keeping Stepmania compatible with the upcoming operating system, I propose we just remove the thread-naming logic.
@freem freem added the macOS label Oct 8, 2014
@wolfman2000
Copy link
Contributor

Can anyone else on the Mac side provide an idea as to why we were naming the thread? @shakesoda or @dtinth perhaps?

@shakesoda
Copy link
Member

Probably for crash logs and debugging.

  • Colby

On Tue, Oct 7, 2014 at 8:22 PM, Jason Felds notifications@github.com
wrote:

Can anyone else on the Mac side provide an idea as to why we were naming
the thread? @shakesoda https://github.com/shakesoda or @dtinth
https://github.com/dtinth perhaps?


Reply to this email directly or view it on GitHub
#324 (comment).

@quietly-turning
Copy link
Contributor

I tested this by building with Xcode 6.0.1 under OS X 10.9.5. It compiled and ran as normal.

I transferred the build to my 10.10 beta partition, and tested there. It ran as normal.

I don't have a dev environment setup in my OS X 10.10 partition yet, so I didn't try building in Yosemite, but this pull request does what it set out to do: allows SM to run in Yosemite. 👍

@dtinth
Copy link
Contributor

dtinth commented Oct 8, 2014

I don't know much about the internal workings of OS X, so I can't give an in-depth comment about this. However, I'd say this makes sense. 👍

RageSoundDriver_AU *This = (RageSoundDriver_AU *)inRefCon;
CFRunLoopObserverInvalidate( observer );
CFRelease( observer );
This->m_pNotificationThread = new RageThreadRegister( "HAL notification thread" );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a reminder to eventually find a way to name threads again.

wolfman2000 added a commit that referenced this pull request Oct 13, 2014
Mac OS X 10.10 Yosemite crash fix
@wolfman2000 wolfman2000 merged commit 578f0b8 into stepmania:master Oct 13, 2014
@nilbus
Copy link

nilbus commented Oct 17, 2014

Thanks @jberney and @wolfman2000 for the quick resolution! 👍 When do you think we can expect a binary release of Alpha 4a? The sooner the better, now that Yosemite is publicly released. In the mean time, I'll attempt a build in XCode 6.0.1.

@nilbus
Copy link

nilbus commented Oct 17, 2014

Oops, I just saw Shakesoda's announcement—over the weekend. :-)

@radesyaa
Copy link

radesyaa commented Nov 7, 2014

seriously I dont know how to fix this problem in my Yosemite T-T

@nilbus
Copy link

nilbus commented Nov 7, 2014

Download and run version alpha 4a

@radesyaa
Copy link

radesyaa commented Nov 7, 2014

where I can get this version? ._.

@nilbus
Copy link

nilbus commented Nov 7, 2014

@wolfman2000 @shakesoda It looks like this was never posted on the Stepmania.com downloads page!

@radesyaa https://github.com/stepmania/stepmania/releases

Also make sure you're not running in fullscreen mode, or hit Alt+Enter if you are. Fullscreen doesn't work any more.

@radesyaa
Copy link

radesyaa commented Nov 7, 2014

@nilbus thankyoou so much !!!!! ><

@jberney
Copy link
Contributor Author

jberney commented Nov 7, 2014

(I submitted a pull request to band-aid the full screen regression, #358)

@quietly-turning
Copy link
Contributor

@jberney As one of the few other active people who can build on OS X, I apologize for not having the time to test your fullscreen fix, yet. (My thesis is eating my free time for breakfast, lunch, and dinner.)

I'll try to get to it tonight, and hopefully it can be merged soon.

@jberney
Copy link
Contributor Author

jberney commented Nov 7, 2014

No worries!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants