libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
10.14k stars 1.85k forks source link

Catch any objective C exceptions thrown in Cocoa_RegisterApp. Fixes #11437 #11502

Closed maia-s closed 1 day ago

maia-s commented 3 days ago

This fixes SDL_Init(SDL_INIT_VIDEO) throwing an exception on macos if called off the main thread

I don't actually know objective C so this may not be the best way to fix it, but I've verified that this does fix the issue for me. In my patch, Cocoa_RegisterApp sets a message with SDL_SetError, but when testing the fix SDL_GetError reported "SDL_Init failed: No available video device" instead, so maybe that SDL_SetError isn't necessary. Still, it's better than crashing.

Existing Issue(s)

11437

maia-s commented 3 days ago

Oh and this doesn't fix the issue with message boxes hanging that I mentioned in a comment in that issue, but I'm not sure if I was using that correctly.

slouken commented 3 days ago

This is definitely not the right way to fix it. The runtime is throwing this exception for a reason, and you should call SDL_Init(SDL_INIT_VIDEO) on the main thread.

maia-s commented 3 days ago

I know, but returning false from SDL_Init is a lot better than throwing an exception that can't be caught and crash the whole app

maia-s commented 3 days ago

Like I said in the linked issue, this came up with automated testing. I never intentionally want to call SDL_Init on a non-main thread, but I'd expect it to return false and not crash if that does happen

slouken commented 3 days ago

Fair enough. I'll leave this open for review. Thanks for contributing it!

slime73 commented 3 days ago

Still, it's better than crashing.

I'm kind of curious about the sort of use cases where this helps a lot, since the crash already gives a fairly descriptive message about thread misuse.

iOS also has the same restrictions as macOS.

maia-s commented 3 days ago

Objective C exceptions can only be caught in objective C code (and ig swift?), so for that automated test this exception brings down the whole test harness and also makes macos pop up an error dialog that requires human interaction.

I'm not set up for ios dev rn so I can't test that.

It's true that the crash provides more information than "No available video device". Ideally the error message from Cocoa_RegisterApp would be preserved.

maia-s commented 3 days ago

Maybe it could use SDL's logging system to log the exception message instead?

icculus commented 3 days ago

Okay, but this is going to leave Cocoa_RegisterApp half-initialized, isn't it?

If this avoids a crash and causes SDL_Init to cleanly report failure, is there a time this gets called successfully from the main thread later? And if so, why not just call it only from there? And if not, how is the rest of SDL working without a successful Init call?

I'm wondering if a better solution is to have SDL_Init proxy to the main thread, but I also 100% do not want to fall down a rabbit hole where we're proxying everything because people are taking the "only call this on the main thread" documentation as merely a kindly suggestion.

(EDIT: the GitHub app just produced all the other comments after I hit send, so a lot of this was already covered.)

icculus commented 3 days ago

The right solution is probably for Cocoa_RegisterApp (or some other Cocoa-specific piece earlier in video init) to check if it's running from the main thread and return an error immediately if not.

slouken commented 2 days ago

The right solution is probably for Cocoa_RegisterApp (or some other Cocoa-specific piece earlier in video init) to check if it's running from the main thread and return an error immediately if not.

Yep, agreed.

icculus commented 1 day ago

Yep, agreed.

This is done now in 2b744c7df3bb4da133c7bd69df0bc0cf1846e7af.