Double free or corruption error

[JMLX42]

Hello,

my project defines the following class for signals:

namespace minko
{
    template <typename... A>
    class Signal :
        public std::enable_shared_from_this<Signal<A...>>
    {
    private:
        typedef std::function<void(A...)>                   Callback;
        typedef typename std::list<Callback>::iterator      CallbackIterator;
        typedef typename std::list<unsigned int>::iterator  SlotIterator;

        template <typename... B>
        class SignalSlot;

    public:
        typedef std::shared_ptr<Signal<A...>>           Ptr;
        typedef std::shared_ptr<SignalSlot<A...>>       Slot;

    private:
        std::list<Callback>                                     _callbacks;
        std::list<unsigned int>                                 _slotIds;
        unsigned int                                            _nextSlotId;

        bool                                                    _locked;
        std::list<std::pair<Callback, uint>>                    _toAdd;
        std::list<std::pair<CallbackIterator, SlotIterator>>    _toRemove;

    private:
        Signal() :
            std::enable_shared_from_this<Signal<A...>>(),
            _nextSlotId(0),
            _locked(false)
        {
        }

        void
        removeConnectionById(const unsigned int connectionId)
        {
            auto connectionIdIt = _slotIds.begin();
            auto callbackIt     = _callbacks.begin();
            auto i              = 0;

            while (*connectionIdIt != connectionId)
            {
                connectionIdIt++;
                callbackIt++;
            }

            removeConnectionByIterator(connectionIdIt, callbackIt);
        }

        void
        removeConnectionByIterator(SlotIterator      connectionIdIt,
                                   CallbackIterator  callbackIt)
        {
            if (_locked)
            {
                auto addIt = std::find_if(_toAdd.begin(), _toAdd.end(), [&](std::pair<Callback, unsigned int>& add)
                {
                    return add.second == *connectionIdIt;
                });

                if (addIt != _toAdd.end())
                    _toAdd.erase(addIt);
                else
                    _toRemove.push_back(std::pair<CallbackIterator, SlotIterator>(callbackIt, connectionIdIt));
            }
            else
            {
                _callbacks.erase(callbackIt);
                _slotIds.erase(connectionIdIt);
            }
        }

    public:
        static
        Ptr
        create()
        {
            return std::shared_ptr<Signal<A...>>(new Signal<A...>());
        }

        inline
        uint
        numCallbacks() const
        {
            return _callbacks.size();
        }

        Slot
        connect(Callback callback)
        {
            auto connection = SignalSlot<A...>::create(Signal<A...>::shared_from_this(), _nextSlotId++);

            if (_locked)
                _toAdd.push_back(std::pair<Callback, unsigned int>(callback, connection->_id));
            else
            {
                _callbacks.push_back(callback);
                _slotIds.push_back(connection->_id);
            }

            return connection;
        }

        void
        execute(A... arguments)
        {
            _locked = true;
            for (auto& callback : _callbacks)
                callback(arguments...);
            _locked = false;

            for (auto& callbackAndConnectionId : _toAdd)
            {
                _callbacks.push_back(callbackAndConnectionId.first);
                _slotIds.push_back(callbackAndConnectionId.second);
            }

            for (auto& callbackIt : _toRemove)
            {
                _callbacks.erase(callbackIt.first);
                _slotIds.erase(callbackIt.second);
            }

            _toAdd.clear();
            _toRemove.clear();
        }

    private:
        template <typename... T>
        class SignalSlot :
            public std::enable_shared_from_this<SignalSlot<T...>>
        {
            friend class Signal<T...>;

        public:
            typedef std::shared_ptr<SignalSlot<T...>>   Ptr;

        public:
            std::shared_ptr<Signal<T...>>
            signal()
            {
                return _signal;
            }

            ~SignalSlot()
            {
                if (_signal != nullptr)
                {
                    _signal->removeConnectionById(_id);
                    _signal = nullptr;
                }
            }

        private:
            std::shared_ptr<Signal<T...>>   _signal;
            const unsigned int              _id;

        private:
            inline static
            Ptr
            create(std::shared_ptr<Signal<T...>> signal, const unsigned int id)
            {
                return std::shared_ptr<SignalSlot<T...>>(new SignalSlot(signal, id));
            }

            SignalSlot(std::shared_ptr<Signal<T...>> signal, const unsigned int id) :
                _signal(signal),
                _id(id)
            {
            }
        };

    };
}

As you can see, the important part is the Signal<...>::connect() function that will be used to add a callback. Thanks to C++11, all of this is type checked at compile time. All is good.

Now I want to be able to use Signal<...>::connect() from Lua. In order to make it simple to bind signals using LuaGlue, I've defined the following macro:

#define BIND_SIGNAL(...) \
    {                                                                               \
        auto& s = _state.Class<Signal<__VA_ARGS__>>("Signal<" #__VA_ARGS__ ">");    \
        _state.Class<Signal<__VA_ARGS__>::Slot>("Signal<" #__VA_ARGS__ ">::Slot");  \
        s.method("connect", &Signal<__VA_ARGS__>::connect);                         \
    }

Everything compile fines. But if I actually uses that macro like this:

// LuaScript.cpp
    auto& input_mouse = _state
        .Class<input::Mouse>("Mouse")
            .method("x", &input::Mouse::x)
            .method("y", &input::Mouse::y);

    BIND_SIGNAL(input::Mouse::Ptr);
    input_mouse.method("leftButtonDown", &input::Mouse::leftButtonDown);

-- test.lua
self.test = mouse():leftButtonDown():connect(test_callback)

function test_callback(mouse)
    print("left button down")
end

I have the following error at runtime:

*** Error in `./bin/debug/example-lua-scripts': double free or corruption (!prev): 0x00000000020fb050 ***

and the application freezes. I'm not even allowed to debug it using GDB.

So I have two questions:

• is it actually possible to use Lua functions as some kind of CPP functions in the way described above?
• where might this "double free or corruption" error come from?

Thanks for your help!

[JMLX42]

Ok I've managed to make a very simple test using only my Signal class. The following line will cause the problem:

_state.Class<Signal<input::Mouse::Ptr>>("Signal_input_Mouse_Ptr")

*** Error in `./bin/debug/example-lua-scripts': double free or corruption (!prev): 0x0000000002ed8e80 ***

I'll try to post a complete example based on your sample code soon.

[JMLX42]

Ok the problem appears the fact that I call LuaGlue::glue() multiple times. I don't really have the choice since I load multiple scripts and each script can load new ones etc... So I fixed LuaGlue::glue() to avoid processing the same class/function twice:

bool glue()
        {
            //printf("LuaGlue.glue()\n");
            for(auto &c: classes)
            {
                if (std::find(gluedClasses.begin(), gluedClasses.end(), c.ptr) != gluedClasses.end())
                    continue;
                gluedClasses.push_front(c.ptr);

                if(!c.ptr->glue(this))
                    return false;
            }

            for(auto &c: functions)
            {
                if (std::find(gluedFunctions.begin(), gluedFunctions.end(), c.ptr) != gluedFunctions.end())
                    continue;
                gluedFunctions.push_front(c.ptr);

                if(!c.ptr->glue(this))
                    return false;
            }

            return true;
        }

private:
//...
        std::list<LuaGlueClassBase *> gluedClasses;
        std::list<LuaGlueFunctionBase *> gluedFunctions;

No more problems since this fix.

[Tomasu]

Hmm. I'm not sure why you need to re-glue at all? It glues everything to a lua_State object which you can then load scripts into.

I have some code I'll be adding at some point that will let you load multiple scripts into their own private "container" (Really their own table, that is then made the scripts "global" namespace) in a single lua_State. Would that be useful to you?

[JMLX42]

I have to glue new classes because each script has it's own "stub class". It's the same interface but with the script name so that useful stuff is available in the 'self' table.

[Tomasu]

I'm not sure I get it. Is it like what I suggested above? each script being stuck in a private namespace? so scripts can't stomp on each other?

[JMLX42]

I want each script to have access to specific methods available in my C++ code. Each script defines a class - named after the very file - with 3 methods: start, update and stop.

For example the following script will make it possible to rotate around a target when added to a camera node:

function rotate:start(node)
    self.oldX = self.mouse.x
    self.oldY = self.mouse.y
    self.rotationSpeedX = 0.
    self.rotationSpeedY = 0.
    self.distance = 3.
    self.lookAt = Vector3.create(0., 0., 0.)
    self.up = Vector3.create(0., 1., 0.)
    self.yaw = 0.
    self.pitch = math.pi * .5
    self.minPitch = 0.00001
    self.maxPitch = math.pi - self.minPitch
end

function rotate:update(node)
    local x = self.mouse.x
    local y = self.mouse.y
    if self.mouse.leftButtonIsDown then
        self.rotationSpeedX = self.rotationSpeedX + (x - self.oldX) / 1000
        self.rotationSpeedY = self.rotationSpeedY + (y - self.oldY) / 1000
    end
    self.oldX = x
    self.oldY = y

    self.yaw = self.yaw + self.rotationSpeedX
    self.rotationSpeedX = self.rotationSpeedX * .9

    self.pitch = self.pitch - self.rotationSpeedY
    self.pitch = math.min(math.max(self.pitch, self.minPitch), self.maxPitch)
    self.rotationSpeedY = self.rotationSpeedY * .9

    self:getModelToWorldMatrix(node):lookAt(
        self.lookAt,
        Vector3.create(
            self.lookAt.x + self.distance * math.cos(self.yaw) * math.sin(self.pitch),
            self.lookAt.y + self.distance * math.cos(self.pitch),
            self.lookAt.z + self.distance * math.sin(self.yaw) * math.sin(self.pitch)
        ),
        self.up
    )
end

As you can see, I'm using thinkgs like "self.mouse", So I have to define the "mouse" property for each script (in this case, the "rotate" class). I also define some global functions, but some of them are relative to the very script being loaded and thus have to be defined in self AFAIK.

[Tomasu]

self isn't some magical variable. It's just the object that your method was called on. just a regular old object.

self in this case would just be an instance of "rotate" that you called the update or start method on.

rotate_glue = glue.Class<Rotate>("rotate");
// bind some properties and methods to rotate_glue
glue.glue();

// do whatever here

// much later
// load script here
rotate_obj = new Rotate();
rotate_glue.invokeMethod("start", rotate_obj, a_node);

That will call "rotate:start" and lua will set "self" to rotate_obj.

Also, : isn't really all that special either, it's just syntax sugar for the period. All it does extra is set "self", which isn't needed for dereferencing plain old properties.

[JMLX42]

I don't have a C++ Rotate class. I have a generic LuaStub class that will pipe those start/update/stop Lua method and bind some C++ methods. For this to work, I have to bind this LuaStub class using the "rotate" class name:

// when script is loaded...
_state.Class<LuaStub>("rotate");

or generally:

// when script is loaded...
_stage.Class<LuaStub>(scriptFileName)

Then I have to glue everything. And I have to do this for each script. There is no specific point in time where "every script is loaded" and I can glue "everything". Even the scripts I load can load scripts.

Here is another example with my "main" script, loaded and attached to the root of my 3D scene:

function main:start(node)
    self.assets
        :queue('effect/Basic.effect')
        :queue('effect/Phong.effect')
        :queue('texture/box.png')
        :queue('script/rotate.lua')
        :load()

    local cube = Node.create()
        :addComponent(Transform.create())
        :addComponent(Surface.create(
            CubeGeometry.create(self.assets.context),
            Material.create():setTexture('diffuseMap', self.assets:texture('texture/box.png')),
            self.assets:effect('effect/Phong.effect')
        ))
    node:addChild(cube)

    self.camera = Node.create()
        :addComponent(Renderer.create())
        :addComponent(Transform.createFromMatrix(Matrix4x4.create():lookAt(
            Vector3.zero(),
            Vector3.create(0., 0., 3.),
            Vector3.up()
        )))
        :addComponent(PerspectiveCamera.create(800. / 600., math.pi * .25, .1, 1000.))
        :addComponent(self.assets:script('script/rotate.lua'))
        :addComponent(DirectionalLight.create(.4))
    node:addChild(self.camera)

    local lights = Node.create()
        :addComponent(AmbientLight.create(.2))
        :addComponent(DirectionalLight.create(.4))
    node:addChild(lights)
end

You can see how I use self.assets to load other scripts and Node:addComponent() to attach them. When they'll be attached, the binding process above kicks in so that the "self" object is defined with all I need for every new loaded script.

In a nutshell: we cannot "glue everything once" as soon as we don't have a specific point in time where all scripts are loaded.

[Tomasu]

Ok, well if it helps, LuaGlueClass also has a glue method.

auto _class = glue.Class<LuaStub>("blah");
// do stuff with _class
_class.glue(&glue);

I wonder if a LuaGlueClass::clone(newname) method might be useful.

I'll think about this some more as well.

[Tomasu]

Oh, and there's nothing stopping you from using one LuaGlue object per script I don't think. a lua_State isn't all that heavy, and LuaGlue's overhead is pretty slim.

[JMLX42]

I might need global stuff according to a script "scope" related to my actual 3D scene. This will indeed make use of multiple lua_state. But this scope handling is very specific to how I want to handle scripts, especially some coroutine stuff.

I didn't notice the LuaGlueClass.glue() method and I should use it instead. I just tested it and it works fine.

Yet I think you should either:

• document the fact LuaGlue::glue() should not be called twice;
• or take my minor additions (or something similar)

because this "double free or corruption" issue is very nasty to understand/find/debug with.