Add-Ref Proxy class causes crash in asIScriptObject::operator= due to early destruction

Started by
8 comments, last by WitchLord 4 years, 6 months ago

Hi i ran into a little problem related to when i want to inherit from a registered class using the mechanism explained here:

https://www.angelcode.com/angelscript/sdk/docs/manual/doc_adv_inheritappclass.html


// Angelscript side

shared abstract class FSMState
{
  private FSMStateProxy@ instance;

  FSMState() { @instance = FSMStateProxy(); }

  void Enter(void) {}
  void Exit(void) {}

  void AddAction(const Variant& in action) { instance.AddAction(action); }
  void RemoveAction(const StringHash &in actionID) { instance.RemoveAction(actionID); }
}

class PlayerMoveState : FSMState
{
  PlayerMoveState()
  {
    Printf("Hi");
  }
}

// function logic thats uses classes

{
    PlayerMoveState moveState;

    controller.AddState(moveState);
}

///////////////////////////////////////////////
C++ side .cpp
///////////////////////////////////////////////
  ScriptProxyRefCountedObject::ScriptProxyRefCountedObject(asIScriptObject* object)
    : GTL::RefCountedObject()
    , m_objectInstance(object)
    , m_bIsDead(object->GetWeakRefFlag())
  {
    // increment weak ref as we want to hold a reference to the object
    m_bIsDead->AddRef();

    // note: internal counter starts of with (1) for script objects
    RefCountedObject::AddRef();
    CHECK(m_objectInstance && m_bIsDead);
  }

  ScriptProxyRefCountedObject::~ScriptProxyRefCountedObject()
  {
    m_bIsDead->Release();
  }

  // add/release for managing object
  unsigned ScriptProxyRefCountedObject::AddRef(void) const
  {
    // increment angelscript side counter as well to made sure it doest not get destroyed
    // before the c++ side
    if (!m_bIsDead->Get())
      m_objectInstance->AddRef();

    return GTL::RefCountedObject::AddRef();
  }

  unsigned ScriptProxyRefCountedObject::Release(void) const
  {
    // release angelscript instance as well
    if (!m_bIsDead->Get())
      m_objectInstance->Release();

    return GTL::RefCountedObject::Release();
  }
}

 

The problem stems when "controller.AddState()" gets called because. Since Add state executes a copy of the object. It has to create a second instance and invoke the ScriptObject::operator= for each property within the script object.

 

So we have CopyA: ScriptObject::RefCount = 1 Proxy::WeakRef=2 Proxy::m_objectInstance = 1 where (Proxy::m_objectInstance == ScriptObject::RefCount)

                    CopyB: ScriptObject::RefCount = 1 Proxy::WeakRef=2 Proxy::m_objectInstance = 1 where (Proxy::m_objectInstance == ScriptObject::RefCount)

 

When the script object invokes the operator= it will invoke the CopyHandle function and perform a Release on the destination handle and then an add-ref on the source handle. Then assigns the new handle. The problem is that for any proxy thats hits this operator= with both instance at a ref-count = 1. Then the release call in the CopyHandle will invoke CopyA::ScriptProxyRefCountedObject::Release, invoking m_objectInstance->Release() and thus Destroying the whole scriptObject. Destroying the script object will force the same CopyA::m_objectInstance->Release() function be called in the destructor. When the logic finishes invoking and goes back up to the operator= call stack, then we crash as 'this' pointer aka the object that was destroyed got nuked.

 

Hope that explains my problem. Hoping @WitchLord can help or elaborate on something im missing with regards to the example shown in the docs. This crash should happen given the example code 

below which is given in the angelscript documentation and i just added an additional function.

 


  
  void TestFunction(FooDerived &in CopyA)
  {
  }

  void main()
  {
    // When newly created the m_value is 0
    FooDerived d;
    assert( d.m_value == 0 );

	// invoking this function should force a copy to be created and the asIScriptObject::operator= to be called
    // where internal ref-count of the object stored in the proxy are both = 1. Crash occurs
    TestFunction(d);

    // When calling the method the m_value is incremented with 1
    d.CallMe(); 
    assert( d.m_value == 1 );
  }

 

please let me know if im missing something in regards on how to property handle proxy inheritance.

Advertisement

Hi hiago,

I think you're spot on with your analysis. The problem lies with asCScriptObject::CopyHandle function first calling the Release and then AddRef. I'll have this fixed so that the Release is called on the old object only after the AddRef has been called on the new object. 

Regards,
Andreas

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

Hi @WitchLord. I dont think the change your suggesting would fix the issue. The main problem is the fact that in your "inherit from a registered class example" in the AddRef  you addref the objectInstance and vice versa in the Release. So that means whenever a copy handle gets copied using the asIScriptObject::CopyHandle(), your going to need to release the 'this' instance as some point. Before or after the old one, but the old one is the 'this' one. Since you are releasing the property proxy, it will release the script object instance and force a nuke of the 'this' script object. When it returns from the callstack it will crash no matter what order you release.  Maybe you might be seeing something im not seeing? I tried implementing the suggested change locally but no luck.

I'll need to do some testing with the code you provided to get a better insight into the problem.

 

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

In addition to this issue, i have found another problematic issue with the "inherit from registered class type" example.

The issue stems from the fact that in the proxy class you are holding a weak reference to the scriptObject that gets passed in during the factory construction, in addition of storing a pointer to that scriptobject class.

Proxy(asIScriptObject* instance). The "instance" is the script object that gets passed in when the object is constructed. When angelscript goes to asIScriptObject::operator= (Presumed that the current crash we talked about is not happening) it will try to copy each property. In the case of the Proxy@ property we just assign the handles by AddRef one and Release the other. Now the new object holds a new handle (Proxy@), BUT BUT that proxy handle holds a Weak Ptr and asIScriptObject* to the old object still. The one that it was created with during construction. So the next time you AddRef/Release it will perform the action to the wrong desired object that should be out of scope.

 

I think you can fix issue by not having the proxy class hold an object handle. But instead just have it instantiate the proxy and then have the c++ implementent an operator=. In the operator= implementation you would need to assign the new 'rhs' asIScriptObject* so that it matches with the proxy.

 

In addition, I have bypassed the crash in operator= by adding two new functions to the proxy class. 

    bool AddRefScriptObject(void) const;
    bool ReleasecriptObject(void) const;

These functions manually AddRef/Release the underlying script object if the c++ side wants to hold a strong reference to them.

This means that the proxy class would not by default attempt to add/ref the underlying script object. Which removed unwanted behavior of the script object owned by angelscript getting nuked where it shouldnt.

 

Thanks, Hope this gives you more information ? 

It all boils down to the internal object not being duplicated when making a copy of the derived script object.

Either the derived class should prohibit the copy, or it needs to do a deep copy of the internal object too.

 

 

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

I've fixed this in revision 2626.

The C++ class and script proxy class must have the operator=/opAssign method implemented to do a deep copy of the object. The script library has also been fixed so the default implementation of opAssign for script classes will use the base class' opAssign method to copy inherited properties.

 

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

Thanks for the fix @WitchLord. Btw now for this to work you cant have the baseclass in angelscript side be ABSTRACT. 

You cant overwrite opAssign for abstract class because the class cant be instantiated and thus angelscript cant copy.

 

For example:

FSMState@ opAssign(const FSMState &in other)

 

if FSMState is an abstract angelscript class this opAssign wont work, should mention that in the docs.

 

Thanks!

 

Remove 'in' and it works.

Regards,

Andreas

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

This topic is closed to new replies.

Advertisement