Players cannot open inventories after server removes their current window

[Muqsit]

Issue description

• Expected result: The server lets players open inventories after closing a player's current window.
• Actual result: The server does not let players open inventories after removing their current window, until they rejoin the server.

Steps to reproduce the issue

1. Place a chest block
2. Open the chest's inventory
3. In console, run /tp yourself ~ ~ ~ to make server invoke Player::removeCurrentWindow()
4. Attempt to open the chest's inventory again

OS and versions

• PocketMine-MP: 4.4.1+dev (git hash: e12e4e8fb878364fbc65b064c01542f5f0e4d4e1-dirty)
• PHP: 8.0.12
• Using JIT: no
• Server OS: linux
• Game version: Win10

Plugins

• If you remove all plugins, does the issue still occur? Yes

Crashdump, backtrace or other files

https://user-images.githubusercontent.com/15074389/172030261-7a9088d5-ef3f-4165-bc5e-351f31f74c19.mp4

2022-06-05 [00:43:59.044] [Server thread/INFO]: [CONSOLE: Given Chest (chest) * 64 to NeedleGalaxy]
2022-06-05 [00:44:01.042] [Server thread/DEBUG]: [Player: NeedleGalaxy] Opening inventory pocketmine\block\inventory\ChestInventory#179361

2022-06-05 [00:44:05.769] [Server thread/DEBUG]: [Player: NeedleGalaxy] Closing inventory pocketmine\block\inventory\ChestInventory#179361
2022-06-05 [00:44:05.769] [Server thread/INFO]: [CONSOLE: Teleported NeedleGalaxy to 1.94, 67, 1.52]

2022-06-05 [00:44:07.154] [Server thread/DEBUG]: [Player: NeedleGalaxy] Opening inventory pocketmine\block\inventory\ChestInventory#179361
2022-06-05 [00:44:07.154] [Server thread/DEBUG]: [NetworkSession: NeedleGalaxy] Deferring opening of new window, waiting for close ack of window 2

2022-06-05 [00:44:11.135] [Server thread/DEBUG]: [Player: NeedleGalaxy] Closing inventory pocketmine\block\inventory\ChestInventory#179361
2022-06-05 [00:44:11.959] [Server thread/DEBUG]: [Player: NeedleGalaxy] Opening inventory pocketmine\block\inventory\ChestInventory#179407
2022-06-05 [00:44:11.959] [Server thread/DEBUG]: [NetworkSession: NeedleGalaxy] Deferring opening of new window, waiting for close ack of window 2

2022-06-05 [00:45:53.928] [Server thread/DEBUG]: [Player: NeedleGalaxy] Closing inventory pocketmine\block\inventory\ChestInventory#179407
2022-06-05 [00:45:53.928] [Server thread/INFO]: NeedleGalaxy left the game
2022-06-05 [00:45:53.937] [Server thread/INFO]: [NetworkSession: NeedleGalaxy] Session closed due to client disconnect

[ghost]

Reproduced.

[dktapps]

This is scuffed. It looks like the client doesn't ack window closes when the block is removed from range?

[Muqsit]

I thought that initially, this is what I've got from observing packet transfers in pocketmine:

When client initiates container close (works fine):
C -> S: ContainerClosePacket(id: 7, server: false)
S -> C: ContainerClosePacket(id: 7, server: false)

When server initiates container close (causes this bug):
S -> C: ContainerClosePacket(id: 8, server: true)
C -> S: ContainerClosePacket(id: 8, server: false)
S -> C: ContainerClosePacket(id: 8, server: false)

When server initiates close, it assigns pendingCloseWindowId a non-null value, and pendingOpenWindowCallback is null at this moment. https://github.com/pmmp/PocketMine-MP/blob/38d6284671e8b657ba557e765a6c29b24a7705f5/src/network/mcpe/InventoryManager.php#L249-L258

But when client sends server the ack packet back, it only resets the value of pendingCloseWindowId if pendingOpenWindowCallback is null. https://github.com/pmmp/PocketMine-MP/blob/38d6284671e8b657ba557e765a6c29b24a7705f5/src/network/mcpe/InventoryManager.php#L272-L277

I tried to rewrite the above like so, because its possible for pendingCloseWindowId to not be null while pendingOpenWindowCallback is null:

if($id === $this->pendingCloseWindowId){
    $this->pendingCloseWindowId = null;
    if($this->pendingOpenWindowCallback !== null){
        $this->session->getLogger()->debug("Opening deferred window after close ack of window $id");
        ($this->pendingOpenWindowCallback)();
        $this->pendingOpenWindowCallback = null;
    }
}

It works well for both - server and client initiated window close, but I am unsure whether this change would still solve #5021 (my latency from my test server is in the 300s and that bug doesn't affect me as frequently).

[dktapps]

It should work, yes. Thanks for the analysis, I overlooked this case during testing.