search

yairm210 / Unciv

Open-source Android/Desktop remake of Civ V
Mozilla Public License 2.0
8.34k stars 1.56k forks source link

Serious bug in latest version #6411

Closed yairm210 closed 2 years ago

yairm210 commented 2 years ago

This requires a patch

java.lang.IllegalArgumentException: at java.lang.Enum.valueOf (Enum.java:257) at com.unciv.models.stats.Stat.valueOf (Unknown Source:2) at com.unciv.logic.city.CityStats.getStatsFromTradeRoute (CityStats.kt:95) at com.unciv.logic.city.CityStats.updateBaseStatList (CityStats.kt:467) at com.unciv.logic.city.CityStats.update (CityStats.kt:506) at com.unciv.logic.city.CityStats.update$default (CityStats.kt:499) at com.unciv.logic.city.CityInfo.startTurn (CityInfo.kt:532) at com.unciv.logic.civilization.CivilizationInfo.startTurn (CivilizationInfo.kt:818) at com.unciv.logic.GameInfo.nextTurn$switchTurn (GameInfo.kt:211) at com.unciv.logic.GameInfo.nextTurn (GameInfo.kt:242) at com.unciv.ui.worldscreen.WorldScreen$nextTurn$1.invoke (WorldScreen.kt:646) at com.unciv.ui.worldscreen.WorldScreen$nextTurn$1.invoke (WorldScreen.kt:638) at com.unciv.ui.utils.ExtensionFunctionsKt$wrapCrashHandling$1.invoke (ExtensionFunctions.kt:329) at com.unciv.ui.utils.ExtensionFunctionsKt$wrapCrashHandlingUnit$1.invoke (ExtensionFunctions.kt:356) at com.unciv.ui.utils.ExtensionFunctionsKt$wrapCrashHandlingUnit$1.invoke (ExtensionFunctions.kt:356) at kotlin.concurrent.ThreadsKt$thread$thread$1.run (Thread.kt:30)

xlenstra commented 2 years ago

Isn't this just #6401? Edited as I made a typo in the issue number

yairm210 commented 2 years ago

IMG_20220322_161810

It's possible that this is from a popular mod, and if so I'm less worried, but that's a lot of crashes for 10% rollout

AdityaMH commented 2 years ago

Also in gnk as well

SomeTroglodyte commented 2 years ago

6401 touched exactly that line, it fixed the simple oversight of pulling the wrong parameter as Stat name. It didn't close the potential DOS hole where any mod with a "[0]% [Crap] from Trade Routes" would be able to deliberately crash us - if it gets to load past our unique checks.

Problem is - lots of code, including the initial setTransients on loading a came, seem to not be covered by the spiffy new crash handler. Probably every thread creation could be suspect - I mean a new thread does not inherit exception handlers, no? So, CrashHandlingStage is a fine thing, but its open borders agreements mean it's not a brainless panacea, right?

SomeTroglodyte commented 2 years ago

I'll do a mod to test that now... All is well:

Error: Machu Picchu's unique "[+25]% [DOS] from Trade Routes" contains parameter DOS, which does not fit parameter type stat !

java.lang.AssertionError
    at org.junit.Assert.fail(Assert.java:87)
    at org.junit.Assert.assertTrue(Assert.java:42)
    at org.junit.Assert.assertFalse(Assert.java:65)
    at org.junit.Assert.assertFalse(Assert.java:75)
    at com.unciv.testing.BasicTests.baseRulesetHasNoBugs(BasicTests.kt:95)