VScript leak fixes

[samisalreadytaken]

Fixes leaks on native function HSCRIPT parameters; adds IScriptVM::CopyObject to copy script objecs to be stored in C++. Every HSCRIPT parameter was duplicated, they would never get released until the VM was shutdown.

Fixes CScriptKeyValues ownership issues and leaks. Every instance of it was leaking, and it was possible to delete KeyValues not owned by script.

Adds correct way to return reference counted objects to script. bAllowDestruct never worked because there was always a reference that was never released.

Converted to ref counted instances: CScriptKeyValues(FileToKeyValues), TraceLineComplex, TraceHullComplex, CreateDamageInfo, CreateFireBulletsInfo

Deprecated: CScriptKeyValues::ReleaseKeyValues, CGameTrace::Destroy, DestroyDamageInfo, DestroyFireBulletsInfo

---

Does this PR close any issues?

• Fixes [VSCRIPT] Memory leak on native function calls #123

PR Checklist

• My PR follows all guidelines in the CONTRIBUTING.md file
• My PR targets a develop branch OR targets another branch with a specific goal in mind

[z33ky]

I think I understand the purpose of HSCRIPT_RC for objects that are created by C++, passed to VScript and there may create objects that keep a reference to that "base instance", which will have it's reference-count increased. This allows safe usage of KeyValues and sub-keys thereof. The premise makes sense and I checked the related changes, albeit I didn't make up a complete metal model of how it is used. It looked pretty sensible insofar.
It is also used for CScriptGameTrace, CTakeDamageInfo and FireBulletsInfo_t, which (despite the name implicating this) are not reference-counted, but similarly created by C++ and passed to VScript, from where the lifetime must be managed. This relied on script-authors manually calling provided "Destroy"-functions, which are retained but now no-ops to maintain compatibility. Destruct-functions are now registered using DEFINE_SCRIPT_REFCOUNTED_INSTANCE() - except for FireBulletsInfo_t, which already got its' one through DEFINE_SCRIPT_CONSTRUCTOR().

I don't quite know about the necessity of CopyObject() for IScriptVGUIObject::SetScriptInstance()/SetHScript(). I guess it shall ensure the HSCRIPT stays valid while it is stored there, regardless of what happens with the original reference. Whatever the concrete situation may be that requires this change, the change seems sensible to me as well.

[z33ky]

I'd probably name this ScriptAddDestructorToClassDesc() (mirroring ScriptAddConstructorToClassDesc()). I don't think REFCOUNTED_INSTANCE quite represents the meaning of this, even if you use this for a ref-counted data-type right now.
Having no construction, only destruction, is not really specific to ref-counted objects. Anything that vscript cannot create itself, but manages some state that needs to be cleaned up via a destruction-function would use this.
It would also be perfectly reasonable to have a ref-counted object that can be created from vscript.

[samisalreadytaken]

DEFINE_SCRIPT_REFCOUNTED_INSTANCE is mirroring DEFINE_SCRIPT_CONSTRUCTOR. The name REFCOUNTED_INSTANCE is about intent instead of internals. Someone looking to register a C++ class doesn't need to know how it works. This could also be added to classes with constructors and the duplicated assignment would likely be optimised out.

Having no construction, only destruction, is not really specific to ref-counted objects. Anything that vscript cannot create itself, but manages some state that needs to be cleaned up via a destruction-function would use this.

What are the examples of this?

It would also be perfectly reasonable to have a ref-counted object that can be created from vscript.

This is what constructors do, that is the case in FireBulletsInfo_t. It can be both constructed in script and returned from CreateFireBulletsInfo.

[z33ky]

The name REFCOUNTED_INSTANCE is about intent instead of internals. Someone looking to register a C++ class doesn't need to know how it works.

I may have missed something here. I was under the impression that this refers to reference-counting in C++, like the CScriptKeyValues do, though it may be intertwined with the ref-counting done through the SquirrelVM? Doesn't every script-instance do reference-counting though, (the reference-counting portion of) which is completely detached to destructors/Destruct-functions?

What are the examples of this?

Well, this is a new implementation. I don't really have any new things in mind for which I'd like to use this.
There are existing classes that you're converting though that don't have intrinsic ref-counting, like the FireBulletsInfo_t you mention. Or does that happen somewhere behind the scenes, or did you intend to refer to the SquirrelVM reference-counting as I wonder about above?

[samisalreadytaken]

All non-primitive (not int, bool, float) script objects always do reference counting in Squirrel. DEFINE_SCRIPT_REFCOUNTED_INSTANCE and HSCRIPT_RC are for taking advantage of that. CScriptKeyValues is a kind of its own because it can create children that have memory that depend on the root, so it manually does reference counting of KeyValues in C++ independent of Squirrel, which only reference count for CScriptKeyValues.

Destructors need to be defined for script refcounted objects so that C++ can allocate memory, pass it to RegisterInstance then forget it all. Otherwise both the script object (HSCRIPT) and the memory it points to (T *pInstance) needs to be freed in C++ (see CBaseEntity).

I tried to explain this and how script objects work in the long reply below.

[z33ky]

bRefCounted seems to have a fixed value at the call-sites, which makes sense I think. If you return a HSCRIPT you can probably determine statically if it's meant to be handled as "ref-counted".
Consider if it is feasible to split the function into two - HSCRIPT RegisterInstance() and HSCRIPT_RC RegisterInstanceRefcounted(). Casts from and to HSCRIPT for HSCRIPT_RC should then also be made explicit.
Same with IScriptManager::CreateScriptKeyValues().

[samisalreadytaken]

Adding RegisterInstanceRefcounted would mean also duplicating the SquirrelVM::RegiserInstance function. A template or a wrapper could work for simplification.

I like the non-explicit nature of HSCRIPT_RC. It's still a valid HSCRIPT object; it can be called, cached, deleted. It doesn't require any change other than return type. I also thought of specifying the refcounted nature in function registration, but that was too noisy.

[z33ky]

As it stands, the programmer must take care to convert the return-value to HSCRIPT_RC - as the comment mentions. This could be enforced by the type-system instead.
Anyway, this is just a drive-by review. I didn't spot a problematic case in the changes FWIW. Feel free to mark as resolved.

[samisalreadytaken]

I think I understand what you mean. I added HSCRIPT_RC RegisterInstanceRefCounted(), removed bRefCounted parameter and HSCRIPT_RC::operator HSCRIPT(), added ScriptVariant_t::operator=( HSCRIPT_RC ). And it works to enforce return type, but it breaks passing HSCRIPT_RC to HSCRIPT parameters to call, copy or delete. I'll just leave it as is without a clean solution for now.

[z33ky]

I imagine the reverse also be true (if it's borrowed it's a child) - Assert( !m_pSelf->borrow || m_pBase ).

[samisalreadytaken]

Not the case for registering C++ managed KeyValues, such as in CLogicExternalData::ScriptGetKeyValues(). Though in this particular case the entity can die while CScriptKeyValues lives. Registering a copy would make editing the result of GetKeyValues() not change m_pRoot, but require SetKeyValues() to update. A cumbersome workaround would be CLogicExternalData keeping copies of script instances it registers. I think @Blixibon could give his input on this since it was his design choice.

Logically it makes sense, and it works given the borrowed memory isn't independently freed before the script VM.

[Blixibon]

I don't remember my exact rationale for having CScriptKeyValues directly wrap the KeyValues data in logic_externaldata, although that I may have thought it would be more efficient or intuitive to directly access it. In retrospect, I think that was a poor choice. It may not have occurred to me that the associated data can be removed while the CScriptKeyValues instance remains.

I like the idea of keeping copies of the script instances to perform proper cleanup while maintaining the current behavior, even if it is cumbersome, although that's not particularly relevant to the rest of this PR and can be resolved another time.

[samisalreadytaken]

HSCRIPT is just SQObject, which is only type and pointer. Its size depends on compile target word size and _SQ64/SQUSEDOUBLE. If vscript didn't need to have arbitrary language compatibility, HSCRIPT could be defined as a struct the size of SQObject (4+4 bytes); then HSCRIPT everywhere could simply be copied instead of ever needing new HSQOBJECT.

When instances are registered in SquirrelVM::RegisterInstance, they are initially created in the VM stack, but then the stack is cleared because IScriptVM API doesn't specify that this should be the last script VM call to leave the extra object in the VM stack (because C++ functions (SQFUNCTION) push their return values to VM stack). So the internal Squirrel ref count is incremented with sq_addref to prevent the release of the object with sq_pop. And this object (type+pointer) is copied into new HSQOBJECT due to the reason stated above. At this point, the Squirrel object has only 1 reference in the VM.

When the result of RegisterInstance is returned from a C++ function, it reaches if (pFunc->m_desc.m_ReturnType != FIELD_VOID) check inside function_stub(). When this HSCRIPT (SQObject) is pushed to the VM stack through PushVariant->sq_pushobject, the VM refcount of the object is incremented to 2, with the VM stack holding the other one. If the C++ side doesn't care about the script object anymore, it can let go of the reference added with sq_addref and the temporary SQObject with sq_release and delete retval.hscript under if ( pFunc->m_flags & SF_REFCOUNTED_RET ). Then the only VM reference remains in the VM stack, where the release hook destructor_stub will be called when it is unreferenced in script. This release hook will do delete (T*)p which calls the destructor and frees the memory allocated in C++, which was passed to RegisterInstance as void *pInstance.

For KeyValues, the script object representation of KeyValues can create children in script whose memory depend on the root KeyValues object. Reference counting externally allows unreferencing parent KeyValues while keeping children alive and functioning. Testing this is easier using the squirrel debugger stack view with $stack watch variable. As an example, this CScriptKeyValues script object local kv = FileToKeyValues("test.txt").GetFirstSubKey() will continue functioning after all references to its parents are released, and the root parent memory will be freed when the only child is unreferenced in script.

test.txt: "test" { "key1" { "key2" 1 } }

SquirrelVM::CopyObject is for scripts passing refcounted objects (SQObject) to C++ functions. Since SQObject doesn't fit in HSCRIPT, they are allocated with new HQSOBJECT under for (int i = 0; i < nargs; ++i) inside function_stub. They don't need reference increment here because they already exist in VM stack during the native call frame. After the C++ function call, this temporary SQObject is freed under for ( int i = 0; i <= nLastHScriptIdx; ++i ). Now, inside the C++ function call, the script object may need to be cached and referenced later on, so they need to copy SQObject (type+pointer) and increment the script VM ref count. This is what SquirrelVM::CopyObject does.

This is used in script function callbacks cached in C++ everywhere: scriptthinkfunc_t::m_hfnFunc (baseentity_shared.cpp), CScriptGameEventListener::m_hCallback (vscript_singletons.cpp), CScriptConCommand::{m_hCallback, m_hCompletionCallback} (vscript_singletons.cpp), CScript_Panel::m_hfnPaint etc (vscript_vgui.cpp).

IScriptVGUIObject::SetScriptInstance actually doesn't need to copy since BindOnRead is already passed a copy in SquirrelVM saverestore, I'll fix that.

[Blixibon]

I don't remember my exact rationale for having CScriptKeyValues directly wrap the KeyValues data in logic_externaldata, although that I may have thought it would be more efficient or intuitive to directly access it. In retrospect, I think that was a poor choice. It may not have occurred to me that the associated data can be removed while the CScriptKeyValues instance remains.

I like the idea of keeping copies of the script instances to perform proper cleanup while maintaining the current behavior, even if it is cumbersome, although that's not particularly relevant to the rest of this PR and can be resolved another time.

[samisalreadytaken]

I was just committing the conflict resolution. The SF_REFCOUNTED_RET checks should be under m_ReturnType != FIELD_VOID

    SQInteger sq_retval;
    if (!sq_isnull(pSquirrelVM->lastError_))
    {
        sq_pushobject(vm, pSquirrelVM->lastError_);
        sq_resetobject(&pSquirrelVM->lastError_);
        sq_retval = sq_throwobject(vm);
    }
    else
    {
        Assert(script_retval.m_type == pFunc->m_desc.m_ReturnType);
        Assert( ( pFunc->m_desc.m_ReturnType != FIELD_VOID ) || !( pFunc->m_flags & SF_REFCOUNTED_RET ) );

        if (pFunc->m_desc.m_ReturnType != FIELD_VOID)
        {
            PushVariant(vm, script_retval);

            if ( ( pFunc->m_flags & SF_REFCOUNTED_RET ) && script_retval.m_hScript )
            {
                Assert( script_retval.m_type == FIELD_HSCRIPT );

                // Release the intermediary ref held from RegisterInstance
                sq_release(vm, (HSQOBJECT*)script_retval.m_hScript);
                delete (HSQOBJECT*)script_retval.m_hScript;
            }

            sq_retval = 1;
        }
        else
        {
            sq_retval = 0;
        }
    }

    // strings never get copied here, Vector and QAngle are stored in script_retval_storage
    // everything else is stored inline, so there should be no memory to free
    Assert(!(script_retval.m_flags & SV_FREE));

    for ( int i = 0; i <= nLastHScriptIdx; ++i )
    {
        if ( pFunc->m_desc.m_Parameters[i] == FIELD_HSCRIPT )
            delete (HSQOBJECT*)params[i].m_hScript;
    }

    return sq_retval;
}

[Blixibon]

Sorry! I should've waited longer after merging z33ky's PR. I wasn't sure if you were going to be around to resolve it on your own.

[samisalreadytaken]

FYI, copy-pasting seems to have converted tabs into spaces in my comment

8576fb1#diff-67fd1ca8e9ca5070bbcb6fda805005b7e72c9f5343129ae55103852af548d992R1468