bNetLoadOnClient objects can cause crashes on remote clients

Development Platform & Builds
, ,
1

Repro, in time order on remote client:

  1. Load into map X, which then starts async loading its package.
  2. Object Y whom is in map X and has bNetLoadOnClient == true (eg. a RecastNavMesh) gets created, postloaded, then immediately deleted (see ULevel::InitializeNetworkActors).
  3. Game code puts in another, indirect async load request for map package X. In our example, a serialization in an overridden AWorldSettings::NotifyBeginPlay ends up requesting package X to be loaded.
  4. A second instance of object Y gets created and postloaded. I assume because the second load request of map package X sees that Y has not been created yet, even though it has and already been deleted.
  5. Actors start BeginPlay’ing
  6. GC triggers, destroying the second instance of object Y.
  7. Crash occurs by -1 indexing into the global uobject map, GUObjectArray, during object Y ticking after it has finished being destroyed (has RF_FinishDestroy flag).

Making GC collect every frame (gc.ForceCollectGarbageEveryFrame 1) prevents the crash. Otherwise, the crash is near 100%.

We also noticed this was only occurring on the 2nd load into a server from a client process - load into server, exit to main menu, load into server again. The first load never caused another load request in for map X, but the second did, despite the code being the same (calling FObjectSerializer::Convert on an object in the map).

2

Steps to Reproduce
Repro, in time order on remote client:

  1. Load into map X, which then starts async loading its package.
  2. Object Y whom is in map X and has bNetLoadOnClient == true (eg. a RecastNavMesh) gets created, postloaded, then immediately deleted (see ULevel::InitializeNetworkActors).
  3. Game code puts in another, indirect async load request for map package X. In our example, a serialization in an overridden AWorldSettings::NotifyBeginPlay ends up requesting package X to be loaded.
  4. A second instance of object Y gets created and postloaded. I assume because the second load request of map package X sees that Y has not been created yet, even though it has and already been deleted.
  5. Actors start BeginPlay’ing
  6. GC triggers, destroying the second instance of object Y.
  7. Crash occurs by -1 indexing into the global uobject map, GUObjectArray, during object Y ticking after it has finished being destroyed (has RF_FinishDestroy flag).

Making GC collect every frame (gc.ForceCollectGarbageEveryFrame 1) prevents the crash. Otherwise, the crash is near 100%.

We also noticed this was only occurring on the 2nd load into a server from a client process - load into server, exit to main menu, load into server again. The first load never caused another load request in for map X, but the second did, despite the code being the same (calling FObjectSerializer::Convert on an object in the map).

3

Hi,

Thank you for the report. I unfortunately haven’t been able to reproduce this in my own test project, and I haven’t found any reports of similar issues.

To get a better idea of the problem, I have a couple of questions.

First, is this happening in PIE, standalone editor instances, or packaged instances?

Next, could you provide some more information on your overridden AWorldSettings::NotifyBeginPlay function? How is this handling the serialization that’s causing the level packaged to be reloaded?

Thanks,

Alex

4

Hi Alex! It is happening only in a test build. The instance happens in shipping but attempted -1 indexing into GUObjectArray doesn’t crash the game then (it deals with the nullness). Since the map load process is so different in PIE (the package is already loaded) this does not occur there either.

The serialization is converting bits saved via a custom persistence system (ServerSettingsData) into a uobject on the game instance (ServerSettings). I’m not sure why but the first call to serialize does not retrigger another package load. I believe subsequent ones do because this ServerSettings UObject gets the persistent level as its outer by default when created, which it then loads.

FMemoryReader Reader(GameInstance->ServerSettingsData);
GameInstance->ServerSettings = FObjectSerializer::Convert<AServerSettings>(Reader, World);
5

Hi,

Thank you for the additional information!

Given that the second package load is being triggered from this custom code, it is difficult to say why this might be occurring, so I have a few more questions.

Could you provide some more information on the ServerSettingsData and AServerSettings object? Does the server settings data contain any references to objects in the level or to any dynamically spawned objects?

If possible, it would also help to know more about how FObjectSerializer and its Convert function work. Could you provide a callstack for the map package’s second load request?

Finally, does this occur in a Development build as well?

Thanks,

Alex

6

Sorry for delay!

ServerSettingsData is a TArray<uint8> that is serialized data from a AServerSettings actor, which is a child of AInfo. This is replicated early via a UGameInstance::HandleGameNetControlMessage override. Calls to get server settings on client then does the conversion from the bytes to a AServerSettings actor, that is then cached on the game instance as a UPROPERTY pointer.

This also occurs on Development builds. I suspect this happens in shipping builds too, but shipping doesn’t care about -1 indexing into GUObjectArray - it just ignores if index is invalid.

Here is the callstack. UGenericGameLibrary::GetServerSettings is where Convert is called.

ARecastNavMesh::PostLoad() RecastNavMesh.cpp:622
UObject::ConditionalPostLoad() Obj.cpp:1356
[Inlined] UE::Trace::FChannel::operator|(const UE::Trace::FChannel &) Channel.inl:30
FAsyncPackage2::Event_DeferredPostLoadExportBundle(FAsyncLoadingThreadState2 &, FAsyncPackage2 *, int) AsyncLoading2.cpp:9015
FEventLoadNode2::Execute(FAsyncLoadingThreadState2 &) AsyncLoading2.cpp:5713
FAsyncLoadEventQueue2::ExecuteSyncLoadEvents(FAsyncLoadingThreadState2 &) AsyncLoading2.cpp:5919
FAsyncLoadingThread2::ProcessLoadedPackagesFromGameThread(FAsyncLoadingThreadState2 &, bool &, TArrayView<…>) AsyncLoading2.cpp:9292
FAsyncLoadingThread2::TickAsyncLoadingFromGameThread(FAsyncLoadingThreadState2 &, bool, bool, double, TArrayView<…>, bool &) AsyncLoading2.cpp:9556
FAsyncLoadingThread2::FlushLoading(TArrayView<…>) AsyncLoading2.cpp:11241
FlushAsyncLoading(TArrayView<…>) AsyncPackageLoader.cpp:362
FlushAsyncLoading(int) AsyncPackageLoader.cpp:331
LoadPackageInternal(UPackage *, const FPackagePath &, unsigned int, FLinkerLoad *, FArchive *, const FLinkerInstancingContext *, const FPackagePath *) UObjectGlobals.cpp:1781
LoadPackage(UPackage *, const FPackagePath &, unsigned int, FArchive *, const FLinkerInstancingContext *, const FPackagePath *) UObjectGlobals.cpp:2145
LoadPackage(UPackage *, const wchar_t *, unsigned int, FArchive *, const FLinkerInstancingContext *) UObjectGlobals.cpp:2121
StaticLoadObjectInternal(UClass *, UObject *, const wchar_t *, const wchar_t *, unsigned int, UPackageMap *, bool, const FLinkerInstancingContext *) UObjectGlobals.cpp:1430
StaticLoadObject(UClass *, UObject *, const wchar_t *, const wchar_t *, unsigned int, UPackageMap *, bool, const FLinkerInstancingContext *) UObjectGlobals.cpp:1476
[Inlined] LoadObject(UObject *, const wchar_t *, const wchar_t *, unsigned int, UPackageMap *, const FLinkerInstancingContext *) UObjectGlobals.h:2084
FObjectAndNameAsStringProxyArchive::operator<<(UObject *&) ArchiveUObject.cpp:194
[Inlined] FBinaryArchiveFormatter::Serialize(UObject *&) BinaryArchiveFormatter.h:282
[Inlined] FStructuredArchiveSlot::operator<<(UObject *&) StructuredArchiveSlots.h:364
FObjectProperty::SerializeItem(FStructuredArchiveSlot, void *, const void *) PropertyObject.cpp:237
FPropertyTag::SerializeTaggedProperty(FStructuredArchiveSlot, FProperty *, unsigned char *, const unsigned char *) PropertyTag.cpp:582
UStruct::SerializeVersionedTaggedProperties(FStructuredArchiveSlot, unsigned char *, UStruct *, unsigned char *, const UObject *) Class.cpp:1853
UStruct::SerializeTaggedProperties(FStructuredArchiveSlot, unsigned char *, UStruct *, unsigned char *, const UObject *) Class.cpp:1503
UObject::SerializeScriptProperties(FStructuredArchiveSlot) Obj.cpp:1977
UObject::Serialize(FStructuredArchiveRecord) Obj.cpp:1741
UObject::Serialize(FArchive &) Obj.cpp:1601
AActor::Serialize(FArchive &) Actor.cpp:992
[Inlined] FObjectSerializer::Convert(FMemoryReader &, UObject *, AServerSettings *&) ObjectSerializer.h:64
[Inlined] FObjectSerializer::Convert(FMemoryReader &, UObject *) ObjectSerializer.h:97
UGenericGameLibrary::GetServerSettings(UObject *) GenericGameLibrary.cpp:1889
AGamePlayerController::ClientEnableFeature_Implementation(bool) GamePlayerController.cpp:10841
UFunction::Invoke(UObject *, FFrame &, void *const) Class.cpp:7460
UObject::ProcessEvent(UFunction *, void *) ScriptCore.cpp:2209
AActor::ProcessEvent(UFunction *, void *) Actor.cpp:1465
FObjectReplicator::ReceivedRPC(FNetBitReader &, const FReplicationFlags &, const FFieldNetCache *, const bool, bool &, TSet<…> &) DataReplication.cpp:1402
FObjectReplicator::ReceivedBunch(FNetBitReader &, const FReplicationFlags &, const bool, bool &) DataReplication.cpp:1178
UActorChannel::ProcessBunchInternal(FInBunch &) DataChannel.cpp:3751
[Inlined] UActorChannel::ProcessBunch(FInBunch &) DataChannel.cpp:3843
UActorChannel::ReceivedBunch(FInBunch &) DataChannel.cpp:3558
[Inlined] UChannel::ReceivedSequencedBunch(FInBunch &) DataChannel.cpp:594
UChannel::ReceivedNextBunch(FInBunch &, bool &) DataChannel.cpp:1160
UChannel::ReceivedRawBunch(FInBunch &, bool &) DataChannel.cpp:730
UNetConnection::DispatchPacket(FBitReader &, int, bool &, bool &) NetConnection.cpp:3940
UNetConnection::ReceivedPacket(FBitReader &, bool, bool) NetConnection.cpp:3308
UNetConnection::ReceivedRawPacket(void *, int) NetConnection.cpp:2147
UIpNetDriver::TickDispatch(float) IpNetDriver.cpp:1293
UNetDriver::InternalTickDispatch(float) NetDriver.cpp:2202
[Inlined] Invoke(void (UNetDriver::*)(float), UNetDriver *&, float &&) Invoke.h:66
[Inlined] UE::Core::Private::Tuple::TTupleBase::ApplyAfter(void (UNetDriver::*&)(float), UNetDriver *&, float &&) Tuple.h:320
TBaseUObjectMethodDelegateInstance::ExecuteIfSafe(float) DelegateInstancesImpl.h:689
[Inlined] TMulticastDelegateBase::Broadcast(float) MulticastDelegateBase.h:258
TMulticastDelegate::Broadcast(float) DelegateSignatureImpl.inl:1080
UWorld::Tick(ELevelTick, float) LevelTick.cpp:1341
UGameEngine::Tick(float, bool) GameEngine.cpp:1898
FEngineLoop::Tick() LaunchEngineLoop.cpp:5621
[Inlined] EngineTick() Launch.cpp:60
GuardedMain(const wchar_t *) Launch.cpp:189
LaunchWindowsStartup(HINSTANCE__ *, HINSTANCE__ *, char *, int, const wchar_t *) LaunchWindows.cpp:271
WinMain(HINSTANCE__ *, HINSTANCE__ *, char *, int) LaunchWindows.cpp:339

That was the failure case, which only happens outside of the first connection. This is creating a second RecastNavMesh on client after it has already been destroyed because bNetLoadOnClient==false. The package being requested is the persistent level, which I believe is AServerSettings’ outer since one isn’t set.

I’m not sure why the package load request from the Convert call doesn’t happen on first connection.

7

Hi,

Thank you for the additional information.

If there are dependencies on other dynamic UObjects, which looks to be the case here, there can be problems with serializing an actor like this, as this can pull in unrequired data. Without more info on how the properties of the AServerSettings actor are set up and how they are being set between connections, it’s hard to say why it is only the serialization during the second connection that causes the level package to be loaded a second time.

It may be worth looking into alternate solutions for saving/setting up the server settings that don’t involve serializing a new actor.

Thanks,

Alex

8

That’s fair! It’s definitely not a good setup - it is an inherited codebase.

I made this post just to show that doing can cause bNetLoadOnClient==true objects to be recreated on client and tick after being fully destroyed (RF_FinishDestroy), which seemed quite alarming.

We have a temporary fix in FActorTickFunction::ExecuteTick, early outing of the function if !GUObjectArray.IsValid(Target). The next GC cleans up the object.

9

Hi,

I see, and I appreciate your patience as we investigated this. Like you said, this is a fairly alarming bug, and while your setup is doing some custom stuff, I did want to make sure this wasn’t an issue the base engine could run into.

If you do decide to look into alternate solutions, I think that temporary fix makes sense in the meanwhile. If you have any more questions or run into any other problems, please don’t hesitate to reach out!

Thanks,

Alex