Fix memory leaks.

[Hoikas]

This changeset fixes a major source of memory leaks in the vault client API. Any time hsRef<NetVaultNode> foo = new NetVaultNode; was called resulted in a leak due to a change in behavior when the old AtomicRef was merged with hsRefCnt.

[Hoikas]

For some background reading...

The onus of this is that on Gehn Beta, downloading a new copy of @DoobesURU Age Veelay Tsahvahn can cause the client to crash shortly after linking in with an std::bad_alloc. I started linking around and noticing that the client's memory usage seems to grow for each link. For example, standing in Relto for the first time, the client uses ~600mb. Linking to Gahreesen and back to Relto causes the client memory usage to be around ~900mb. That's an extreme example--smaller Ages like the Nexus have less of an impact. While I am fixing leaks reported by VLD, I'm really not seeing anything that corresponds to the observed behavior.

[Hoikas]

I recently received a complaint in IRC that boiled down to folks with large buddy lists saw std::bad_alloc rather frequently, so I decided to revisit this changeset due to its fixing of leaked vault nodes. It has been updated to merge cleanly. However, due to an issue introduced in the upgrade to Python 3, we depend on #720.

[DamnBriggsy]

Prior to these changes, the client would crash on MOULa in an hour or less. After these changes, it stayed up for over 10 hours with no problems.

[zrax]

What cases do we want to support where the ref count is < 1? I feel like we should catch too-many-unrefs as well as too few...

[Hoikas]

NetVaultNode's starting reference count is zero. An earlier refactor changed its base class from AtomicRef to hsRefCnt but neglected to address the difference in starting ref count, introducing all of these leaks. I initially wanted to add a template parameter for the starting ref count to hsRefCnt, but I wasn't able to come up with a clean solution that didn't expose implementation details to the rest of the engine

[Hoikas]

@zrax RE our discussion in IRC about this PR... Maybe the easiest path forward is to back out 80e00cd? That restores the starting ref count to 1, but uses a weird hack for initializing an hsRef from a newly allocated object. I'm not sure if we're going to find anything much cleaner. :/

[zrax]

Part of the problem here is the mix of managed (hsRef<T>) and unmanaged objects all over the code base... If we could assume all were managed, this could be simplified and made safer. Unfortunately, the code base is far from that goal, so we have these ugly hacks instead.

Perhaps instead of the New() template, we could add a more generic mechanism for adopting/stealing a ref. Something like (completely untested):

struct hsAdoptRef_Type {};
constexpr hsAdoptRef_Type hsAdoptRef;

template <class _Ref>
class hsRef
{
public:
    /* ... */

    hsRef(_Ref *obj, hsAdoptRef_Type) : fObj(obj) { }

    /* ... */

    void Adopt(_Ref *obj)
    {
        if (fObj)
            fObj->UnRef();
        fObj = obj;
        return *this;
    }

    /* ... */
};

Then in usage:

hsRef<Whatever> myref(new Whatever, hsAdoptRef);
// or
hsRef<Whatever> myref;
myref.Adopt(new Whatever);

In the long run, we'd want to eventually ensure that promotion from a "raw" hsRefCnt to an hsRef managed object is always one-way, which would mean that the adoption behavior becomes the default (and this workaround can be removed). We may actually be close already, but that would require more investigation and testing than I think you're willing to add in this PR ;)

[Hoikas]

Agreed. I'll look into this, although I may change hsAdoptRef to hsStealRef - I think the naming is clearer in that case 😛

[Hoikas]

It compiles!

[Hoikas]

hsRef<_Ref>::New() is really gone now 🤣

[Hoikas]

💥