yairm210 / Unciv

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

Mysterious crash on pressing next turn #4570

Closed xlenstra closed 3 years ago

xlenstra commented 3 years ago

A discord user send me a save file, which crashed when pressing the 'next turn' button. I tried this out myself, and somehow doing this resulted in a stack overflow exception in the json parser when creating an autosave. I have no idea how or why this would even happen. If someone does have an idea, the save file is appended to this bug report.

Mysterious Crash.txt

xlenstra commented 3 years ago

I've looked into it further, and there appears to be some reason to the riddle. First note that this is a modded save, using the outdated civ5expansion mod. During the turn, Greece picks the final policy of the autocracy policy tree, which yields him a temporary combat boost. But, due to it being an outdated mod, this is a unique with no parameters. When the save file is created, the temporary uniques are stored in a map, mapping a Unique object to the amount of turns the boost should still remain (note to self: this is a terrible choice, it should have mapped the unique text including [] to the amount of turns the boost should still remain, creating a unique from this is easy and we wouldn't run into these weird crashes). This unique is then parsed into a JSON object. Note for the reader, the Unique object is the following type:

class Unique(val text:String){
    val placeholderText = text.getPlaceholderText()
    val params = text.getPlaceholderParameters()
    /** This is so the heavy regex-based parsing is only activated once per unique, instead of every time it's called
     *  - for instance, in the city screen, we call every tile unique for every tile, which can lead to ANRs */
    val stats: Stats by lazy {
        val firstStatParam = params.firstOrNull { Stats.isStats(it) }
        if (firstStatParam == null) Stats() // So badly-defined stats don't crash the entire game
        else Stats.parse(firstStatParam)
    }
}

As you can see, this type has a stats parameter. I don't know how, but when this unique is parsed into a JSON object, the following is written to the file:

{class:com.unciv.models.ruleset.Unique,placeholderText:+20% attack bonus to all Military Units for 30 turns,params:
{class:kotlin.collections.EmptyList,items:[]},stats$delegate:{class:kotlin.SynchronizedLazyImpl,initializer:
{class:com.unciv.models.ruleset.Unique$stats$2,arity:0},_value:{class:kotlin.UNINITIALIZED_VALUE},lock:
{class:kotlin.SynchronizedLazyImpl,initializer:{class:com.unciv.models.ruleset.Unique$stats$2,arity:0},_value:
{class:kotlin.UNINITIALIZED_VALUE},lock:{class:kotlin.SynchronizedLazyImpl,initializer:
{class:com.unciv.models.ruleset.Unique$stats$2,arity:0},_value:{class:kotlin.UNINITIALIZED_VALUE},lock:
[...]

It appears that the constructor for the stats is not called correctly or something, so everything starts crashing down and burning.

Altough this issue surfaced as a mod specific issue at the start, the problem is also caused by the brilliant design choice to save Unique objects directly to the save file. This is, however, almost impossible to rectify without either invalidating all saves, or creating another variable and deprecating the old one.

However, this problem is also solved for now. The old autocracy unique was deprecated in the last commit to master, and the new autocracy unique doesn't crash because it has parameters and this confuses the constructor of the Unique in such a way that a Stats() object is somehow generated. So I actually vote to completely ignore this for now, and maybe later when we add more temporary uniques this problem will resurface, and we'll deal with it then.

What is your opinion?

yairm210 commented 3 years ago

Fair enough. If this problem specific to older versions, and cannot be easily solved, a 'solution' would be to deprecate the older 'unique in save' entirely and not allow it to even be loaded (fields that do not appear in the class are ignored in json parsing)

SomeTroglodyte commented 3 years ago

lazies should never be serialized, period. I vote marking all of them @Transient and deal with the consequences - if any. The Uniques as shown would IMHO not have a problem because the lazy delegate will be called after a load.

SomeTroglodyte commented 3 years ago

Actually I think some should open an issue for Gdx telling them that serializing lazies is a bug, and just because they're a java only library doesn't mean they couldn't learn a little kotlin... :roll_eyes: :zany_face:

yairm210 commented 3 years ago

Closing this as not relevant.