Re: super bad

Nigel Atkinson wrote:

> I had a bug in an app I'm writing, and while it was my goof i.e. you
> shouldn't really rely on the order of objects being garbage collected, I
> thought I'd share a debugging post mortem.  I'm using 0.9.0 but it may
> apply to 0.8.1 and possibly 0.7 
>
> So I found that last created instance of a Lua class that has a base
> class, stayed around until lua_close, despite having any reference I
> knew about set to nil.
>
> In hind sight 'super' seems almost obvious.  Even though it is
> deprecated, it's still there, to tell you its deprecated when you use
> it. 
>
> It took me awhile to narrow it down to this, as I had shared pointers in
> the mix witch sent me barking up the wrong tree. ;-)
>
> I just commented out the 'super' code in class_rep.cpp in the 
>
> int luabind::detail::class_rep::constructor_dispatcher(lua_State* L)
>
> function.  It's pretty obvious what to zap.  
>
> Seeing as super is depreciated, this problem will go away eventually.  A
> quick fix I guess would be to set 'super' to nil, after you have created
> a class instance.
>   
Thanks for bringing this to my attention. I've pushed this fix to the
0.9 branch:

commit 2c99f0475afea7c282c2e432499fd22aa17744e3
Author: Daniel Wallin <[email protected]>
Date:   Wed Nov 25 21:23:52 2009 +0100

    Fix leak in "super" upvalue.
    
    The last created instance would stick around in the "super" upvalue
    unless "super()" was called from the constructor. This happened because
    "super" wasn't cleaned up after construction, only after being called.

diff --git a/src/class_rep.cpp b/src/class_rep.cpp
index 4ee657c..0f8ab1b 100755
--- a/src/class_rep.cpp
+++ b/src/class_rep.cpp
@@ -126,6 +126,12 @@ luabind::detail::class_rep::allocate(lua_State* L) const
 	return std::pair<void*,void*>(mem, 0);
 }
 
+namespace
+{
+
+  bool super_deprecation_disabled = false;
+
+} // namespace unnamed
 
 // this is called as metamethod __call on the class_rep.
 int luabind::detail::class_rep::constructor_dispatcher(lua_State* L)
@@ -142,8 +148,9 @@ int luabind::detail::class_rep::constructor_dispatcher(lua_State* L)
     lua_rawgeti(L, LUA_REGISTRYINDEX, cls->metatable_ref());
     lua_setmetatable(L, -2);
 
-    if (cls->get_class_type() == class_rep::lua_class
-     && !cls->bases().empty())
+    if (super_deprecation_disabled
+        && cls->get_class_type() == class_rep::lua_class
+        && !cls->bases().empty())
     {
         lua_pushstring(L, "super");
         lua_pushvalue(L, 1);
@@ -166,6 +173,13 @@ int luabind::detail::class_rep::constructor_dispatcher(lua_State* L)
 
     lua_call(L, args, 0);
 
+    if (super_deprecation_disabled)
+    {
+        lua_pushstring(L, "super");
+        lua_pushnil(L);
+        lua_settable(L, LUA_GLOBALSINDEX);
+    }
+
     return 1;
 }
 
@@ -192,13 +206,6 @@ void luabind::detail::class_rep::add_base_class(const luabind::detail::class_rep
 	m_bases.push_back(binfo);
 }
 
-namespace
-{
-
-  bool super_deprecation_disabled = false;
-
-} // namespace unnamed
-
 LUABIND_API void luabind::disable_super_deprecation()
 {
     super_deprecation_disabled = true;
@@ -206,16 +213,6 @@ LUABIND_API void luabind::disable_super_deprecation()
 
 int luabind::detail::class_rep::super_callback(lua_State* L)
 {
-	if (!super_deprecation_disabled)
-	{
-		lua_pushstring(L,
-			"DEPRECATION: 'super' has been deprecated in favor of "
-			"directly calling the base class __init() function. "
-			"This error can be disabled by calling 'luabind::disable_super_deprecation()'."
-		);
-		lua_error(L);
-	}
-
 	int args = lua_gettop(L);
 		
 	class_rep* crep = static_cast<class_rep*>(lua_touserdata(L, lua_upvalueindex(1)));
diff --git a/src/open.cpp b/src/open.cpp
index e39d53d..24ff4e8 100755
--- a/src/open.cpp
+++ b/src/open.cpp
@@ -52,6 +52,18 @@ namespace
 
   int main_thread_tag;
 
+  int deprecated_super(lua_State* L)
+  {
+      lua_pushstring(L,
+          "DEPRECATION: 'super' has been deprecated in favor of "
+          "directly calling the base class __init() function. "
+          "This error can be disabled by calling 'luabind::disable_super_deprecation()'."
+      );
+      lua_error(L);
+
+      return 0;
+  }
+
 } // namespace unnamed
 
     LUABIND_API lua_State* get_main_thread(lua_State* L)
@@ -142,6 +154,10 @@ namespace
         lua_pushlightuserdata(L, &main_thread_tag);
         lua_pushlightuserdata(L, L);
         lua_rawset(L, LUA_REGISTRYINDEX);
+
+        lua_pushstring(L, "super");
+        lua_pushcclosure(L, &deprecated_super, 0);
+        lua_settable(L, LUA_GLOBALSINDEX);
     }
 
 } // namespace luabind
diff --git a/test/Jamfile b/test/Jamfile
index 21f298c..d1343cb 100755
--- a/test/Jamfile
+++ b/test/Jamfile
@@ -42,6 +42,7 @@ SOURCES =
     test_create_in_thread.cpp
     test_extend_class_in_lua.cpp
     test_smart_ptr_attributes.cpp
+    test_super_leak.cpp
  ;
 
 obj main : main.cpp : <library>..//luabind : : <library>..//luabind ;
diff --git a/test/test_super_leak.cpp b/test/test_super_leak.cpp
new file mode 100644
index 0000000..e2d87e2
--- /dev/null
+++ b/test/test_super_leak.cpp
@@ -0,0 +1,60 @@
+// Copyright Daniel Wallin 2009. Use, modification and distribution is
+// subject to the Boost Software License, Version 1.0. (See accompanying
+// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt)
+
+
+#include "test.hpp"
+#include <luabind/luabind.hpp>
+
+struct Foo
+{
+    static std::size_t count;
+
+    Foo()
+    {
+        ++count;
+    }
+
+    ~Foo()
+    {
+        --count;
+    }
+};
+
+std::size_t Foo::count = 0;
+
+void test_main(lua_State* L)
+{
+    using namespace luabind;
+
+    module(L) [
+        class_<Foo>("Foo")
+          .def(constructor<>())
+    ];
+
+    // This used to leak the last created instance in one of the
+    // upvalues to the "super" function.
+
+    disable_super_deprecation();
+
+    DOSTRING(L,
+        "class 'Bar' (Foo)\n"
+        "function Bar.__init(self)\n"
+        "  Foo.__init(self)\n"
+        "end\n"
+    );
+
+    DOSTRING(L,
+        "x = Bar()\n"
+    );
+
+    assert(Foo::count == 1);
+
+    DOSTRING(L,
+        "x = nil\n"
+    );
+
+    lua_gc(L, LUA_GCCOLLECT, 0);
+
+    assert(Foo::count == 0);
+}


-- 
Daniel Wallin
BoostPro Computing
http://www.boostpro.com


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
trial. Simplify your report design, integration and deployment - and focus on 
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july