OpenSim::Set::insert() can cause issues with MATLAB garbage collector

[carmichaelong]

This was seen when @gurchiek was using MATLAB to insert a new point into a PathPointSet created with clone().

clone() creates a new point that did not have a model or owner (hasModel() and hasOwner() were false). Then, using PathPointSet::insert() and finalizeConnections() made the Set take ownership, which can cause problems with MATLAB's garbage collector. Using markAdopted() fixed the issue in an isolated example.

We should consider adding this to one of our .i files (perhaps java_simulation.i?). Some things to note:

1. If the object being inserted is non-const, we still might need to check if the Set has getMemoryOwner() as true or false. https://simtk.org/api_docs/opensim/api_docs/classOpenSim_1_1Set.html#adf1da111571bb1f156da8ab169181a48
2. If the object being inserted is const, we wouldn't need to check as it always makes a copy, but I'm not sure how/if MATLAB differentiates between these two methods anyway. https://simtk.org/api_docs/opensim/api_docs/classOpenSim_1_1Set.html#af79a11b57529978ce78c9dc28771bfe2

[aymanhab]

As we discussed in person, there're two parts to the solution:

1. Short term surgical fix is to make sure insert method communicates that the array/set takes ownership of the passed in object so it doesn't get double deleted causing the crash.
2. As @carmichaelong pointed out this is complicated by the fact that Arrays may or may not have setMemOwner set, in fact the way it's made public this flag can change AFTER the set is created. This is a relic of the old c++ pre-smart pointers. I'd advocate for removing this method (setMemoryOwner) from the public interface completely. Clients who need shared pointers to objects should create containers of shared pointers which are well managed by modern flavors of C++ for more than a decade. I'll open a separate issue for that.

[aymanhab]

Can we mark this fixed now, @carmichaelong ?