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

Assert when opening industry directory #7631

Closed
andythenorth opened this issue Jun 29, 2019 · 7 comments
Closed

Assert when opening industry directory #7631

andythenorth opened this issue Jun 29, 2019 · 7 comments
Assignees
Labels
bug Something isn't working regression It used to work, and now it's broken.
Milestone

Comments

@andythenorth
Copy link
Contributor

Version of OpenTTD

commit a83b80bacfa4aa5da2a6043136749b164ac2b191 (HEAD -> 7380)

Using dev version of FIRS (attached), economy parameter set to "Steeltown".

Expected result

OpenTTD doesn't assert when opening industry directory window.

Actual result

OpenTTD asserts when opening industry directory window.

Steps to reproduce

Add attached FIRS, set economy to "Steeltown".
Start a game.
Open industry directory window.
OpenTTD will assert.

ouch

firs-1.tar.zip

@andythenorth
Copy link
Contributor Author

andythenorth commented Jun 29, 2019

(lldb) thread backtrace all
thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
frame #0: 0x00007fff6022d2c6 libsystem_kernel.dylib`__pthread_kill + 10
frame #1: 0x00007fff602e8bf1 libsystem_pthread.dylib`pthread_kill + 284
frame #2: 0x00007fff601976a6 libsystem_c.dylib`abort + 127
frame #3: 0x00007fff6016020d libsystem_c.dylib`__assert_rtn + 324
frame #4: 0x0000000100041b82 openttd`StringParameters::SetParam(this=0x0000000100ea7708, n=20, v=0) at strings_func.h:164:3
frame #5: 0x00000001002a2c15 openttd`SetDParam(n=20, v=0) at strings_func.h:203:24
frame #6: 0x00000001002b5ea5 openttd`IndustryDirectoryWindow::GetIndustryString(this=0x0000000134201590, i=0x0000000139ce8020) const at industry_gui.cpp:1329:4
frame #7: 0x00000001002b20bd openttd`IndustryDirectoryWindow::UpdateWidgetSize(this=0x0000000134201590, widget=2, size=0x00007ffeefbfc3b8, padding=0x0000000101b74ed0, fill=0x00007ffeefbfc3b0, resize=0x00007ffeefbfc3a8) at industry_gui.cpp:1414:47
frame #8: 0x00000001007e664e openttd`NWidgetBackground::SetupSmallestSize(this=0x000000013424bb00, w=0x0000000134201590, init_array=false) at widget.cpp:1799:8
frame #9: 0x00000001007e4580 openttd`NWidgetVertical::SetupSmallestSize(this=0x0000000134203100, w=0x0000000134201590, init_array=false) at widget.cpp:1311:14
frame #10: 0x00000001007e3b50 openttd`NWidgetHorizontal::SetupSmallestSize(this=0x000000013422c000, w=0x0000000134201590, init_array=false) at widget.cpp:1146:14
frame #11: 0x00000001007e4580 openttd`NWidgetVertical::SetupSmallestSize(this=0x0000000134229870, w=0x0000000134201590, init_array=false) at widget.cpp:1311:14
frame #12: 0x00000001007e32ca openttd`NWidgetStacked::SetupSmallestSize(this=0x00000001342cbd50, w=0x0000000134201590, init_array=false) at widget.cpp:1014:14
frame #13: 0x00000001007e4580 openttd`NWidgetVertical::SetupSmallestSize(this=0x00000001342038a0, w=0x0000000134201590, init_array=false) at widget.cpp:1311:14
frame #14: 0x00000001007f5288 openttd`Window::InitializeData(this=0x0000000134201590, window_number=0) at window.cpp:1462:22
frame #15: 0x00000001007f5e6f openttd`Window::FinishInitNested(this=0x0000000134201590, window_number=0) at window.cpp:1830:8
frame #16: 0x00000001002b1965 openttd`IndustryDirectoryWindow::IndustryDirectoryWindow(this=0x0000000134201590, desc=0x0000000100f0d180, number=0) at industry_gui.cpp:1351:9
frame #17: 0x00000001002b1853 openttd`IndustryDirectoryWindow::IndustryDirectoryWindow(this=0x0000000134201590, desc=0x0000000100f0d180, number=0) at industry_gui.cpp:1342:2
frame #18: 0x00000001002a345e openttd`IndustryDirectoryWindow* AllocateWindowDescFront<IndustryDirectoryWindow>(desc=0x0000000100f0d180, window_number=0, return_existing=false) at window_gui.h:883:13
frame #19: 0x00000001002a33ba openttd`ShowIndustryDirectory() at industry_gui.cpp:1525:2
frame #20: 0x000000010071e05b openttd`MenuClickIndustry(index=0) at toolbar_gui.cpp:751:11
frame #21: 0x000000010071bee6 openttd`MainToolbarWindow::OnDropdownSelect(this=0x00000001338b15a0, widget=14, index=0) at toolbar_gui.cpp:2090:26
frame #22: 0x00000001007efadc openttd`DropdownWindow::OnMouseLoop(this=0x000000013b99c180) at dropdown.cpp:318:8
frame #23: 0x00000001007f87a0 openttd`DecreaseWindowCounters() at window.cpp:1951:6
frame #24: 0x00000001007f7e91 openttd`UpdateWindows() at window.cpp:3143:3
frame #25: 0x0000000100807d3b openttd`QZ_GameLoop() at event.mm:706:4
frame #26: 0x0000000100801134 openttd`::-[OTTDMain launchGameEngine:](self=0x000000010400ad40, _cmd="launchGameEngine:", note="ottdmain_launch_game_engine") at cocoa_v.mm:92:2
frame #27: 0x00007fff34227596 CoreFoundation`__CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12
frame #28: 0x00007fff34227510 CoreFoundation`___CFXRegistrationPost_block_invoke + 63
frame #29: 0x00007fff3422747a CoreFoundation`_CFXRegistrationPost + 404
frame #30: 0x00007fff3422f928 CoreFoundation`___CFXNotificationPost_block_invoke + 87
frame #31: 0x00007fff34198384 CoreFoundation`-[_CFXNotificationRegistrar find:object:observer:enumerator:] + 1642
frame #32: 0x00007fff34197737 CoreFoundation`_CFXNotificationPost + 732
frame #33: 0x00007fff3641d06b Foundation`-[NSNotificationCenter postNotificationName:object:userInfo:] + 66
frame #34: 0x00000001008011f6 openttd`::-[OTTDMain applicationDidFinishLaunching:](self=0x000000010400ad40, _cmd="applicationDidFinishLaunching:", note="NSApplicationDidFinishLaunchingNotification") at cocoa_v.mm:107:2
frame #35: 0x00007fff34227596 CoreFoundation`__CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12
frame #36: 0x00007fff34227510 CoreFoundation`___CFXRegistrationPost_block_invoke + 63
frame #37: 0x00007fff3422747a CoreFoundation`_CFXRegistrationPost + 404
frame #38: 0x00007fff3422f928 CoreFoundation`___CFXNotificationPost_block_invoke + 87
frame #39: 0x00007fff34198384 CoreFoundation`-[_CFXNotificationRegistrar find:object:observer:enumerator:] + 1642
frame #40: 0x00007fff34197737 CoreFoundation`_CFXNotificationPost + 732
frame #41: 0x00007fff3641d06b Foundation`-[NSNotificationCenter postNotificationName:object:userInfo:] + 66
frame #42: 0x00007fff318501a8 AppKit`-[NSApplication _postDidFinishNotification] + 312
frame #43: 0x00007fff3184fafb AppKit`-[NSApplication _sendFinishLaunchingNotification] + 208
frame #44: 0x00007fff3184dc4f AppKit`-[NSApplication(NSAppleEventHandling) _handleAEOpenEvent:] + 552
frame #45: 0x00007fff3184d89f AppKit`-[NSApplication(NSAppleEventHandling) _handleCoreEvent:withReplyEvent:] + 688
frame #46: 0x00007fff36466bec Foundation`-[NSAppleEventManager dispatchRawAppleEvent:withRawReply:handlerRefCon:] + 286
frame #47: 0x00007fff36466a69 Foundation`_NSAppleEventManagerGenericHandler + 102
frame #48: 0x00007fff353e8397 AE`aeDispatchAppleEvent(AEDesc const*, AEDesc*, unsigned int, unsigned char*) + 1815
frame #49: 0x00007fff353e7c29 AE`dispatchEventAndSendReply(AEDesc const*, AEDesc*) + 41
frame #50: 0x00007fff353e7b01 AE`aeProcessAppleEvent + 414
frame #51: 0x00007fff334b8e97 HIToolbox`AEProcessAppleEvent + 54
frame #52: 0x00007fff31849c7e AppKit`_DPSNextEvent + 1724
frame #53: 0x00007fff3184871f AppKit`-[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1361
frame #54: 0x00007fff3184283c AppKit`-[NSApplication run] + 699
frame #55: 0x0000000100801f53 openttd`VideoDriver_Cocoa::MainLoop(this=0x0000000102d24c20) at cocoa_v.mm:553:2
frame #56: 0x000000010045892f openttd`openttd_main(argc=1, argv=0x00007ffeefbff6c0) at openttd.cpp:860:30
frame #57: 0x0000000100488ac6 openttd`main(argc=1, argv=0x00007ffeefbff6c0) at unix.cpp:259:12
frame #58: 0x00007fff600f23d5 libdyld.dylib`start + 1
thread #6, name = 'com.apple.audio.IOThread.client'
frame #0: 0x00007fff6022722a libsystem_kernel.dylib`mach_msg_trap + 10
frame #1: 0x00007fff6022776c libsystem_kernel.dylib`mach_msg + 60
frame #2: 0x00007fff33c3deda CoreAudio`HALB_MachPort::SendMessageWithReply(unsigned int, unsigned int, unsigned int, unsigned int, mach_msg_header_t*, bool, unsigned int) + 122
frame #3: 0x00007fff33c3de4f CoreAudio`HALB_MachPort::SendSimpleMessageWithSimpleReply(unsigned int, unsigned int, int, int&, bool, unsigned int) + 45
frame #4: 0x00007fff33c3a39f CoreAudio`HALC_ProxyIOContext::IOWorkLoop() + 1017
frame #5: 0x00007fff33c39df4 CoreAudio`HALC_ProxyIOContext::IOThreadEntry(void*) + 122
frame #6: 0x00007fff33c39956 CoreAudio`HALB_IOThread::Entry(void*) + 72
frame #7: 0x00007fff602e62eb libsystem_pthread.dylib`_pthread_body + 126
frame #8: 0x00007fff602e9249 libsystem_pthread.dylib`_pthread_start + 66
frame #9: 0x00007fff602e540d libsystem_pthread.dylib`thread_start + 13
thread #13, name = 'com.apple.NSEventThread'
frame #0: 0x00007fff6022722a libsystem_kernel.dylib`mach_msg_trap + 10
frame #1: 0x00007fff6022776c libsystem_kernel.dylib`mach_msg + 60
frame #2: 0x00007fff341c4bee CoreFoundation`__CFRunLoopServiceMachPort + 328
frame #3: 0x00007fff341c415c CoreFoundation`__CFRunLoopRun + 1612
frame #4: 0x00007fff341c38be CoreFoundation`CFRunLoopRunSpecific + 455
frame #5: 0x00007fff318516a6 AppKit`_NSEventThread + 175
frame #6: 0x00007fff602e62eb libsystem_pthread.dylib`_pthread_body + 126
frame #7: 0x00007fff602e9249 libsystem_pthread.dylib`_pthread_start + 66
frame #8: 0x00007fff602e540d libsystem_pthread.dylib`thread_start + 13
thread #14
frame #0: 0x00007fff602e53f0 libsystem_pthread.dylib`start_wqthread
thread #15
frame #0: 0x00007fff602e53f0 libsystem_pthread.dylib`start_wqthread

@andythenorth
Copy link
Contributor Author

[7:33pm] andythenorth: should I inspect any frames?
[7:34pm] LordAro: backtrace is probably enough
[7:35pm] Eddi|zuHause: andythenorth: it's trying to push 20 things to the text stack, which seems to blow it up
[7:35pm] andythenorth: that's interesting
[7:36pm] Eddi|zuHause: StringParameters _global_string_params(_global_string_params_data, 20, _global_string_params_type); <-- 20 is the hardcoded limit
[7:36pm] andythenorth: wonder what I've done in FIRS
[7:36pm] andythenorth: and why it didn't trigger until now
[7:38pm] Eddi|zuHause: probably you added enough input/output things to trigger this
[7:38pm] andythenorth: possible I didn't try to use the industry directory previously
[7:39pm] Eddi|zuHause: that's of course also possible
[7:40pm] Eddi|zuHause: andythenorth: as a workaround you can increase that number in the strings.cpp line i quoted, but there should probably be a proper fix
[7:41pm] Eddi|zuHause: there's some more 20s that probably need to change
[7:53pm] LordAro: good ol magic numbers

@LordAro LordAro added bug Something isn't working regression It used to work, and now it's broken. labels Jun 29, 2019
@LordAro
Copy link
Member

LordAro commented Jun 29, 2019

Can confirm that this is an issue from #6867 - FIRS has a "Bulk Terminal" that produces 5 cargoes, which blows through the 20 parameter limit in _global_string_params_data (1 + 4 + 4 + 4 + 4 + 4 == 21) when constructing a string in GetIndustryString

Previously, the maximum number of parameters from this function would've been 9, for 2 produced cargoes

A horribly quick and dirty fix would be to replace lengthof(i->produced_cargo) in both for loops, but the actual solution might require some more thought...

@Eddi-z
Copy link
Contributor

Eddi-z commented Jun 29, 2019

i think the real issue is that GetIndustryString wasn't properly adapted to 16-in-out.

i haven't looked at what the resulting string should look like, but it probably needs to be rewritten to not all be packed in one string (like cargo list or something)

@James103
Copy link
Contributor

Would it be feasible to allow up to 255 parameters in a single string? If the parameter limit was 255, then that would solve many problems with current industry strings caused by having 16 cargoes I/O of a single industry. Also, that would allow Game Scripts such as Simpleton's City Builder to display full town requirements to grow (via option), instead of just the first few cargoes.

@nielsmh nielsmh added this to the 1.10.0 milestone Sep 1, 2019
glx22 added a commit to glx22/OpenTTD that referenced this issue Oct 13, 2019
glx22 added a commit to glx22/OpenTTD that referenced this issue Oct 13, 2019
@glx22
Copy link
Contributor

glx22 commented Oct 13, 2019

Maybe we can fix it using something like master...glx22:fix_7631_alt then open a new issue about the current limitation

glx22 added a commit to glx22/OpenTTD that referenced this issue Oct 13, 2019
glx22 added a commit to glx22/OpenTTD that referenced this issue Oct 13, 2019
glx22 added a commit to glx22/OpenTTD that referenced this issue Oct 13, 2019
glx22 added a commit to glx22/OpenTTD that referenced this issue Oct 13, 2019
@glx22
Copy link
Contributor

glx22 commented Oct 13, 2019

Another option is on master...glx22:industry_directory removing the produced/transported strings and replacing them with cargo sprites

glx22 added a commit to glx22/OpenTTD that referenced this issue Oct 28, 2019
glx22 added a commit to glx22/OpenTTD that referenced this issue Oct 28, 2019
glx22 added a commit to glx22/OpenTTD that referenced this issue Nov 2, 2019
glx22 added a commit to glx22/OpenTTD that referenced this issue Nov 3, 2019
glx22 added a commit to glx22/OpenTTD that referenced this issue Nov 4, 2019
glx22 added a commit to glx22/OpenTTD that referenced this issue Nov 4, 2019
douiwby pushed a commit to douiwby/OpenTTD that referenced this issue Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working regression It used to work, and now it's broken.
Projects
None yet
Development

No branches or pull requests

7 participants