Prevent use of stale pointers in item_location

[jbytheway]

Summary

SUMMARY: Infrastructure "Prevent use of stale pointers in item_location, which has been a source of various strange bugs over the years"

Purpose of change

item_location is used to store references to items in activities and various other contexts.

It was primarily based on an item* member, which was quite unsafe as items could move or be destroyed mid-activity (especially resumable activities like butchering).

You can see an example of the simplest sort of way this could go wrong in the item_location_doesnt_return_stale_map_item test I added on this PR.

We need a safer way to keep track of items.

Describe the solution

Create a new safe_reference class template which automatically invalidates itself if a targeted object is destroyed or assigned to.

Store a safe_reference<item> instead of a raw item* in item_location.

Incidentally, this makes checking the validity of item_locations much more performant than it used to be.

Describe alternatives you've considered

This safe_reference is built on shared_ptr, but using it in a somewhat unorthodox way. The more normal thing to have done would be to store all items using shared_ptrs, but I don't think that's actually a good idea, and it would heave required massive surgery to the entire codebase, whereas this is a minimal change.

Additional context

With this change, I believe item_location is now memory-safe.

However, it's still not doing the right thing in all cases.

It can end up pointing to the wrong item through a save-load cycle because of the lazy loading used after load. To fix that we need to ensure all item_location objects get unpacked before any items move after game load. @ifreund suggested that we don't need lazy loading at all, and we could just resolve all these items as the item_locations are deserialized. Given how widely item_location is used, it would be hard to verify that that would be safe.

Alternatively, we could do a pass over all item_location objects after the game is loaded and unpack them all then. But even that probably isn't enough. Consider for example how to handle NPCs who are mid-activity when their map gets unloaded. How do we fix that all up properly when the map is reloaded? Because the NPC object isn't serialized but the map is.

In any case, that's all out of scope for this PR.

[ifreund]

Thanks for doing this, it would have been way messier if I was the one implementing it. Making a wrapper for the weak and shared pointer trick was a great idea.

I'm still thinking that we may not need the lazy loading, though you're right that it's super difficult to validate. As I pointed out earlier, it seems that vehicle item_locations already depend on the map being loaded to be successfully deserialized.

[ifreund]

Looks like this and other functions in item_location::impl::nowhere need to be marked as override

[ifreund]

Jenkins Rebuild