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

[Crash]: OpenTTD crashes on startup (MacBook Pro 2020, M1) #9743

Closed
codetwice opened this issue Dec 12, 2021 · 21 comments
Closed

[Crash]: OpenTTD crashes on startup (MacBook Pro 2020, M1) #9743

codetwice opened this issue Dec 12, 2021 · 21 comments
Labels
OS: MacOS

Comments

@codetwice
Copy link

@codetwice codetwice commented Dec 12, 2021

Version of OpenTTD

12.0, 12.1, nightly

Steps to reproduce

The game crashes on startup, it does not get to the title screen. I think the frame of the main window gets drawn for a brief moment before the app crashes.

System info:

  • MacBook Pro (13-inch, M1, 2020)
  • 16Gb RAM
  • MacOS 12.0.1 Monterey

Reproduction steps (for me):

  1. Start the game by double clicking the OpenTTD icon in the Applications folder
  2. Observe a brief flash on the window of the screen. Could be a window. Stays for about 100ms.
  3. Observe the crash. Read the crash dump on screen

Other info that may be interesting:

Same crash happened on Big Sur (11.6). Then I updated to Monterey, crash is still on.
Tested different versions of OpenTTD. Results:

  • OpenTTD 12.1 - crashes
  • OpenTTD 12.0 - crashes
  • OpenTTD Nightly (20211128-master-g802ca4e722) - crashes
  • OpenTTD 1.11.2 - works fine

I included crash reports for both

  • OpenTTD 12.0 crash on Big Sur
  • OpenTTD 12.1 crash on Big Sur
  • OpenTTD 12.1 crash on Monterey

Upload crash files

OpenTTD 12.1 Monterey crash.txt
OpenTTD 12.0 Big Sur crash.txt
OpenTTD 12.1 Big Sur crash.txt

@nielsmh
Copy link
Contributor

@nielsmh nielsmh commented Dec 12, 2021

From the crash logs, this appears to be related to the touch bar support.

@nielsmh nielsmh added the OS: MacOS label Dec 12, 2021
@codetwice
Copy link
Author

@codetwice codetwice commented Dec 13, 2021

Confirmed. I took the sources from the master branch and compiled locally and hooked on a debugger and it is indeed coming from cocoa_wmd.mm::touchBar.

The line that triggers it is this:

		NSImage *generatedImage = [ self generateImage:num.unsignedIntValue ];

I tried changing this method not to do this by replacing

		NSNumber *num = touchBarButtonSprites[identifier];
		NSImage *generatedImage = [ self generateImage:num.unsignedIntValue ];
		if (generatedImage != nullptr) {
			button.image = generatedImage;
		} else {
			button.title = NSLocalizedString(touchBarFallbackText[identifier], @"");
		}

with

		button.title = NSLocalizedString(touchBarFallbackText[identifier], @"");

, then recompiled and the game starts up alright. Unfortunately my C++ skills are not sufficient and my knowledge of this codebase is inexistent to dig deeper and maybe fix.

I attached a screenshot of VSCode when the bug triggers and attached the call stack. Let me know if I can help any further in figuring this out.

The horror:
OpenTTD crash

The call stack (the only thing I could copy paste out properly):
OpenTTD crash call stack.txt

@nielsmh
Copy link
Contributor

@nielsmh nielsmh commented Dec 13, 2021

Okay after looking a bit, if the output from those local variables on the left pane in your screenshot is right, then n = 4291226460 (close to 2^32, and it's of type uint so presumably 32 bit unsigned) looks like a serious problem. It's a buffer underflow, the address shown in the exception also could look like an allocation page boundary being crossed.
It would imply that n is being initialized to zero or a very large value.

while (dst < dst_end) {
n = std::min<uint>(*src_n++, dst_end - dst);

That snippet is where it gets initialized, but since the loop condition on the line before should protect against the case of dst == dst_end the only way for n to get a bad value would be if src_n contains invalid data somehow. I'm not sure how that would happen.

@codetwice
Copy link
Author

@codetwice codetwice commented Dec 14, 2021

Maybe there's something with the sprite it's trying to display? When the error occurs, it is trying to display spriteId 726 (which is a particular touchbar icon). The call stack contains

DrawSpriteToRgbaBuffer(726)

Maybe the bug could be reproduced without a Mac by trying to draw this sprite just like that?

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Dec 14, 2021

I'd recommend dropping the emojibar support and just backing it out.

Apple haven't announced emojibar is dead yet, but it's a niche sport at this point. 2021 Macbook Pros all dropped it.

@glx22
Copy link
Contributor

@glx22 glx22 commented Dec 14, 2021

726 is SPR_IMG_PAUSE, which is the first sprite in main toolbar.

From clean source tree, there's an easy way to skip the issue, you just need to add return nullptr; before https://github.com/OpenTTD/OpenTTD/blob/master/src/video/cocoa/cocoa_wnd.mm#L431

Won't solve the issue of course, but allows to run the game and see the main toolbar icons, touchbar uses the same sprites.

@glx22
Copy link
Contributor

@glx22 glx22 commented Dec 14, 2021

Assuming touchbar stuff happens during video driver start, I tried things under windows.
With master...glx22:testing_9743 I get a crash, and it also crash if I move the added line to after video driver selection (it's caused by unitialised spritecache).
But that's just me doing blind tests, based on assumptions.
(I don't really understand how 753b1d7 works)

My test was wrong.

@glx22
Copy link
Contributor

@glx22 glx22 commented Dec 14, 2021

Maybe @danskidb can have a look on this issue.

@frosch123
Copy link
Member

@frosch123 frosch123 commented Dec 14, 2021

This seems to be a race condition:

  • the touchbar icons are rendered in the video thread without locking the spritecache.
  • the gameloop thread may be loading the sprites in parallel.

I think to fix this, the rendering of the icons must be moved. The Cocoa driver could do this within VideoDriver::PopulateSystemSprites(). But it cannot do it while rendering the window.

@danskidb
Copy link
Contributor

@danskidb danskidb commented Dec 14, 2021

Hey, I just compiled & tested with latest commit, the 12.1 release from the website and through steam, but did not manage to reproduce the crash. My machine's specifications/OS version are the exact same.

@codetwice Are you running a custom graphics set or do you have an idea if you changed some graphics-related settings? I could try the same config then and try the repro again.

If we can't find it here, indeed backing out my original CL would be best I think. It's a nice to have it for devices that support it but maintainability of the project is more important here.

@codetwice
Copy link
Author

@codetwice codetwice commented Dec 14, 2021

@danskidb My setup is totally vanilla. I didn't change any settings because the game never ever started up. I am however not entirely sure where OpenTTD may be keeping persistent config files around, so is there any particular place where I should look around for possible leftovers from previous installations?

Also, is there anything I could do to help you debug this?

As for removing the touchbar support, it's your call guys. I am not using the touchbar for anything for sure and when I got this Macbook Pro I almost went for the Macbook Air just because I prefer physical F keys over the touchbar.

@danskidb
Copy link
Contributor

@danskidb danskidb commented Dec 14, 2021

@frosch123 I could try to implement this early next week (work largely dictates my time here). cocoa_wnd seems to have a reference to it so rendering/caching the sprites there and retreiving them from cocoa_wnd seems worth a try.

@codetwice just deleted my UserDir/Documents/OpenTTD folder to force a clean start and again, no luck in reproducing. :( after downloading the GFX the buttons and a restart the buttons showed up as expected. Could you try the same? (keep a backup though!)

@frosch123
Copy link
Member

@frosch123 frosch123 commented Dec 14, 2021

@danskidb Thanks, I do not have any OSX setup, so I can't help debug or fix this any further.

@codetwice
Copy link
Author

@codetwice codetwice commented Dec 14, 2021

@danskidb So this is the content directory I was looking for and couldn't find because I was looking in ~/Library instead :( I deleted (renamed) it and the game starts up alright, so there must be something in there which breaks it.

I zipped it for you, but I don't know if it's worth any further investigation as it seems to have some ancient stuff inside (pre-OpenTTD 1.0). Thanks for the hint and sorry for calling out for a bug but I really wasn't aware that my setup wasn't vanilla.

Github didn't like my zip or gz files so here it goes from external:
OpenTTD content dir zipped

@glx22
Copy link
Contributor

@glx22 glx22 commented Dec 14, 2021

I can see different OpenGFX versions but that should not be an issue.
Oh there's a crash.log too (with the expected useless call trace ;) )
Maybe something in the cfg, but it's hard to tell (comparing it with the default one may give a hint though).

@danskidb
Copy link
Contributor

@danskidb danskidb commented Dec 17, 2021

@codetwice Good to hear, at least for now you are able to play the game then 😄
I've tested your content dir and I can repro the crash, thanks for sending that dir over, is very helpful.

@frosch123 Made a quick test caching the images from PopulateSystemSprites and no crash anymore, so it seems you are correct! Thanks for the tip. I'll try making it into a proper cl over the coming days when I got some more time.

@danskidb
Copy link
Contributor

@danskidb danskidb commented Dec 19, 2021

So, I had some more time to investigate today, what i've found:

  • Slight change on my previous statement as I was testing in cocoa_ogl instead of cocoa_v - it also crashed in PopulateSystemSprites - it's also called when the video buffer is locked.
  • In the video driver, MakeWindow seems to be too early to cache the sprite as they fail the SpriteExists() check. I tried caching from the main loop start as well as it fires off the render thread (from what i could see), but same crash still happened unfortunately.
  • When diffing settings, I found the crash seemed to trigger when I changed the setting zoom_min from the default 0 to 2. 1 did not trigger the crash.

@michicc
Copy link
Member

@michicc michicc commented Dec 19, 2021

At least one fault is in DrawSpriteToRgbaBuffer, more exactly src/gfx.cpp:1224. This unconditionally tries to render the sprite with ZOOM_LVL_NORMAL, which does not exists if zoom_min is changed.

@danskidb
Copy link
Contributor

@danskidb danskidb commented Dec 22, 2021

Yeap, changing it to zoom_min prevents the crash, but as expected, this also scaled down the sprite on the image buffer.
My original intent at least was to grab the sprite without scaling (and let OSX handle rescaling if need be). Would that still be a possibility? I tried modifying some of the zoom values but I think I'm still missing some context here on the blitter, since I didn't find the desired result.

@michicc
Copy link
Member

@michicc michicc commented Dec 22, 2021

For each sprite, only the zoom levels between zoom_min and zoom_max will be generated. I think it was initially meant to save on memory on low-end systems, which is why other zoom levels are completely invalid.

The whole touch bar stuff should probably be done in PopulateSystemSprites/ClearSystemSprites of the video driver (with appropriate locking like the OpenGL mouse cursor) which also properly refresh the sprites for e.g. base set changes.

@danskidb
Copy link
Contributor

@danskidb danskidb commented Dec 29, 2021

Heya, sorry for the delay, hope you all had a good xmas if you celebrate it.
 
I see, having only those zoom levels available highlights the critical flaw in the current implementation of the touchbar.
I had a try at your suggestion but I'm not quite getting it right at the moment, and I hate to admit but I don't have enough time to fix this short term.
 
Maybe I'll revisit this later myself at a certain point, but I don't want to hold open this crash for too long either, so I'd like to ask if my original commit could be backed out.

michicc added a commit to michicc/OpenTTD that referenced this issue Dec 31, 2021
michicc added a commit to michicc/OpenTTD that referenced this issue Dec 31, 2021
michicc added a commit to michicc/OpenTTD that referenced this issue Dec 31, 2021
michicc added a commit to michicc/OpenTTD that referenced this issue Dec 31, 2021
michicc added a commit to michicc/OpenTTD that referenced this issue Dec 31, 2021
michicc added a commit to michicc/OpenTTD that referenced this issue Dec 31, 2021
michicc added a commit to michicc/OpenTTD that referenced this issue Dec 31, 2021
michicc added a commit to michicc/OpenTTD that referenced this issue Dec 31, 2021
michicc added a commit that referenced this issue Jan 1, 2022
TrueBrain pushed a commit to TrueBrain/OpenTTD that referenced this issue Jan 3, 2022
TrueBrain pushed a commit to TrueBrain/OpenTTD that referenced this issue Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS: MacOS
Projects
None yet
Development

No branches or pull requests

7 participants