Have the host load a package, and then stop referencing the package
Client rpcs to the server to load the package with a different netguid and then stops referencing the package
Host does a garbage collection
Host loads the package again but still references the package
Client loads the package again
Steps to Reproduce
We’ve been encountering a hard to reproduce crash in the package map client with this specific check being triggered:
checkf(!It.Value().Object.IsValid() || NetGUIDLookup.FindRef(It.Value().Object) == It.Key() || It.Value().ReadOnlyTimestamp != 0, TEXT("Failed to validate ObjectLookup map in UPackageMap. Object '%s' was not in the NetGUIDLookup map with with value '%s'." ), *It.Value().Object.Get()->GetPathName(), *It.Key().ToString());The reason this check was being triggered was tricky to track down, but I finally managed to create a repro case.
In our setup, we have a client connecting to a listen server. We do allow the client to remap netguids. The client can change weapons by sending an RPC with which asset he wants to change to. If they together perform these steps:
1. Host and client are both joined into a game 2. Host switches to a weapon with a certain BP class (let's pretend it's a pistol), and switches off of that weapon to any other weapon (this is make sure package is loaded) 3. Client switches to the same weapon with the same BP class and switches off that weapon as well 4. The host then runs garbage collection forcibly 5. The host switches to pistol again, (loading the package again since it was garbage collected after nothing referenced it) 6. The client switches to the pistol as well
When these steps are done, the package map on the host ends up running through this code in PackageMapClient:
`if (bIsNotReadOnlyOrAllowRemap || !NetGUIDLookup.Contains(Object))
{
if (CacheObjectPtr->ReadOnlyTimestamp > 0)
{
UE_LOG( LogNetPackageMap, Warning, TEXT( “GetObjectFromNetGUID: Attempt to reassign read-only guid. FullNetGUIDPath: %s” ), *FullNetGUIDPath( NetGUID ) );
if (bAllowClientRemap)
{
CacheObjectPtr->ReadOnlyTimestamp = 0;
}
}
NetGUIDLookup.Add( Object, NetGUID );`That last step being the one where it stomps the previous entry into the NetGUIDLookup which actually had a higher net guid.
Later when the host performs a seamless travel, inside of void FNetGUIDCache::CleanReferences()
else if (FoundGuid < Guid) { ObjectLookup[FoundGuid].ReadOnlyTimestamp = Time; FoundGuid = Guid; }
this code will trigger. The object is in the ObjectLookup twice, and it will mark the ‘older’ guid as expiring but keep the new one. Since the NetGUIDLookup entry was stomped with the older one, it will crash when it hits the check in the loop here:
`for (auto It = ObjectLookup.CreateIterator(); It; ++It)
{
check(!It.Key().IsDefault());
check(It.Key().IsStatic() != It.Key().IsDynamic());
checkf(!It.Value().Object.IsValid() || NetGUIDLookup.FindRef(It.Value().Object) == It.Key() || It.Value().ReadOnlyTimestamp != 0, TEXT("Failed to validate ObjectLookup map in UPackageMap. Object '%s' was not in the NetGUIDLookup map with with value '%s'." ), *It.Value().Object.Get()->GetPathName(), *It.Key().ToString());
}`
I have a possible fix, but wanted to run it by someone familar with the code.
I wanted to replace this code in PackageMapClient
`if (bIsNotReadOnlyOrAllowRemap || !NetGUIDLookup.Contains(Object))
{
if (CacheObjectPtr->ReadOnlyTimestamp > 0)
{
UE_LOG( LogNetPackageMap, Warning, TEXT( “GetObjectFromNetGUID: Attempt to reassign read-only guid. FullNetGUIDPath: %s” ), *FullNetGUIDPath( NetGUID ) );
if (bAllowClientRemap)
{
CacheObjectPtr->ReadOnlyTimestamp = 0;
}
}
NetGUIDLookup.Add( Object, NetGUID );`with the following:
`if (bIsNotReadOnlyOrAllowRemap || !NetGUIDLookup.Contains(Object))
{
if (CacheObjectPtr->ReadOnlyTimestamp > 0)
{
UE_LOG( LogNetPackageMap, Warning, TEXT( “GetObjectFromNetGUID: Attempt to reassign read-only guid. FullNetGUIDPath: %s” ), *FullNetGUIDPath( NetGUID ) );
if (bAllowClientRemap)
{
CacheObjectPtr->ReadOnlyTimestamp = 0;
}
}
// If the guid already exists, replace it if this guid is newer. Otherwise, keep the new one
if (NetGUIDLookup.Contains(Object))
{
if (NetGUIDLookup[Object] < NetGUID)
{
NetGUIDLookup[Object] = NetGUID;
}
}
else
{
NetGUIDLookup.Add( Object, NetGUID );
}`
Hi,
Thank you for the report. Unfortunately, I haven’t been able to reproduce the issue in my own test project following the steps outlined here, making it difficult to comment on the fix.
Would you be able to provide a basic sample project reproducing the issue?
Thanks,
Alex
Sorry, I can’t pull out the relevant pieces from our project easily. If it’s relevant, our “weapons” are primary data assets that are not always loaded, so it has to go through the Load step on the server and client independently.I don’t know if it’s relevant, but the weapons have meshes, and animations associated with them, and they get spawned on the client locally as well as the server which then spawns an entity to replace the client local version.
When both players switched off the weapon, it was cleaned up so I think it’s probably important that nothing else references the “weapons”
We knew it would crash if it entered this section in PackageMapClient at all while NetGUIDLookup.Contains(Object)
`// Assign the guid to the object
// We don’t want to assign this guid to the object if this guid is timing out
// But we’ll have to if there is no other guid yet
const bool bAllowClientRemap = !bIsNetGUIDAuthority && GbAllowClientRemapCacheObject;
const bool bIsNotReadOnlyOrAllowRemap = (CacheObjectPtr->ReadOnlyTimestamp == 0 || bAllowClientRemap);
if (bIsNotReadOnlyOrAllowRemap || !NetGUIDLookup.Contains(Object))
{`With the repro steps above, does it even enter into this portion of code? (I had added a
ensure(!NetGUIDLookup.Contains(Object)) in that section locally just to confirm that it hit that section of code. If it ever did, I knew the package map was in a bad state. Having said that, the full crash didn’t repro all the time because during level cleanup, if all references to the “weapons” cleaned up correclty, the “weapons” would get cleaned up and no longer crash inside of void FNetGUIDCache::CleanReferences()
Hi,
Apologies for the delay in getting back to you, and thank you for the extra info! By converting the actors in my test to primary data assets, I was able to reproduce the assert you’re seeing.
I’ve discussed your proposed workaround with a dev more familiar with the PackageMapClient. While it does generally seem as though the NetGUID should always be remapped to a more recent (i.e. higher) value, we weren’t confident that this would always be the case. (There was also a change made very recently that made NetGUIDs no longer monotonically increasing. Although, I believe this is disabled by default and was implemented for an experimental plugin, so it shouldn’t affect your project.)
Another potential workaround you could try would be to ensure the server doesn’t assign the asset’s package a new NetGUID when reloading it after garbage collection. While the associated objects in the PackageMapClient’s lookup maps will be nulled after garbage collection, the ObjectLookup map should still have the package’s path. By finding this, we can instead reuse the original NetGUID for the package, instead of assigning a new one on the server that later gets stomped by this original value sent by the client.
For instance, I added this to my repro project, in FNetGUIDCache::GetOrAssignNetGUID after it checks if the NetGUID retrieved from the NetGUIDLookup is valid:
`// May be a package that was previously garbage collected, check if ObjectLookup has an entry with a matching path
else if (Object->GetOuter() == nullptr)
{
for (auto It = ObjectLookup.CreateIterator(); It; ++It)
{
FNetGuidCacheObject CacheObj = It.Value();
FName CacheObjPath = CacheObj.PathName;
if (CacheObjPath == Object->GetPathName())
{
//If there’s a matching path, set the Object pointer on the CacheObject to this package, and return the original NetGUID
It.Value().Object = Object;
NetGUID = It.Key();
//Make sure to add this back to the NetGUIDLookup as well
if (!NetGUIDLookup.Contains(Object))
{
NetGUIDLookup.Add(Object, NetGUID);
}
return NetGUID;
}
}
}` This did prevent the check from being hit during the seamless travel. However, please note that this workaround has not been extensively tested, and there are also performance concerns around iterating through the ObjectLookup map, which could be mitigated by including another TMap keyed by the path name in order to make the lookup faster.
Regardless, I’ve opened a new issue for this for us to look into this further, UE-278414, which should be visible in the public tracker in a day or so.
Thanks,
Alex