`firstPlayed` data corruption issue

[OfficialTheosis]

I recently discovered an issue where if a player logs out while you have their inventory open, when you close their inventory, their firstPlayed timestamp will be overwritten with the current time.

Before I realised OpenInv was the culprit, I had the following conversation in PaperMC's Discord server:

As you can see, according to masmc05, who is a Paper contributor, if a plugin is accessing a player's data file directly (which OpenInv must be doing in order to read offline players' inventories), then it can cause this issue.

Steps to reproduce:

1. Set up a server with two different players' data in the world's playerdata folder.
2. Take note of Player A's firstPlayed timestamp - this can be viewed with programs like NBTExplorer, inside the bukkit compound tag.
3. Start the server, and join it with both Player A and Player B.
4. Have Player B open Player A's inventory.
5. Log out with Player A.
6. Close Player A's inventory on Player B.
7. Observe as Player A's firstPlayed timestamp has been overwritten.

[Jikoo]

This isn't related to direct file access per se. Not doing so is way, way worse - the Bukkit API is full of holes related to loading and saving player data. Calling load and save on an offline mounted player will delete their mount, for example. I would love for Paper to improve that area, but as can be seen from the amount of code OI has compensating for it, the diff would probably be really noisy, and I think only myself and a small handful of other plugins would potentially benefit. OI still would have to rely on NMS either way, because it ensures that inventories stay synced by using the same internal items list as the real inventories for its view of the inventory opened.

Either way, should be an easy fix. The fact that the tag is bukkit.firstPlayed means it's been there all along, weird that I missed it when doing the setup for this. I thought I'd tested all the various time APIs. Which Paper version is this? I wonder if it was working by accident and they changed the load process slightly.

[OfficialTheosis]

This isn't related to direct file access per se. Not doing so is way, way worse - the Bukkit API is full of holes related to loading and saving player data. Calling load and save on an offline mounted player will delete their mount, for example. I would love for Paper to improve that area, but as can be seen from the amount of code OI has compensating for it, the diff would probably be really noisy, and I think only myself and a small handful of other plugins would potentially benefit. OI still would have to rely on NMS either way, because it ensures that inventories stay synced by using the same internal items list as the real inventories for its view of the inventory opened.

Either way, should be an easy fix. The fact that the tag is bukkit.firstPlayed means it's been there all along, weird that I missed it when doing the setup for this. I thought I'd tested all the various time APIs. Which Paper version is this? I wonder if it was working by accident and they changed the load process slightly.

Paper 1.21.11 build 126.

I don't know if only that tag is getting messed with, that's just the one I noticed.

[Jikoo]

I don't know if only that tag is getting messed with, that's just the one I noticed.

I'll do a review of it along the way, yeah. We do already revert the session data, but those are tags that don't actually "load" because they're always going to be overwritten by the current play session for the player. It's weird that a piece of data that should be preserved in the player itself isn't loading. My working idea going in is that Paper might have reverted the change that made it so we didn't have to explicitly load "extra data" aka all of the Bukkit/Paper API data when loading player data, but that's usually something I look for when checking the tags to preserve, so I'm not confident in that.

[Jikoo]

I believe I found it - when injecting the CB-side player replacement to fix issues with the default save process, I neglected to copy the extra tag values from the replaced version. It looks like this has probably been an issue since I tried to generify the process more. Should only affect the bukkit.xyz values, and then only the ones that aren't reverted already.