Game crashes on loading world with a java.lang.StackOverflowError after using structure blocks due to multiple block entities in the same location

Please attach the crash report (.minecraft/crash-reports/crash-<DATE>-client.txt) here.

Your Java is heavily outdated. You can find the latest version here. Does this issue still occur after updating to the latest version?

Will this update fix my world or is it permanently corrupted?

Could you please attach your world (zipped) and the structure file?

I can't. It's over 600 mb and the limit here is 10

@@unknown That crash report is for modded 1.7.10; we don't support older versions and modded versions on this tracker (and IIRC 1.7.10 doesn't have structure blocks either), and even then doesn't seem to be related to this specific issue (from the crash) - this is related to malformed NBT data; that's somewhere in the rendering process. Contact the Galacticraft developer; they'll probably be able to help.

If you attached the wrong crash report, that's fine, just get the right one and I'll take a quick look and see if it's this issue or another one.

We've figured out the basic steps to trigger this.

Notes

First off, it's important to note that the duplicate block entities were in the area that was being saved by a structure block - not the loading area (as I initially thought). This can be seen on the world from this ticket ("The Syndicate Epic Seed ! - Copy") - there are still structure blocks left over for saving, and the locations of the duplicate block entities are within there.

Secondly, it was initially noticed that all of the duplicate block entities were in the same chunk, but across multiple structure blocks (and there was one block entity that wasn't duplicated, which was part of a double-chest that stretched across chunk borders at 426 81 47). However, this observation seems to be less important, as the city world from the other ticket had duplicated ones in multiple chunks, and there were a lot of block entities that weren't duplicated (including parts of a double chest). So as far as we can tell, this is a red herring.

Cause of corruption + steps to reproduce

The key observation came from @unknown: Chunk.getTileEntity(BlockPos, EnumCreateEntityType) can create a new block entity when IMMEDIATE is used. And, IMMEDIATE is used when World.getTileEntity(BlockPos) is called. However, this itself doesn't immediately seem like a problem - after all, it's definitely intended that it creates a new block entity.

Yet it is. Grab

(extract into a new folder), which contains a chest in the 0, 0 chunk that was NBTEdited to not have a block entity (more on this later) and a structure block that saves that chest. Open the structure block, and save it. Then save it a second time. Exit and reload the world; you will get this crash (and NBTEditing again shows that there are two chest block entities in the same location).

Why does this happen? It seems like it should be impossible to have two block entities in the same location, as chunks use Map<BlockPos, TileEntity> tileEntities which must have unique keys; it seems impossible for it to initially get into this state.

However, that assumption is false. If a MutableBlockPos is used, it can be put into that map, and then change position, which causes the behavior of the map to be unspecified (but in this case just means that it won't overwrite or get the MutableBlockPos-keyed block entity). Does this ever happen in practice? Yes!

The way that structure blocks save involves a loop that looks like this (simplified):

Template.takeBlocksFromWorld

for (BlockPos.MutableBlockPos blockpos$mutableblockpos : BlockPos.getAllInBoxMutable(start, end)) {
    BlockPos relativePos = blockpos$mutableblockpos.subtract(start);
    IBlockState state = worldIn.getBlockState(blockpos$mutableblockpos);
    TileEntity tileentity = worldIn.getTileEntity(blockpos$mutableblockpos);

    if (tileentity != null) {
        NBTTagCompound tag = tileentity.writeToNBT(new NBTTagCompound());
        tag.removeTag("x");
        tag.removeTag("y");
        tag.removeTag("z");
        list.add(new BlockInfo(relativePos, state, tag));
    } else {
        list.add(new BlockInfo(relativePos, state, null));
    }
}

Now, if there already is a block entity in that location, then getTileEntity will work fine. But if it ends up creating one, that passes the MutableBlockPos to Chunk.addTileEntity(BlockPos, TileEntity), which puts the MutableBlockPos as a key in the tileEntities map (and tells the block entity that it is located at that position via TileEntity.setPos, which makes an immutable copy). Then afterwards, the MutableBlockPos changes, so the block entity is "lost". Saving again adds it the second time (because since the MutableBlockPos changed, the map doesn't return it when checking for existing block entities).

But, when the world is saved, it uses chunk.getTileEntityMap().values(), which will include both block entities keyed by MutableBlockPos instances. And, since the location was captured immutably before, these will both actually save the correct location values (and not the ones wherever the MutableBlockPos ended up). Thus, multiple block entities are saved at the same location.

I've identified a second location where this can also be an issue: /testforblocks also uses a loop with MutableBlockPos that calls getTileEntity. It's a little more complicated to reproduce this bug that way, though, so I won't explain the details; just note that this technichally isn't a bug with structure blocks.

Fix for corruption

Simply put, avoid calling setTileEntity or addTileEntity with a MutableBlockPos. I'd do that by changing this:

Chunk.getTileEntity as it exists currently

if (creationMode == Chunk.EnumCreateEntityType.IMMEDIATE) {
    tileentity = this.createNewTileEntity(pos);
    this.world.setTileEntity(pos, tileentity);
} else if (creationMode == Chunk.EnumCreateEntityType.QUEUED) {
    this.tileEntityPosQueue.add(pos);
}

to this:

Chunk.getTileEntity, fixed

if (creationMode == Chunk.EnumCreateEntityType.IMMEDIATE) {
    tileentity = this.createNewTileEntity(pos);
    this.world.setTileEntity(pos.toImmutable(), tileentity);
} else if (creationMode == Chunk.EnumCreateEntityType.QUEUED) {
    this.tileEntityPosQueue.add(pos.toImmutable());
}

(note that the QUEUED mode does not appear to be used currently, but would still have problems if a MutableBlockPos were used).

For similar reasons, the call to setTileEntity in Chunk.setBlockState should also call toImmutable (however, I don't know of any current situation where setBlockState is called with a MutableBlockPos):

Chunk.setBlockState

this.world.setTileEntity(pos.toImmutable(), tileentity1);

Cause of crash

All of the above explains how two block entities got put in the same location. But it doesn't explain why that causes a crash. Well, first off, here's a snippet of the crash report with MCP names (with 3 lines repeated):

at net.minecraft.tileentity.TileEntityChest.invalidate(TileEntityChest.java:391)
at net.minecraft.world.chunk.Chunk.addTileEntity(Chunk.java:905)
at net.minecraft.world.chunk.Chunk.addTileEntity(Chunk.java:888)
at net.minecraft.world.chunk.storage.AnvilChunkLoader.readChunkFromNBT(AnvilChunkLoader.java:443)
at net.minecraft.world.chunk.storage.AnvilChunkLoader.checkedReadChunkFromNBT(AnvilChunkLoader.java:109)
at net.minecraft.world.chunk.storage.AnvilChunkLoader.loadChunk(AnvilChunkLoader.java:76)
at net.minecraft.world.gen.ChunkProviderServer.loadChunkFromFile(ChunkProviderServer.java:151)
at net.minecraft.world.gen.ChunkProviderServer.loadChunk(ChunkProviderServer.java:103)
at net.minecraft.world.gen.ChunkProviderServer.provideChunk(ChunkProviderServer.java:118)
at net.minecraft.world.World.getChunkFromChunkCoords(World.java:355)
at net.minecraft.world.World.getChunkFromBlockCoords(World.java:347)
at net.minecraft.world.World.getBlockState(World.java:954)
at net.minecraft.tileentity.TileEntityChest.isChestAt(TileEntityChest.java:235)
at net.minecraft.tileentity.TileEntityChest.getAdjacentChest(TileEntityChest.java:212)
at net.minecraft.tileentity.TileEntityChest.checkForAdjacentChests(TileEntityChest.java:200)
at net.minecraft.tileentity.TileEntityChest.invalidate(TileEntityChest.java:391)
at net.minecraft.world.chunk.Chunk.addTileEntity(Chunk.java:905)
at net.minecraft.world.chunk.Chunk.addTileEntity(Chunk.java:888)

This is fairly easy to understand: Chunk.addTileEntity contains this code:

Chunk.addTileEntity(BlockPos, TileEntity)

if (this.getBlockState(pos).getBlock() instanceof ITileEntityProvider) {
    if (this.tileEntities.containsKey(pos)) {
        ((TileEntity)this.tileEntities.get(pos)).invalidate();
    }

    tileEntityIn.validate();
    this.tileEntities.put(pos, tileEntityIn);
}

TileEntityChest.invalidate looks like this:

public void invalidate() {
    super.invalidate();
    this.updateContainingBlockInfo();
    this.checkForAdjacentChests();
}

checkForAdjacentChests calls getAdjacentChest for the 4 cardinal directions. getAdjacentChest first calls isChestAt, and then does some checking on the block entity if there is a chest there. Here's isChestAt:

private boolean isChestAt(BlockPos posIn) {
    if (this.world == null) {
        return false;
    } else {
        Block block = this.world.getBlockState(posIn).getBlock();
        return block instanceof BlockChest && ((BlockChest)block).chestType == this.getChestType();
    }
}

Since the chunk has not been (fully) loaded, calling getBlockState will attempt to load it again. Which repeats the whole process once it attempts to add the second chest block entity, forever, until the stack overflows and the game dies.

Fix for already corrupted worlds

There's two ways of fixing already corrupted worlds: fixing the chest case, or detecting and rejecting duplicate block entities when chunks are loaded. I'd favor the later one.

The chest case

Here's a possible way to handle the chest case: if the world hasn't loaded the chunk containing the block entity yet, don't check for adjacent chests. This requires adding a public method to check if a chunk is loaded.

TileEntityChest.checkForAdjacentChests()

if (this.world == null || !this.world.isChunkLoaded(this.pos, false)) {
    return;
}
// ... original code.  Note that isChestAt also checks that this.world != null

World.isChunkLoaded (new method)

public boolean isChunkLoaded(BlockPos pos, boolean allowEmpty) {
    // other isChunkLoaded is protected
    return isChunkLoaded(pos.getX() >> 4, pos.getZ() >> 4, allowEmpty);
}

The general solution

Here's a more general solution to the problem that rejects all duplicate block entities. The original code to read block entities is this:

AnvilChunkLoader.readChunkFromNBT (unmodified)

NBTTagList teList = compound.getTagList("TileEntities", 10);

for (int i = 0; i < teList.tagCount(); i++) {
    NBTTagCompound teCompound = teList.getCompoundTagAt(i);
    TileEntity te = TileEntity.create(worldIn, teCompound);

    if (te != null) {
        chunk.addTileEntity(te);
    }
}

Checking for duplicate block entities could be done like this:

AnvilChunkLoader.readChunkFromNBT (w/ check)

NBTTagList teList = compound.getTagList("TileEntities", 10);
Object2IntMap<BlockPos> teIndexes = new Object2IntOpenHashMap<>();

for (int i = 0; i < teList.tagCount(); i++) {
    NBTTagCompound teCompound = teList.getCompoundTagAt(i);
    TileEntity te = TileEntity.create(worldIn, teCompound);

    if (te != null) {
        if (teIndexes.containsKey(te.getPos())) {
            LOGGER.warn("Skipping duplicate block entity at {}: already have {}, ignoring {} ({})",
                te.getPos(), teList.getCompoundTagAt(teIndexes.get(te.getPos())), teCompound, te);
            continue;
        }
        teIndexes.put(te.getPos(), i);
        chunk.addTileEntity(te);
    }
}

I include both the new and the original NBT in the log just because I'm paranoid about dropping data without any way to recover it, which makes complicates this slightly; if it seems like that's unnecessary, then this can be simplified to just use a HashSet.

But how does this situation occur without NBT editing?

I don't know. I still haven't figured out how a block which should have a block entity can be created without the block entity.

One idea (from @unknown) was that it could happen when downgrading from 1.11 to 1.10; while this is definitely a possibility, it seems unlikely (this ticket was created early on in the 1.11 snapshots, and there doesn't seem to be any indication that the world was loaded in them; another reproduction happened in 1.12 and it seems unlikely that they downgraded to 1.10 and then loaded in 1.12 again).

Another possibility is that the block was first set with a call to setBlockState using a MutableBlockPos, which would also create a block entity using one. However, I cannot find any evidence of this being the case. But, if this hypothetical situation did occur, then the same problem would happen.

It seems like there must be a way this could happen, but it's still unknown.

Is this still a issue in the latest version of the game(currently 1.13.1)?

If so, please add it to the affected versions, thanks!

The crash was fixed in 17w47a (the flattening) since chests now store their connections directly in the block state and they aren't determined by the block entity. I'm still trying to figure out whether the duplicate block entity situation can still be triggered – it seems like it should still be possible but I'm having trouble with it. I'll probably create a separate ticket for that behavior if it still exists.

The duplicate block entity thing doesn't seem to be happening anymore, but I'm not entirely sure from when (it happens in 18w06a, but in 18w15a it doesn't happen and no TE is saved but one is saved in the structure, and in 18w47b only one is saved somehow). As far as I can tell mutable positions are still used, though it might be the case that that was just fixed in a different version and I haven't searched well enough. Since it seems to be fixed, I'm not going to create a separate issue for it.

The crash was definitely fixed in 17w74a though.