system-controller: mark masks DYNAMIC, add funcbridge gettop/settop wrappers.

Mark Lua *_mask classes (window_mask, layer_mask, input_mask,
output_mask, code_mask) as dynamic.

Finally (I think) I understood where the mysterious and seemingly
quasi-randomly occuring dynamic object leaks were stemming from.
It is caused by functionbridge invocations from a peculiar type
of context (see below). Sadly, in its current form, there is no
easy automatic way to mitigate the problem, or at least I haven't
managed to come up with one yet, to protect people from shooting
themselves in the foot with other instances of this bug. For the
time being folks will need to make sure they do the right things,
which inherently and eventually tends to be error prone...

Anyway, to make a long story short, the leaks were stemming from
calls to mrp_func{bridge,array}_call_from_c from contexts which
were not triggered, directly or indirectly, from Lua from a control
flow perspective. IOW, if there was a call to one of the *from_c
bridge-invoking and on the return path we never ended up returning
to the Lua interpreter (which would have then had been the one
having made the call out to C/us) then all the objects that were
pushed on the Lua stack before the call to the *from_c function
were simply left on the Lua stack forever.

These were not genuine leaks in the sense that the object still
were reachable on the Lua stack, it was just that there was never
a return to Lua or to anybode else to clean up the stack for us
so the object were never ever reclaimed.

The fix for now is to make sure that all invocations to bridges
strictly follow the pattern below, which makes sure that the Lua
stack top is saved before anything is pushed there and restored
before returning from the function:

  ...
  int top;

  error checks and potential bailout();

  top = lua_gettop(L);

  /* from here on no direct bailout, only return via 'goto out;' */
  preparation argument gathering and creation();

  fill funcbridge argument array();
  invoke funcbridge with one of the call_from_c calls();
  free returned values if needed();

  out;
  lua_settop(L, top);

  return status;

Now there are a couple of ways how this could be made safer / less
error prone, but AFAICT all of those require slight changes to the
funcbridge implementation and/or signature.

One, and a rather trivial, way would be to let people pass in the
stack top to be restored to the *_call_from_c functions and let
these make sure that the stack is restored properly before they
return. This would still require the caller to correctly determine
the right stack top, but it would save him from the trouble of
having to remember to correctly restore the stack on all return
pathes.

Another alternative would be to change the funcbridge signature
and allow the called to indicate there how many items were pushed
to the stack just for the funcbridge call (eg. the last item being
the number of items to clean up), or to have separate signature
markers for items that have been specifically allocated for the
funcbridge call. So for instance O would indicate a Lua object
while D would indicate a Lua object that needs to be cleaned from
the stack before returning. Neither of these would be fully fail-
safe though as the caller still needs to provide the right info
one way or another...

Change-Id: If002c8bb4936bbc8e2677dfb9cdc4a0a640c78d6

diff --git a/src/plugins/system-controller/plugin-system-controller.c b/src/plugins/system-controller/plugin-system-controller.c
index a7482f9..66ca144 100644 (file)
--- a/src/plugins/system-controller/plugin-system-controller.c
+++ b/src/plugins/system-controller/plugin-system-controller.c
@@ -274,6 +274,7 @@ static void disconnected_event(client_t *c)
     mrp_funcbridge_value_t args[4];
     mrp_funcbridge_value_t ret;
     char                   rt;
+    int                    top;
 
     /*
      * crate and emit a synthetic disconnection message if the client
@@ -304,6 +305,8 @@ static void disconnected_event(client_t *c)
     h = c->sc->handler.client;
 
     if (h != NULL) {
+        top = lua_gettop(c->sc->L);
+
         args[0].pointer = c->sc->scl;
         args[1].integer = c->id;
         args[2].pointer = mrp_json_lua_wrap(c->sc->L, req);
@@ -314,6 +317,8 @@ static void disconnected_event(client_t *c)
                           ret.string ? ret.string : "<unknown error>");
             mrp_free((void *)ret.string);
         }
+
+        lua_settop(c->sc->L, top);
     }
 
     mrp_json_unref(req);
@@ -435,6 +440,7 @@ static void recv_evt(mrp_transport_t *t, void *data, void *user_data)
     mrp_funcbridge_value_t args[4];
     mrp_funcbridge_value_t ret;
     char                   rt;
+    int                    top;
 
     MRP_UNUSED(t);
 
@@ -461,6 +467,8 @@ static void recv_evt(mrp_transport_t *t, void *data, void *user_data)
         h = sc->handler.client;
 
         if (h != NULL) {
+            top = lua_gettop(sc->L);
+
             args[0].pointer = sc->scl;
             args[1].integer = c->id;
             args[2].pointer = mrp_json_lua_wrap(sc->L, req);
@@ -471,6 +479,8 @@ static void recv_evt(mrp_transport_t *t, void *data, void *user_data)
                               ret.string ? ret.string : "<unknown error>");
                 mrp_free((void *)ret.string);
             }
+
+            lua_settop(sc->L, top);
         }
     }
 
@@ -489,6 +499,8 @@ static void recv_evt(mrp_transport_t *t, void *data, void *user_data)
     }
 
     if (h != NULL || (h = sc->handler.generic) != NULL) {
+        top = lua_gettop(sc->L);
+
         args[0].pointer = sc->scl;
         args[1].integer = c->id;
         args[2].pointer = mrp_json_lua_wrap(sc->L, req);
@@ -500,6 +512,8 @@ static void recv_evt(mrp_transport_t *t, void *data, void *user_data)
                           ret.string ? ret.string : "<unknown error>");
             mrp_free((void *)ret.string);
         }
+
+        lua_settop(sc->L, top);
     }
     else
         mrp_debug("No handler for system-controller message of type 0x%x.",

diff --git a/src/plugins/system-controller/resource-manager/scripting-resource-manager.c b/src/plugins/system-controller/resource-manager/scripting-resource-manager.c
index 837f089..3d5858c 100644 (file)
--- a/src/plugins/system-controller/resource-manager/scripting-resource-manager.c
+++ b/src/plugins/system-controller/resource-manager/scripting-resource-manager.c
@@ -300,6 +300,7 @@ static void event_handler_callback(mrp_resmgr_t *resmgr,
     mrp_funcbridge_t *fb;
     mrp_funcbridge_value_t args[4], ret;
     char t;
+    int top;
 
     MRP_ASSERT(resmgr && event, "invalid argument");
 
@@ -314,6 +315,8 @@ static void event_handler_callback(mrp_resmgr_t *resmgr,
         return;
     }
 
+    top = lua_gettop(L);
+
     switch (event->type) {
 
     case MRP_RESMGR_EVENT_SCREEN:
@@ -330,18 +333,18 @@ static void event_handler_callback(mrp_resmgr_t *resmgr,
 
     default:
         mrp_debug("can't deliver resource events to LUA: invalid event type");
-        return;
+        goto out;
     }
 
     if (!sev) {
         mrp_debug("can't deliver %s resource events to LUA: "
                   "failed to create scripting event", type_str);
-        return;
+        goto out;
     }
     if (!fb) {
         mrp_debug("can't deliver %s resource events to LUA: "
                   "no funcbridge", type_str);
-        return;
+        goto out;
     }
 
     args[0].pointer = rm;
@@ -354,6 +357,9 @@ static void event_handler_callback(mrp_resmgr_t *resmgr,
                       "method (%s)", ret.string ? ret.string : "NULL");
         mrp_free((void *)ret.string);
     }
+
+ out:
+    lua_settop(L, top);
 }
 
 

diff --git a/src/plugins/system-controller/wayland/scripting-code.c b/src/plugins/system-controller/wayland/scripting-code.c
index 9fba52d..924800d 100644 (file)
--- a/src/plugins/system-controller/wayland/scripting-code.c
+++ b/src/plugins/system-controller/wayland/scripting-code.c
@@ -100,7 +100,7 @@ MRP_LUA_CLASS_DEF_SIMPLE (
     )
 );
 
-MRP_LUA_CLASS_DEF_SIMPLE (
+MRP_LUA_CLASS_DEF_SIMPLE_FLAGS (
     code_mask,                  /* class name */
     scripting_code_mask_t,      /* userdata type */
     code_mask_destroy,          /* userdata destructor */
@@ -113,7 +113,8 @@ MRP_LUA_CLASS_DEF_SIMPLE (
        MRP_LUA_OVERRIDE_GETFIELD  (code_mask_getfield)
        MRP_LUA_OVERRIDE_SETFIELD  (code_mask_setfield)
        MRP_LUA_OVERRIDE_STRINGIFY (code_mask_stringify)
-    )
+    ),
+    MRP_LUA_CLASS_DYNAMIC
 );
 
 

diff --git a/src/plugins/system-controller/wayland/scripting-input-manager.c b/src/plugins/system-controller/wayland/scripting-input-manager.c
index ce94612..2ec0fb4 100644 (file)
--- a/src/plugins/system-controller/wayland/scripting-input-manager.c
+++ b/src/plugins/system-controller/wayland/scripting-input-manager.c
@@ -828,6 +828,7 @@ static void input_update_callback(mrp_wayland_t *wl,
     lua_State *L;
     scripting_inpmgr_t *inpmgr;
     mrp_funcbridge_value_t args[4], ret;
+    int top;
     char t;
     bool success;
 
@@ -853,6 +854,8 @@ static void input_update_callback(mrp_wayland_t *wl,
         return;
     }
 
+    top = lua_gettop(L);
+
     args[0].pointer = inpmgr;
     args[1].integer = oper;
     args[2].pointer = inp->scripting_data;
@@ -867,6 +870,8 @@ static void input_update_callback(mrp_wayland_t *wl,
                       "(%s)", inpmgr->name, ret.string ? ret.string : "NULL");
         mrp_free((void *)ret.string);
     }
+
+    lua_settop(L, top);
 }
 
 static void code_update_callback(mrp_wayland_t *wl,
@@ -877,6 +882,7 @@ static void code_update_callback(mrp_wayland_t *wl,
     lua_State *L;
     scripting_inpmgr_t *inpmgr;
     mrp_funcbridge_value_t args[4], ret;
+    int top;
     char t;
     bool success;
 
@@ -902,6 +908,8 @@ static void code_update_callback(mrp_wayland_t *wl,
         return;
     }
 
+    top = lua_gettop(L);
+
     args[0].pointer = inpmgr;
     args[1].integer = oper;
     args[2].pointer = code->scripting_data;
@@ -916,6 +924,8 @@ static void code_update_callback(mrp_wayland_t *wl,
                       "(%s)", inpmgr->name, ret.string ? ret.string : "NULL");
         mrp_free((void *)ret.string);
     }
+
+    lua_settop(L, top);
 }
 
 #if 0

diff --git a/src/plugins/system-controller/wayland/scripting-input.c b/src/plugins/system-controller/wayland/scripting-input.c
index f643c1d..5e9b438 100644 (file)
--- a/src/plugins/system-controller/wayland/scripting-input.c
+++ b/src/plugins/system-controller/wayland/scripting-input.c
@@ -98,7 +98,7 @@ MRP_LUA_CLASS_DEF_SIMPLE (
     )
 );
 
-MRP_LUA_CLASS_DEF_SIMPLE (
+MRP_LUA_CLASS_DEF_SIMPLE_FLAGS (
     input_mask,                 /* class name */
     scripting_input_mask_t,     /* userdata type */
     input_mask_destroy,         /* userdata destructor */
@@ -111,7 +111,8 @@ MRP_LUA_CLASS_DEF_SIMPLE (
        MRP_LUA_OVERRIDE_GETFIELD  (input_mask_getfield)
        MRP_LUA_OVERRIDE_SETFIELD  (input_mask_setfield)
        MRP_LUA_OVERRIDE_STRINGIFY (input_mask_stringify)
-    )
+    ),
+    MRP_LUA_CLASS_DYNAMIC
 );
 
 

diff --git a/src/plugins/system-controller/wayland/scripting-layer.c b/src/plugins/system-controller/wayland/scripting-layer.c
index 10d8a9c..0c08e16 100644 (file)
--- a/src/plugins/system-controller/wayland/scripting-layer.c
+++ b/src/plugins/system-controller/wayland/scripting-layer.c
@@ -98,7 +98,7 @@ MRP_LUA_CLASS_DEF_SIMPLE (
     )
 );
 
-MRP_LUA_CLASS_DEF_SIMPLE (
+MRP_LUA_CLASS_DEF_SIMPLE_FLAGS (
     layer_mask,                 /* class name */
     scripting_layer_mask_t,     /* userdata type */
     layer_mask_destroy,         /* userdata destructor */
@@ -111,7 +111,8 @@ MRP_LUA_CLASS_DEF_SIMPLE (
        MRP_LUA_OVERRIDE_GETFIELD  (layer_mask_getfield)
        MRP_LUA_OVERRIDE_SETFIELD  (layer_mask_setfield)
        MRP_LUA_OVERRIDE_STRINGIFY (layer_mask_stringify)
-    )
+    ),
+    MRP_LUA_CLASS_DYNAMIC
 );
 
 

diff --git a/src/plugins/system-controller/wayland/scripting-output.c b/src/plugins/system-controller/wayland/scripting-output.c
index 744c380..29a9b3b 100644 (file)
--- a/src/plugins/system-controller/wayland/scripting-output.c
+++ b/src/plugins/system-controller/wayland/scripting-output.c
@@ -100,7 +100,7 @@ MRP_LUA_CLASS_DEF_SIMPLE (
     )
 );
 
-MRP_LUA_CLASS_DEF_SIMPLE (
+MRP_LUA_CLASS_DEF_SIMPLE_FLAGS (
     output_mask,                /* class name */
     scripting_output_mask_t,    /* userdata type */
     output_mask_destroy,        /* userdata destructor */
@@ -112,7 +112,8 @@ MRP_LUA_CLASS_DEF_SIMPLE (
        MRP_LUA_OVERRIDE_GETFIELD  (output_mask_getfield)
        MRP_LUA_OVERRIDE_SETFIELD  (output_mask_setfield)
        MRP_LUA_OVERRIDE_STRINGIFY (output_mask_stringify)
-    )
+    ),
+    MRP_LUA_CLASS_DYNAMIC
 );
 
 

diff --git a/src/plugins/system-controller/wayland/scripting-window-manager.c b/src/plugins/system-controller/wayland/scripting-window-manager.c
index 1fb2689..45963d6 100644 (file)
--- a/src/plugins/system-controller/wayland/scripting-window-manager.c
+++ b/src/plugins/system-controller/wayland/scripting-window-manager.c
@@ -852,6 +852,7 @@ static void window_update_callback(mrp_wayland_t *wl,
     lua_State *L;
     scripting_winmgr_t *winmgr;
     mrp_funcbridge_value_t args[4], ret;
+    int top;
     char t;
     bool success;
 
@@ -877,6 +878,8 @@ static void window_update_callback(mrp_wayland_t *wl,
         return;
     }
 
+    top = lua_gettop(L);
+
     args[0].pointer = winmgr;
     args[1].integer = oper;
     args[2].pointer = win->scripting_data;
@@ -891,6 +894,8 @@ static void window_update_callback(mrp_wayland_t *wl,
                       "(%s)", winmgr->name, ret.string ? ret.string : "NULL");
         mrp_free((void *)ret.string);
     }
+
+    lua_settop(L, top);
 }
 
 static void window_hint_callback(mrp_wayland_t *wl,
@@ -901,6 +906,7 @@ static void window_hint_callback(mrp_wayland_t *wl,
     scripting_winmgr_t *winmgr;
     void *wh;
     mrp_funcbridge_value_t args[4], ret;
+    int top;
     char t;
     bool success;
 
@@ -920,6 +926,8 @@ static void window_hint_callback(mrp_wayland_t *wl,
 
     MRP_ASSERT(wl == winmgr->wl, "confused with data structures");
 
+    top = lua_gettop(L);
+
     if (!(wh = mrp_wayland_scripting_window_hint_create_from_c(L, hint)))
         return;
 
@@ -937,6 +945,8 @@ static void window_hint_callback(mrp_wayland_t *wl,
                       "(%s)", winmgr->name, ret.string ? ret.string : "NULL");
         mrp_free((void *)ret.string);
     }
+
+    lua_settop(L, top);
 }
 
 static bool output_request_bridge(lua_State *L,
@@ -1006,6 +1016,7 @@ static void output_update_callback(mrp_wayland_t *wl,
     lua_State *L;
     scripting_winmgr_t *winmgr;
     mrp_funcbridge_value_t args[4], ret;
+    int top;
     char t;
     bool success;
 
@@ -1030,6 +1041,8 @@ static void output_update_callback(mrp_wayland_t *wl,
         return;
     }
 
+    top = lua_gettop(L);
+
     args[0].pointer = winmgr;
     args[1].integer = oper;
     args[2].pointer = out->scripting_data;
@@ -1044,6 +1057,8 @@ static void output_update_callback(mrp_wayland_t *wl,
                       "(%s)", winmgr->name, ret.string ? ret.string : "NULL");
         mrp_free((void *)ret.string);
     }
+
+    lua_settop(L, top);
 }
 
 static bool area_create_bridge(lua_State *L,
@@ -1176,6 +1191,7 @@ static void layer_update_callback(mrp_wayland_t *wl,
     lua_State *L;
     scripting_winmgr_t *winmgr;
     mrp_funcbridge_value_t args[4], ret;
+    int top;
     char t;
     bool success;
 
@@ -1199,6 +1215,8 @@ static void layer_update_callback(mrp_wayland_t *wl,
         return;
     }
 
+    top = lua_gettop(L);
+
     args[0].pointer = winmgr;
     args[1].integer = oper;
     args[2].pointer = layer->scripting_data;
@@ -1213,6 +1231,8 @@ static void layer_update_callback(mrp_wayland_t *wl,
                       "(%s)", winmgr->name, ret.string ? ret.string : "NULL");
         mrp_free((void *)ret.string);
     }
+
+    lua_settop(L, top);
 }
 
 

diff --git a/src/plugins/system-controller/wayland/scripting-window.c b/src/plugins/system-controller/wayland/scripting-window.c
index 5374db4..2231871 100644 (file)
--- a/src/plugins/system-controller/wayland/scripting-window.c
+++ b/src/plugins/system-controller/wayland/scripting-window.c
@@ -124,7 +124,7 @@ MRP_LUA_CLASS_DEF_SIMPLE (
     )
 );
 
-MRP_LUA_CLASS_DEF_SIMPLE (
+MRP_LUA_CLASS_DEF_SIMPLE_FLAGS (
     window_mask,                /* class name */
     scripting_window_mask_t,    /* userdata type */
     window_mask_destroy,        /* userdata destructor */
@@ -137,7 +137,8 @@ MRP_LUA_CLASS_DEF_SIMPLE (
        MRP_LUA_OVERRIDE_GETFIELD  (window_mask_getfield)
        MRP_LUA_OVERRIDE_SETFIELD  (window_mask_setfield)
        MRP_LUA_OVERRIDE_STRINGIFY (window_mask_stringify)
-    )
+    ),
+    MRP_LUA_CLASS_DYNAMIC
 );
 
 MRP_LUA_CLASS_DEF_SIMPLE (