From 2cce18946df428f1bc9342a87e1fea7e2a5cb464 Mon Sep 17 00:00:00 2001 From: Alex Szpakowski Date: Sat, 18 Jul 2015 03:26:07 -0300 Subject: [PATCH] Fixed objects which store Lua callback functions to avoid attempting to use dead coroutines when deleting the reference to the callback function. Should fix crashes when coroutines are mixed with those objects. Also improved the performance of World:rayCast and World:queryBoundingBox. --- src/common/Reference.cpp | 40 +++++---------- src/common/Reference.h | 33 +++--------- src/common/runtime.cpp | 39 ++++++++++++-- src/common/runtime.h | 19 ++++++- src/modules/love/love.cpp | 4 +- src/modules/physics/box2d/Body.cpp | 10 +--- src/modules/physics/box2d/Body.h | 2 +- src/modules/physics/box2d/Fixture.cpp | 10 +--- src/modules/physics/box2d/Fixture.h | 2 +- src/modules/physics/box2d/Joint.cpp | 10 +--- src/modules/physics/box2d/Joint.h | 2 +- src/modules/physics/box2d/World.cpp | 65 ++++++++++++++---------- src/modules/physics/box2d/World.h | 23 ++++++--- src/modules/physics/box2d/wrap_World.cpp | 2 + 14 files changed, 139 insertions(+), 122 deletions(-) diff --git a/src/common/Reference.cpp b/src/common/Reference.cpp index e077c3d67..a54ef38e4 100644 --- a/src/common/Reference.cpp +++ b/src/common/Reference.cpp @@ -26,13 +26,13 @@ namespace love const char REFERENCE_TABLE_NAME[] = "love-references"; Reference::Reference() - : L(nullptr) + : pinnedL(nullptr) , idx(LUA_REFNIL) { } Reference::Reference(lua_State *L) - : L(L) + : pinnedL(nullptr) , idx(LUA_REFNIL) { ref(L); @@ -46,7 +46,7 @@ Reference::~Reference() void Reference::ref(lua_State *L) { unref(); // Just to be safe. - this->L = L; + pinnedL = luax_getpinnedthread(L); luax_insist(L, LUA_REGISTRYINDEX, REFERENCE_TABLE_NAME); lua_insert(L, -2); // Move reference table behind value. idx = luaL_ref(L, -2); @@ -57,38 +57,26 @@ void Reference::unref() { if (idx != LUA_REFNIL) { - luax_insist(L, LUA_REGISTRYINDEX, REFERENCE_TABLE_NAME); - luaL_unref(L, -1, idx); - lua_pop(L, 1); + // We use a pinned thread/coroutine for the Lua state because we know it + // hasn't been garbage collected and is valid, as long as the whole lua + // state is still open. + luax_insist(pinnedL, LUA_REGISTRYINDEX, REFERENCE_TABLE_NAME); + luaL_unref(pinnedL, -1, idx); + lua_pop(pinnedL, 1); idx = LUA_REFNIL; } } -void Reference::push(lua_State *newL) +void Reference::push(lua_State *L) { if (idx != LUA_REFNIL) { - luax_insist(newL, LUA_REGISTRYINDEX, REFERENCE_TABLE_NAME); - lua_rawgeti(newL, -1, idx); - lua_remove(newL, -2); + luax_insist(L, LUA_REGISTRYINDEX, REFERENCE_TABLE_NAME); + lua_rawgeti(L, -1, idx); + lua_remove(L, -2); } else - lua_pushnil(newL); -} - -void Reference::push() -{ - push(L); -} - -lua_State *Reference::getL() const -{ - return L; -} - -void Reference::setL(lua_State *newL) -{ - L = newL; + lua_pushnil(L); } } // love diff --git a/src/common/Reference.h b/src/common/Reference.h index 295069536..3c1e9079b 100644 --- a/src/common/Reference.h +++ b/src/common/Reference.h @@ -64,36 +64,17 @@ public: void unref(); /** - * Pushes the referred value onto the stack of a different coroutine - * in the same main Lua state. - * THIS SHOULD NOT BE USED FOR DIFFERENT LUA STATES (created with - * luaL_newstate)! Only with different coroutines! + * Pushes the referred value onto the stack of the specified Lua coroutine. + * NOTE: The coroutine *must* belong to the same Lua state that was used for + * Reference::ref. **/ - void push(lua_State *newL); - - /** - * Pushes the referred value onto the stack. - **/ - void push(); - - /** - * Gets the Lua state associated with this - * reference. - **/ - lua_State *getL() const; - - /** - * Associates a new Lua state with this reference. - * THIS IS DANGEROUS! It is only designed to be - * used with different coroutines from the same - * main Lua state! - **/ - void setL(lua_State *newL); + void push(lua_State *L); private: - // The Lua state in which the reference resides. - lua_State *L; + // A pinned coroutine (probably the main thread) belonging to the Lua state + // in which the reference resides. + lua_State *pinnedL; // Index to the Lua reference. int idx; diff --git a/src/common/runtime.cpp b/src/common/runtime.cpp index e7ba8bd9a..6c5b5f90d 100644 --- a/src/common/runtime.cpp +++ b/src/common/runtime.cpp @@ -83,7 +83,7 @@ Reference *luax_refif(lua_State *L, int type) void luax_printstack(lua_State *L) { - for (int i = 1; i<=lua_gettop(L); i++) + for (int i = 1; i <= lua_gettop(L); i++) std::cout << i << " - " << luaL_typename(L, i) << std::endl; } @@ -590,8 +590,6 @@ int luax_insistregistry(lua_State *L, Registry r) { switch (r) { - case REGISTRY_GC: - return luax_insistlove(L, "_gc"); case REGISTRY_MODULES: return luax_insistlove(L, "_modules"); case REGISTRY_OBJECTS: @@ -605,8 +603,6 @@ int luax_getregistry(lua_State *L, Registry r) { switch (r) { - case REGISTRY_GC: - return luax_getlove(L, "_gc"); case REGISTRY_MODULES: return luax_getlove(L, "_modules"); case REGISTRY_OBJECTS: @@ -617,6 +613,39 @@ int luax_getregistry(lua_State *L, Registry r) } } +static const char *MAIN_THREAD_KEY = "_love_mainthread"; + +lua_State *luax_insistpinnedthread(lua_State *L) +{ + lua_getfield(L, LUA_REGISTRYINDEX, MAIN_THREAD_KEY); + + if (lua_isnoneornil(L, -1)) + { + lua_pop(L, 1); + + // lua_pushthread returns 1 if it's actually the main thread, but we + // can't actually get the real main thread if lua_pushthread doesn't + // return it (in Lua 5.1 at least), so we ignore that for now... + // We do store a strong reference to the current thread/coroutine in + // the registry, however. + lua_pushthread(L); + lua_pushvalue(L, -1); + lua_setfield(L, LUA_REGISTRYINDEX, MAIN_THREAD_KEY); + } + + lua_State *thread = lua_tothread(L, -1); + lua_pop(L, 1); + return thread; +} + +lua_State *luax_getpinnedthread(lua_State *L) +{ + lua_getfield(L, LUA_REGISTRYINDEX, MAIN_THREAD_KEY); + lua_State *thread = lua_tothread(L, -1); + lua_pop(L, 1); + return thread; +} + extern "C" int luax_typerror(lua_State *L, int narg, const char *tname) { int argtype = lua_type(L, narg); diff --git a/src/common/runtime.h b/src/common/runtime.h index f0facdeee..6d7f8cde2 100644 --- a/src/common/runtime.h +++ b/src/common/runtime.h @@ -49,7 +49,6 @@ class Reference; **/ enum Registry { - REGISTRY_GC, REGISTRY_MODULES, REGISTRY_OBJECTS }; @@ -390,6 +389,24 @@ int luax_insistregistry(lua_State *L, Registry r); **/ int luax_getregistry(lua_State *L, Registry r); +/** + * Gets (and pins if needed) a "pinned" Lua thread (coroutine) in the specified + * Lua state. This will usually be the main Lua thread, unless the first call + * to this function for a specific Lua state is made from within a coroutine. + * NOTE: This does not push anything to the stack. + **/ +lua_State *luax_insistpinnedthread(lua_State *L); + +/** + * Gets a "pinned" Lua thread (coroutine) in the specified Lua state. This will + * usually be the main Lua thread. This can be used to access global variables + * in a specific Lua state without needing another alive lua_State value. + * PRECONDITION: luax_insistpinnedthread must have been called on a lua_State + * value corresponding to the Lua state which will be used with this function. + * NOTE: This does not push anything to the stack. + **/ +lua_State *luax_getpinnedthread(lua_State *L); + extern "C" { // Also called from luasocket int luax_typerror(lua_State *L, int narg, const char *tname); } diff --git a/src/modules/love/love.cpp b/src/modules/love/love.cpp index ff2ca66fc..0271a940f 100644 --- a/src/modules/love/love.cpp +++ b/src/modules/love/love.cpp @@ -238,8 +238,10 @@ static int w_love_isVersionCompatible(lua_State *L) return 1; } -int luaopen_love(lua_State * L) +int luaopen_love(lua_State *L) { + love::luax_insistpinnedthread(L); + love::luax_insistglobal(L, "love"); // Set version information. diff --git a/src/modules/physics/box2d/Body.cpp b/src/modules/physics/box2d/Body.cpp index 998685d49..01e7da9bd 100644 --- a/src/modules/physics/box2d/Body.cpp +++ b/src/modules/physics/box2d/Body.cpp @@ -535,15 +535,7 @@ int Body::setUserData(lua_State *L) body->SetUserData((void *) udata); } - if (udata->ref != nullptr) - { - // We set the Reference's lua_State to this one before deleting it, so - // it unrefs using the current lua_State's stack. This is necessary - // if setUserData is called in a coroutine. - udata->ref->setL(L); - delete udata->ref; - } - + delete udata->ref; udata->ref = new Reference(L); return 0; diff --git a/src/modules/physics/box2d/Body.h b/src/modules/physics/box2d/Body.h index e399cbc92..d222a19fb 100644 --- a/src/modules/physics/box2d/Body.h +++ b/src/modules/physics/box2d/Body.h @@ -48,7 +48,7 @@ class Fixture; struct bodyudata { // Reference to arbitrary data. - Reference *ref; + Reference *ref = nullptr; }; /** diff --git a/src/modules/physics/box2d/Fixture.cpp b/src/modules/physics/box2d/Fixture.cpp index ff014355a..af33e128b 100644 --- a/src/modules/physics/box2d/Fixture.cpp +++ b/src/modules/physics/box2d/Fixture.cpp @@ -227,15 +227,7 @@ int Fixture::setUserData(lua_State *L) { love::luax_assert_argc(L, 1, 1); - if (data->ref != nullptr) - { - // We set the Reference's lua_State to this one before deleting it, so - // it unrefs using the current lua_State's stack. This is necessary - // if setUserData is called in a coroutine. - data->ref->setL(L); - delete data->ref; - } - + delete data->ref; data->ref = new Reference(L); return 0; diff --git a/src/modules/physics/box2d/Fixture.h b/src/modules/physics/box2d/Fixture.h index f942592cd..32c362453 100644 --- a/src/modules/physics/box2d/Fixture.h +++ b/src/modules/physics/box2d/Fixture.h @@ -47,7 +47,7 @@ namespace box2d struct fixtureudata { // Reference to arbitrary data. - Reference *ref; + Reference *ref = nullptr; }; /** diff --git a/src/modules/physics/box2d/Joint.cpp b/src/modules/physics/box2d/Joint.cpp index acc76d544..a7f2d4e3a 100644 --- a/src/modules/physics/box2d/Joint.cpp +++ b/src/modules/physics/box2d/Joint.cpp @@ -193,15 +193,7 @@ int Joint::setUserData(lua_State *L) { love::luax_assert_argc(L, 1, 1); - if (udata->ref != nullptr) - { - // We set the Reference's lua_State to this one before deleting it, so - // it unrefs using the current lua_State's stack. This is necessary - // if setUserData is called in a coroutine. - udata->ref->setL(L); - delete udata->ref; - } - + delete udata->ref; udata->ref = new Reference(L); return 0; diff --git a/src/modules/physics/box2d/Joint.h b/src/modules/physics/box2d/Joint.h index d20bb3856..36149c9dd 100644 --- a/src/modules/physics/box2d/Joint.h +++ b/src/modules/physics/box2d/Joint.h @@ -46,7 +46,7 @@ class World; struct jointudata { // Reference to arbitrary data. - Reference *ref; + Reference *ref = nullptr; }; /** diff --git a/src/modules/physics/box2d/World.cpp b/src/modules/physics/box2d/World.cpp index 2cb973046..564d0daf6 100644 --- a/src/modules/physics/box2d/World.cpp +++ b/src/modules/physics/box2d/World.cpp @@ -36,6 +36,7 @@ namespace box2d World::ContactCallback::ContactCallback() : ref(nullptr) + , L(nullptr) { } @@ -48,10 +49,9 @@ World::ContactCallback::~ContactCallback() void World::ContactCallback::process(b2Contact *contact, const b2ContactImpulse *impulse) { // Process contacts. - if (ref != nullptr) + if (ref != nullptr && L != nullptr) { - lua_State *L = ref->getL(); - ref->push(); + ref->push(L); // Push first fixture. { @@ -97,6 +97,7 @@ void World::ContactCallback::process(b2Contact *contact, const b2ContactImpulse World::ContactFilter::ContactFilter() : ref(nullptr) + , L(nullptr) { } @@ -124,10 +125,9 @@ bool World::ContactFilter::process(Fixture *a, Fixture *b) (filterB[1] & filterA[0]) == 0) return false; // A and B aren't set to collide - if (ref != nullptr) + if (ref != nullptr && L != nullptr) { - lua_State *L = ref->getL(); - ref->push(); + ref->push(L); luax_pushtype(L, PHYSICS_FIXTURE_ID, a); luax_pushtype(L, PHYSICS_FIXTURE_ID, b); lua_call(L, 2, 1); @@ -136,50 +136,51 @@ bool World::ContactFilter::process(Fixture *a, Fixture *b) return true; } -World::QueryCallback::QueryCallback() - : ref(nullptr) +World::QueryCallback::QueryCallback(lua_State *L, int idx) + : L(L) + , funcidx(idx) { + luaL_checktype(L, funcidx, LUA_TFUNCTION); } World::QueryCallback::~QueryCallback() { - if (ref != nullptr) - delete ref; } bool World::QueryCallback::ReportFixture(b2Fixture *fixture) { - if (ref != nullptr) + if (L != nullptr) { - lua_State *L = ref->getL(); - ref->push(); + lua_pushvalue(L, funcidx); Fixture *f = (Fixture *)Memoizer::find(fixture); if (!f) throw love::Exception("A fixture has escaped Memoizer!"); luax_pushtype(L, PHYSICS_FIXTURE_ID, f); lua_call(L, 1, 1); - return luax_toboolean(L, -1); + bool cont = luax_toboolean(L, -1); + lua_pop(L, 1); + return cont; } + return true; } -World::RayCastCallback::RayCastCallback() - : ref(nullptr) +World::RayCastCallback::RayCastCallback(lua_State *L, int idx) + : L(L) + , funcidx(idx) { + luaL_checktype(L, funcidx, LUA_TFUNCTION); } World::RayCastCallback::~RayCastCallback() { - if (ref != nullptr) - delete ref; } float32 World::RayCastCallback::ReportFixture(b2Fixture *fixture, const b2Vec2 &point, const b2Vec2 &normal, float32 fraction) { - if (ref != nullptr) + if (L != nullptr) { - lua_State *L = ref->getL(); - ref->push(); + lua_pushvalue(L, funcidx); Fixture *f = (Fixture *)Memoizer::find(fixture); if (!f) throw love::Exception("A fixture has escaped Memoizer!"); @@ -193,8 +194,11 @@ float32 World::RayCastCallback::ReportFixture(b2Fixture *fixture, const b2Vec2 & lua_call(L, 6, 1); if (!lua_isnumber(L, -1)) luaL_error(L, "Raycast callback didn't return a number!"); - return (float32)lua_tonumber(L, -1); + float32 fraction = (float32) lua_tonumber(L, -1); + lua_pop(L, 1); + return fraction; } + return 0; } @@ -345,24 +349,28 @@ int World::setCallbacks(lua_State *L) { lua_pushvalue(L, 1); begin.ref = luax_refif(L, LUA_TFUNCTION); + begin.L = L; } if (nargs >= 2) { lua_pushvalue(L, 2); end.ref = luax_refif(L, LUA_TFUNCTION); + end.L = L; } if (nargs >= 3) { lua_pushvalue(L, 3); presolve.ref = luax_refif(L, LUA_TFUNCTION); + presolve.L = L; } if (nargs >= 4) { lua_pushvalue(L, 4); postsolve.ref = luax_refif(L, LUA_TFUNCTION); + postsolve.L = L; } return 0; @@ -377,6 +385,11 @@ int World::getCallbacks(lua_State *L) return 4; } +void World::setCallbacksL(lua_State *L) +{ + begin.L = end.L = presolve.L = postsolve.L = filter.L = L; +} + int World::setContactFilter(lua_State *L) { if (!lua_isnoneornil(L, 1)) @@ -385,6 +398,7 @@ int World::setContactFilter(lua_State *L) if (filter.ref) delete filter.ref; filter.ref = luax_refif(L, LUA_TFUNCTION); + filter.L = L; return 0; } @@ -519,8 +533,7 @@ int World::queryBoundingBox(lua_State *L) box.lowerBound = Physics::scaleDown(b2Vec2(lx, ly)); box.upperBound = Physics::scaleDown(b2Vec2(ux, uy)); luaL_checktype(L, 5, LUA_TFUNCTION); - if (query.ref) delete query.ref; - query.ref = luax_refif(L, LUA_TFUNCTION); + QueryCallback query(L, 5); world->QueryAABB(&query, box); return 0; } @@ -534,9 +547,7 @@ int World::rayCast(lua_State *L) b2Vec2 v1 = Physics::scaleDown(b2Vec2(x1, y1)); b2Vec2 v2 = Physics::scaleDown(b2Vec2(x2, y2)); luaL_checktype(L, 5, LUA_TFUNCTION); - if (raycast.ref) - delete raycast.ref; - raycast.ref = luax_refif(L, LUA_TFUNCTION); + RayCastCallback raycast(L, 5); world->RayCast(&raycast, v1, v2); return 0; } diff --git a/src/modules/physics/box2d/World.h b/src/modules/physics/box2d/World.h index 6e4e7da7e..da35d4f7a 100644 --- a/src/modules/physics/box2d/World.h +++ b/src/modules/physics/box2d/World.h @@ -70,6 +70,7 @@ public: { public: Reference *ref; + lua_State *L; ContactCallback(); ~ContactCallback(); void process(b2Contact *contact, const b2ContactImpulse *impulse = NULL); @@ -79,6 +80,7 @@ public: { public: Reference *ref; + lua_State *L; ContactFilter(); ~ContactFilter(); bool process(Fixture *a, Fixture *b); @@ -87,19 +89,23 @@ public: class QueryCallback : public b2QueryCallback { public: - Reference *ref; - QueryCallback(); + QueryCallback(lua_State *L, int idx); ~QueryCallback(); virtual bool ReportFixture(b2Fixture *fixture); + private: + lua_State *L; + int funcidx; }; class RayCastCallback : public b2RayCastCallback { public: - Reference *ref; - RayCastCallback(); + RayCastCallback(lua_State *L, int idx); ~RayCastCallback(); virtual float32 ReportFixture(b2Fixture *fixture, const b2Vec2 &point, const b2Vec2 &normal, float32 fraction); + private: + lua_State *L; + int funcidx; }; /** @@ -157,6 +163,13 @@ public: **/ int getCallbacks(lua_State *L); + /** + * Updates the Lua thread/coroutine used when callbacks are executed in + * the update method. This should be called in the same Lua function which + * calls update(). + **/ + void setCallbacksL(lua_State *L); + /** * Sets the ContactFilter callback. **/ @@ -281,8 +294,6 @@ private: // Contact callbacks. ContactCallback begin, end, presolve, postsolve; ContactFilter filter; - QueryCallback query; - RayCastCallback raycast; }; } // box2d diff --git a/src/modules/physics/box2d/wrap_World.cpp b/src/modules/physics/box2d/wrap_World.cpp index e88b93459..a0c86082e 100644 --- a/src/modules/physics/box2d/wrap_World.cpp +++ b/src/modules/physics/box2d/wrap_World.cpp @@ -39,6 +39,8 @@ int w_World_update(lua_State *L) { World *t = luax_checkworld(L, 1); float dt = (float)luaL_checknumber(L, 2); + // Make sure the world callbacks are using the calling Lua thread. + t->setCallbacksL(L); luax_catchexcept(L, [&](){ t->update(dt); }); return 0; }