crashes when a Ragdoll points to a destroyed PhysicsSystem

[stephengold]

As I mentioned before, I'm creating Java bindings for JoltPhysics. Earlier this month I began experiencing various crashes:

• mutex lock failed
• assertion failures in Ragdoll.cpp line 543 and Array.h line 455
• EXCEPTION_ACCESS_VIOLATION
• SIGSEGV in 4 different locations

After much experimentation, I've narrowed down the issue. I believe it relates to the order in which destructors for RagDoll and PhysicsSystem are invoked.

As you're probably aware, Java programmers aren't accustomed to thinking about object destruction. When using my Java bindings, C++ objects like physics systems and ragdoll references are allocated on the heap, and their destructors typically get invoked by an asynchronous automatic cleanup thread. In this scenario, the Java application cannot control the order in which those destructors are invoked. It's entirely possible for PhysicsSystem::~PhysicsSystem to be invoked before Ragdoll::~Ragdoll. In that case, ~Ragdoll accesses a destroyed PhysicsSystem.

This scenario is unlikely in C++ applications, where typical coding practices ensure destruction of all ragdolls prior to destruction of the physics system(s) they point to.

I ran an experiment where I disabled my cleanup thread so I could specify the destruction sequence. When the sequence was:

	for (RagdollRef r : mRagdolls)
		r.close();
        mPhysicsSystem.close();

the application always completed successfully, but when the sequence was:

        mPhysicsSystem.close();
	for (RagdollRef r : mRagdolls)
		r.close();

the application crashed every time at Jolt/Core/Array.h:455: (inIdx < mSize).

It ought to be possible to modify the PhysicsSystem class to keep track of all ragdolls that point to the system so that ~PhysicsSystem can invalidate those pointers. Or perhaps you can devise a better solution.

[jrouwe]

Unfortunately this is not something that can be fixed. The PhysicsSystem does not know about the existance of the Ragdoll so it cannot destroy it first. Ragdoll is a higher level concept that the physics system should not know about. In general Jolt tries to avoid links between objects as much as possible. This is one of the main reasons why you can do many things in a thread safe way.

[stephengold]

I understand.
I have another idea: how about changing Ragdoll::RemoveFromPhysicsSystem() to null out the mSystem field?

[jrouwe]

That's not a proper solution either. You can do:

• AddToPhysicsSystem
• RemoveFromPhysicsSystem
• AddToPhysicsSystem
• ...

This is quite common as a character may only need an active ragdoll during certain situations (e.g. hit responses), but you want to keep the ragdoll around because it's expensive to create.

I don't think this is going to be the only case where destruction order matters and I would suggest looking into a more generic system to handle this. I took a quick look at jolt-jni:

• Could JoltPhysicsObject keep track of dependencies and prevent premature destruction? E.g. an object reference to the class that created it? This would contain the PhysicsSystem for Ragdolls.
• Can the Cleaner be smarter about ordering? E.g. can you order by type or perhaps by some priority value before running the actual clean so that PhysicsSystem is always destroyed last?

My Java skills are pretty low so excuse me if my suggestions don't make sense.

[stephengold]

you want to keep the ragdoll around because it's expensive to create.

That makes sense.

Could JoltPhysicsObject keep track of dependencies and prevent premature destruction?

I already track dependencies for C++ objects contained in other C++ objects, such as vectors. However, tracking doesn't help with this issue because it only prevents containers from being cleaned while their contents are strongly reachable. In this issue, neither the ragdoll nor the physics space is strongly reachable.

can you order by type or perhaps by some priority value before running the actual clean so that PhysicsSystem is always destroyed last?

I don't know of any way to do this. I'll ask around.

[stephengold]

Question: are there other situations in Jolt Physics where the destructor of a high-level object refers to a low-level object with its own destructor?

[jrouwe]

Depends a bit on your definition of a high level and a low level object.

PhysicsSystem destroys its Bodies and can destroy Constraints (refcounted)
Character destroys its Body (so it better be destroyed before the PhysicsSystem)
CharacterVirtual destroys its (optional) Body (so it better be destroyed before the PhysicsSystem)
Ragdoll destroys its Bodies and can destroy its GroupFilter (refcounted) (so it better be destroyed before the PhysicsSystem)
Body can destroy its Shape (refcounted) and GroupFilter (refcounted)
Shape can destroy its PhysicsMaterial (refcounted)
VehicleConstraint destroys its controller and wheels
SkeletalAnimation can destroy its Skeleton (refcounted)
SkeletonPose can destroy its Skeleton (refcounted)
CompoundShapes can destroy their sub shapes (refcounted)
SoftBodyMotionProperties can destroy its SoftBodySharedSettings (refcounted)

And there are probably more that I forgot.

[stephengold]

Thanks for the brainstorm.
Refcounted objects shouldn't cause ordering issues like this one, which involves an explicit pointer (mSystem).
I'll investigate the other examples you listed.

[stephengold]

PhysicsSystem destroys its Bodies and can destroy Constraints (refcounted)

The ~PhysicsSystem destructor refers only to mBroadPhase, which it allocated in Init(), so I see no risk of use-after-destruction there.

Character destroys its Body (so it better be destroyed before the PhysicsSystem)
CharacterVirtual destroys its (optional) Body (so it better be destroyed before the PhysicsSystem)

Yes, I need to control the destruction order for Character and CharacterVirtual relative to PhysicsSystem, just as I do for Ragdoll.

Body can destroy its Shape (refcounted) and GroupFilter (refcounted)

The ~Body destructor refers to mMotionProperties. However, it doesn't dereference the pointer, so I see no risk of use-after destruction there.

(to be continued)

[jrouwe]

The ~PhysicsSystem destructor refers only to mBroadPhase, which it allocated in Init(), so I see no risk of use-after-destruction there.

I think the main risk here is that if you destroy a body after the physics system is destroyed (like in the case of a Ragdoll or when a CharacterVirtual has an inner rigid body) that you are working with a dangling pointer (both those classes point keep a pointer to the PhysicsSystem).

[stephengold]

I understand. I recently changed how the Java objects for Character, CharacterVirtual, and Ragdoll are tracked, to ensure they get cleaned (by the cleaning thread) before the corresponding physics systems get cleaned. At this point, I'm trying to identify other situations where cleaning order matters, to sure they're correctly handled in the next release of jolt-jni.

[stephengold]

Shape can destroy its PhysicsMaterial (refcounted)

The Shape class itself doesn't have a user-provided destructor, only a default one. The subclasses I checked track their materials as counted references, so I see no risk of use-after-destruction there.

VehicleConstraint destroys its controller and wheels

Yes, I need to control the destruction order for VehicleConstraint relative to VehicleController and Wheel. This one looks tricky. I notice that VehicleController is a refcounted class. Why does VehicleConstraint store an ordinary pointer (mController) instead of a counted reference?

[jrouwe]

Yes, I need to control the destruction order for VehicleConstraint relative to VehicleController and Wheel. This one looks tricky. I notice that VehicleController is a refcounted class. Why does VehicleConstraint store an ordinary pointer (mController) instead of a counted reference?

Actually it being a ref count target is wrong and I've created #1441 to fix that. Both the wheels and controller are created in the constructor of the VehicleConstraint and they're destroyed in the destructor. They can't be shared with other constraints, so I think you can simply not destruct objects of these types as you're never going to be the owner (and if you don't expose their constructors in Java then users can't create them accidentally either).

[stephengold]

That makes sense.

Thank you for the PR. I'll resume work on this after it's integrated into the JoltPhysics "master" branch.

[jrouwe]

It's integrated.

[stephengold]

I've handled VehicleConstraint as you suggested. I verified that Java can't construct controllers and wheels outside a VehicleConstraint.

SkeletalAnimation can destroy its Skeleton (refcounted)

I don't see where that occurs in the code.

SkeletonPose can destroy its Skeleton (refcounted)

SkeletonPose doesn't have a user-provided destructor. Since its Skeleton is accessed via a counted reference, I see no risk of use-after-destruction there.

CompoundShapes can destroy their sub shapes (refcounted)

CompoundShape doesn't have a user-provided destructor. Since its subshapes are accessed via counted references, I see no risk of use-after-destruction there. I also studied both at its subclasses and didn't spot any issues.

SoftBodyMotionProperties can destroy its SoftBodySharedSettings (refcounted)

SoftBodyMotionProperties doesn't have a user-provided destructor. Since its shared settings are accessed via counted references, I see no risk of use-after-destruction there.

And there are probably more that I forgot.

I greatly appreciate your help. I'll keep my eyes open for similar situations.

For the record, the technique I'm using to control destruction order (for Character, CharacterVirtual, and Ragdoll relative to PhysicsSystem) is simple: whenever I register a cleaning action for a character or ragdoll, I pass the Java object that owns the PhysicsSystem to that action.

[jrouwe]

SkeletalAnimation can destroy its Skeleton (refcounted)

I don't see where that occurs in the code.

Sorry, my mistake. I thought it kept a reference to its skeleton, but I must have looked at the wrong thing.

For the record, the technique I'm using to control destruction order (for Character, CharacterVirtual, and Ragdoll relative to PhysicsSystem) is simple: whenever I register a cleaning action for a character or ragdoll, I pass the Java object that owns the PhysicsSystem to that action.

I'm not sure if I follow. So if I delete a Ragdoll then you pass the Java object for the PhysicsSystem? How does the Ragdoll get destructed then?

[stephengold]

I must have looked at the wrong thing.

No worries. I merely wanted to make sure I wasn't missing something. Thanks for double checking.

How does the Ragdoll get destructed then?

Along with the Java object, I pass the virtual address of the Ragdoll. The address is cast to a C++ pointer and used to delete the Ragdoll. The Java object (that owns the PhysicsSystem) isn't actually used; its sole purpose is to keep the Java object reachable until the ragdoll's cleaning action has run.

Last night I released jolt-jni v0.9.4 . Thanks all for your help, Jorrit!