[Regression] `TickingBlockEntity#getPos` sometimes returns null

[andriihorpenko]

Version Information

canary-mc1.19.2.0.2.10

Expected Behavior

TickingBlockEntity#getPos should never return null.

Actual Behavior

TickingBlockEntity#getPos may return null, causing unexpected behavior.

Reproduction Steps

Since Canary 0.2.9, TickingBlockEntity#getPos sometimes returns true, which breaks mods relying on non-null value, as there is no @Nullable annotation and no once expects that it can be null. Class net.minecraft.world.level.Level has the following block of code (see below). Some mods are mixing into tickingblockentity.tick() call to do various things (for instance, profiling how much tick time each block entity took).

while(iterator.hasNext()) {
   TickingBlockEntity tickingblockentity = iterator.next();
   if (tickingblockentity.isRemoved()) {
      iterator.remove();
   } else if (this.shouldTickBlocksAt(tickingblockentity.getPos())) {
      tickingblockentity.tick(); // popular injection point for profiling mods
   }
}

Seems like https://github.com/AbdElAziz333/Canary/blob/mc1.19.2/dev/src/main/java/com/abdelaziz/canary/mixin/world/block_entity_ticking/sleeping/LevelMixin.java should've prevented that, but according to my crash report, this check basically didn't run, letting tickingblockentity.tick() actually occur.

This is my mixin making this issue possible:

@WrapOperation(method = "tickBlockEntities", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/level/block/entity/TickingBlockEntity;tick()V"))
private void captureBlockEntityTickTime(TickingBlockEntity instance, Operation<Void> original) {
    System.out.println(instance.getPos()); // this causes NPE
}

Other Information

Note: this is not happening on Canary 0.2.8. This regression happens since 0.2.9 onwards. Crash report: crash-2024-01-02_00.44.10-server.txt

[AbdElAziz333]

Please try the new update from here for MC 1.19.2.

[andriihorpenko]

Note, 0.3.0 release became incompatible with ServerCore https://bytebin.lucko.me/pDmO5UTOqO

[andriihorpenko]

And no, 0.3.0 has exactly the same issue as stated above.The crash report is identical.

[AbdElAziz333]

Can you please tell me the steps to produce this issue?

[andriihorpenko]

Here are sufficient steps to get a crash:

• Install canary-mc1.19.2.0.3.0 (or canary-mc1.19.2.0.2.9)
• Add the following mixin in any sample mod:

import com.llamalad7.mixinextras.injector.wrapoperation.Operation;
import com.llamalad7.mixinextras.injector.wrapoperation.WrapOperation;
import net.minecraft.world.level.Level;
import net.minecraft.world.level.block.entity.TickingBlockEntity;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.injection.At;

@Mixin(Level.class)
public abstract class LevelMixin {
    @WrapOperation(method = "tickBlockEntities", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/level/block/entity/TickingBlockEntity;tick()V"))
    private void captureBlockEntityTickTime(TickingBlockEntity instance, Operation<Void> original) {
        System.out.println(instance.getPos());
    }
}

You should get a crash like this: crash-2024-01-02_00.44.10-server.txt

[AbdElAziz333]

After testing this with only Canary and the code you provided, in both client and server it just prints the block positions like this:

BlockPos{x=-77, y=5, z=-268}
BlockPos{x=-82, y=11, z=-201}
BlockPos{x=-60, y=6, z=-284}

and doesn't crash at all, do you have specific mods to try with?

[andriihorpenko]

Maybe try with ModernFix and ServerCore, I guess they both provide a lot of optimizations as well. Mind that ServerCore crashes with latest 0.3.0 version.

[AbdElAziz333]

Can you try remove Canary to see if the issue still happens?

[andriihorpenko]

It does not happen without Canary, and it does not happen if you have Canary 0.2.8 and below. I had Canary for quite a long time and this crash happens after updating to 0.2.9 and above.

[AbdElAziz333]

Can you please try it in the latest update for 1.19.2 from here.

[andriihorpenko]

This fixes ServerCore incompatibility, but the original crash remains the same as it has nothing to do with ServerCore. I might try binary search to determine what exactly causes incompatibility with the Canary mixin states above as right now I have absolutely no clue. In the crash report you can see what mixins were applied to Level class, this might help you find out which mods are somehow mixing into Level and causing such an unexpected behavior.

[andriihorpenko]

After testing this with only Canary and the code you provided, in both client and server it just prints the block positions like this:

BlockPos{x=-77, y=5, z=-268}
BlockPos{x=-82, y=11, z=-201}
BlockPos{x=-60, y=6, z=-284}

and doesn't crash at all, do you have specific mods to try with?

As for this: try simulating a situation when position is null. I don't know when this happens, seems like it's somehow connected to new "sleeping" thing.

[andriihorpenko]

I think I know how this happens. This mixin works just fine. The thing is that the position becomes NULL in a slightly different place.

Let's look at the following code again:

// Class net.minecraft.world.level.Level

} else if (this.shouldTickBlocksAt(tickingblockentity.getPos())) {
    // position is not NULL yet
    tickingblockentity.tick();
    // position became NULL
 }

Basically, after a block entity tick, its position may become null, causing unexpected behavior. The only block entity wrapper class I can think of is com.abdelaziz.canary.common.block.entity.SleepUntilTimeBlockEntityTickInvoker, which returns position dynamically, potentially causing such an unexpected behavior.

[andriihorpenko]

No longer valid, so it's safe to close