Dangling pointer + mem corruption related to OwnerLastRenderTimePtr

Development Rendering
, ,
1

We’ve been seeing asserts and crashes related to OwnerLastRenderTimePtr. We’ve traced this to the mentioned pointer ending up dangling after using UActorComponent::Rename() on primitive components to reuse/reparent them, for example:

WeaponMeshComponent->Rename(nullptr, WeaponActor);where:

  • WeaponMeshComponent is derived from PrimitiveComponent
  • WeaponActor is a newly created actor
  • The previous owner of WeaponMeshComponent is being destroyed

The problem is that WeaponMeshComponent’s SceneData.OwnerLastRenderTimePtr pointer still points to the old parent object being destroyed. The code in UPrimitiveComponent::ReleaseSceneProxy() that accesses this pointer can crash directly or corrupt mem elsewhere via the atomic operation on SceneData.OwnerLastRenderTimePtr->NumAlwaysVisibleComponents. We trapped this with stomp malloc.

Potential fix that has eliminated lots of the random crashes in our testing so far is to add this PostRename method.

`void UPrimitiveComponent::PostRename(UObject* OldOuter, const FName OldName)
{
Super::PostRename(OldOuter, OldName);

FActorLastRenderTime* OldLastRenderTimePtr = SceneData.OwnerLastRenderTimePtr;
FActorLastRenderTime* NewLastRenderTimePtr = FActorLastRenderTime::GetPtr(GetOwner());

if (OldLastRenderTimePtr &&
OldLastRenderTimePtr != NewLastRenderTimePtr)
{
SceneData.OwnerLastRenderTimePtr = NewLastRenderTimePtr;

// Update the Number of visible components on the new owner if we are always visible,
// otherwise we’ll assert later due to this count being off when releasing the sceneproxy
if (SceneData.bAlwaysVisible && SceneData.OwnerLastRenderTimePtr)
{
SceneData.OwnerLastRenderTimePtr->NumAlwaysVisibleComponents.fetch_add(1, std::memory_order_relaxed);
}
}
}`

2

Steps to Reproduce

see description above

3

Question i’m sitting here asking myself…this works much better than before, but is it safe to change OwnerLastRenderTimePtr at this point from the game thread? Presumably the render thread could access it via SetLastRenderTime() at any time.

4

A good find! I’ll look into fixing this in UE very soon, we’ll most likely opt for a full renderstate update to be sure everything is tied together correctly (other things could depend on the actor).

We usually do not change this ptr while in use by the render thread and actor/component destruction uses a fence in the renderthread commands and a conditionaldestroy to make sure it’s ok to delete the object. I’ll see how I reconcile this with the fact renames cannot be delayed. We will probably need a mechanism to force the old actor to use the fence even if it has no primitive components anymore just to be 100% sure a preempeted renderthread doesn’t have this ptr value loaded in its thread state. The write of the new ptr should be atomic on most platform as is, so will be seen instantly and should work. In any case my fix will ensure all of this is ok.

So there’s still a tiny possibility of stomp with your fix even if it’s exceedingly rare. We’ll let you know when we have a CL, but in the meantime the fix is better than the original code.

5

Thanks Dominic!