Fix nested uobject

[Nesquick0]

Hi, I found problem what I believe are nested UObjects, but it should be in your master without any my changes so I want to let you know. I split pull request into 2 commits:

• First is just added test that create Parent and 5 children classes. It is not nice code but I need all 6 classes different and this do it's job.
• Second commit is potential fix.

Repro with only first commit:

• Run "SPUDTest.NestedObject" test in Session Frontend. It should fail that it can't restore objects.
• Or better is to start editor with "-stompmalloc" parameter to enable stomp allocator which checks whether you write to invalid pointer and stop debugger much sooner which helps me very much to find problem.

Context:

Reason why it failed is connected to reference to value in FSpudClassMetadata::ClassDefinitions saved in USpudState::StoreObjectProperties in auto& ClassDef variable. And that when adding 5th element into ClassDefinitions in FSpudClassMetadata::FindOrAddClassDef the array is resized. Don't know whether resize after 4th element is Unreal default or some platform specific.

Please check this callstack:

USpudState::StoreObjectProperties(UObject * Obj, unsigned int PrefixID, TArray<unsigned int,TSizedDefaultAllocator<32>> & PropOffsets, FSpudClassMetadata & Meta, FMemoryWriter & Out, int StartDepth) Line 407
USpudState::StorePropertyVisitor::StoreNestedUObjectIfNeeded(UObject * RootObject, FProperty * Property, unsigned int CurrentPrefixID, void * ContainerPtr, int Depth) Line 116
USpudState::StorePropertyVisitor::VisitProperty(UObject * RootObject, FProperty * Property, unsigned int CurrentPrefixID, void * ContainerPtr, int Depth) Line 84
SpudPropertyUtil::VisitPersistentProperties(UObject * RootObject, const UStruct * Definition, unsigned int PrefixID, void * ContainerPtr, bool IsChildOfSaveGame, int Depth, SpudPropertyUtil::PropertyVisitor & Visitor) Line 304
SpudPropertyUtil::VisitPersistentProperties(UObject * RootObject, SpudPropertyUtil::PropertyVisitor & Visitor, int StartDepth) Line 277
USpudState::StoreObjectProperties(UObject * Obj, unsigned int PrefixID, TArray<unsigned int,TSizedDefaultAllocator<32>> & PropOffsets, FSpudClassMetadata & Meta, FMemoryWriter & Out, int StartDepth) Line 407
USpudState::StoreObjectProperties(UObject * Obj, FSpudPropertyData & Properties, FSpudClassMetadata & Meta, int StartDepth) Line 395
USpudState::StoreGlobalObject(UObject * Obj, FSpudNamedObjectData * Data) Line 372
USpudState::StoreGlobalObject(UObject * Obj, const FString & ID) Line 355
FTestNestedObject::RunTest(const FString & Parameters) Line 312

USpudState::StoreObjectProperties is there twice so auto& ClassDef is invalid when second call of this function resize ClassDefinitions array.

I don't know whether there is better solution than just have array of pointers and create items dynamically instead of array of instances. So feel free to just use or rewrite just the test. I hope it will help.

[sinbad]

Thanks very much for tracking this down, I didn't know about -stompmalloc. Appreciate the tests as well! Making these pointers probably is the most elegant way to solve it. I haven't used raw new/delete in UE before, I assume there isn't a UE-specific wrapper for new/delete we should be using instead to make sure we get correct allocators? Unless they override new/delete, I haven't dug into it yet.

[sinbad]

Actually the one case I have used new is in MakeShareable(), perhaps a TSharedPtr might be best instead of a raw pointer, more UE canon and then we can get rid of the manual delete calls too?

[Nesquick0]

Sure, great idea, I will check that when possible.

Note: I've seen stompmalloc probably from unreal blog in 2015 and finally it was useful for me. https://pzurita.wordpress.com/2015/06/29/memory-stomp-allocator-for-unreal-engine-4/ https://www.unrealengine.com/en-US/blog/memory-corruption-finding-and-fixing-elusive-crashes

[sinbad]

I planned to check this out properly this afternoon anyway so I'll try applying the SharedPtr change on top. Thanks for the links, I'd found the first one but I'm surprised there's so few hits for it in Google!

[sinbad]

I've refactored a bunch of things on top of this PR so they're using TSharedPtr<FSpudClassDef> now instead of references (except in one limited const case), which means everything's a bit safer. Thanks again!