Fix a drawin crash, a drawin memory leak and a wallpaper error.

lib/wibox/drawable.lua

@@ -55,6 +55,10 @@ local function get_widget_context(self)
         for k, v in pairs(self._widget_context_skeleton) do
             context[k] = v
         end
+
+        -- Set the metatable to give access to the weak wibox.
+        setmetatable(context, getmetatable(self._widget_context_skeleton))
+
         self._widget_context = context
 
         -- Give widgets a chance to react to the new context
@@ -121,7 +125,7 @@ local function do_redraw(self)
     -- Draw the background
     cr:save()
 
-    if not capi.awesome.composite_manager_running then
+    if (not capi.awesome.composite_manager_running) and capi.root.wallpaper then
         -- This is pseudo-transparency: We draw the wallpaper in the background
         local wallpaper = surface.load_silently(capi.root.wallpaper(), false)
         cr.operator = cairo.Operator.SOURCE

lib/wibox/init.lua

@@ -254,7 +254,10 @@
     local function clone_signal(name)
         -- When "name" is emitted on wibox.drawin, also emit it on wibox
         obj:connect_signal(name, function(_, ...)
-            w:emit_signal(name, ...)
+            local wb = obj.get_wibox()
+            if wb then
+                wb:emit_signal(name, ...)
+            end
         end)
     end
 
@@ -296,16 +299,30 @@
     local ret = object()
     local w = capi.drawin(args)
 
+    local weak_wibox = setmetatable({ret}, {__mode = "v"})
+
+    -- Strong ref
     function w.get_wibox()
         return ret
     end
 
     ret.drawin = w
-    ret._drawable = wibox.drawable(w.drawable, { wibox = ret },
+
+    -- Do not pass a strong reference to the wibox, it will confuse the GC.
+    local context_skeleton = setmetatable({}, {
+        __index = function(_, key)
+            if key == "wibox" then return weak_wibox[1] end
+
+            return nil
+        end
+    })
+
+    ret._drawable = wibox.drawable(w.drawable, context_skeleton,
         "wibox drawable (" .. object.modulename(3) .. ")")
 
+    -- Weak ref
     function ret._drawable.get_wibox()
-        return ret
+        return weak_wibox[1]
     end
 
     ret._drawable:_inform_visible(w.visible)
@@ -328,13 +345,7 @@
     ret:set_fg(args.fg or beautiful.fg_normal)
 
     -- Add __tostring method to metatable.
-    local mt = {}
     local orig_string = tostring(ret)
-    mt.__tostring = function()
-        return string.format("wibox: %s (%s)",
-                             tostring(ret._drawable), orig_string)
-    end
-    ret = setmetatable(ret, mt)
 
     -- Make sure the wibox is drawn at least once
     ret.draw()
@@ -359,6 +370,10 @@
             else
                 rawset(self, k, v)
             end
+        end,
+        __tostring = function()
+            return string.format("wibox: %s (%s)",
+                                tostring(ret._drawable), orig_string)
         end
     })
 

objects/drawable.c

@@ -193,6 +193,12 @@ static int
 luaA_drawable_refresh(lua_State *L)
 {
     drawable_t *drawable = luaA_checkudata(L, 1, &drawable_class);
+
+    /* GC race condition where the drawin is gone, but the drawable as a
+     * refresh in a delayed call */
+    if (!drawable->refresh_callback)
+        return 0;
+
     drawable->refreshed = true;
     (*drawable->refresh_callback)(drawable->refresh_data);
 
@@ -240,6 +246,12 @@ drawable_class_setup(lua_State *L)
                             NULL);
 }
 
+void drawable_invalidate(drawable_t *d)
+{
+    d->refresh_callback = NULL;
+    d->refresh_data = NULL;
+}
+
 /* @DOC_cobject_COMMON@ */
 
 // vim: filetype=c:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:textwidth=80

objects/drawable.h

@@ -50,6 +50,7 @@ typedef struct drawable_t drawable_t;
 drawable_t *drawable_allocator(lua_State *, drawable_refresh_callback *, void *);
 void drawable_set_geometry(lua_State *, int, area_t);
 void drawable_class_setup(lua_State *);
+void drawable_invalidate(drawable_t *d);
 
 #endif
 // vim: filetype=c:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:textwidth=80

objects/drawin.c

@@ -187,6 +187,7 @@ drawin_wipe(drawin_t *w)
         w->window = XCB_NONE;
     }
     /* No unref needed because we are being garbage collected */
+    drawable_invalidate(w->drawable);
     w->drawable = NULL;
 }
 

tests/test-leaks-wibox.lua

@@ -0,0 +1,177 @@
+-- Ensure the wibox and internal `drawin` objects don't leak.
+local runner = require("_runner")
+-- local wibox = require("wibox")
+
+local steps = {}
+
+-- local wiboxes   = setmetatable({}, {__mode = "v"})
+local drawins   = setmetatable({}, {__mode = "v"})
+local keys      = setmetatable({}, {__mode = "v"})
+local drawables = {}
+
+-- Establish a baseline that it should work using the `key` objects.
+table.insert(steps, function()
+    for _=1, 5 do
+        table.insert(keys, key { key="#1" })
+    end
+
+    return true
+end)
+
+table.insert(steps, function()
+    for _=1, 5 do
+        collectgarbage("collect")
+    end
+
+    if #keys > 0 then return end
+
+    return true
+end)
+
+-- Add some invisible wiboxes.
+--
+-- Since they are not visible, they have zero internal references and should be
+-- GCed without any issue.
+table.insert(steps, function()
+    for _=1, 5 do
+        local d = drawin {
+            visible = false,
+            x       = 0,
+            y       = 0,
+            width   = 100,
+            height  = 100,
+        }
+
+        for _=1, 5 do
+            d.visible = true
+            d.visible = false
+        end
+
+        table.insert(drawins, d)
+        table.insert(drawables, d.drawable)
+    end
+
+    return true
+end)
+
+table.insert(steps, function()
+    for _, d in ipairs(drawins) do
+        d.visible = false
+    end
+
+    return true
+end)
+
+table.insert(steps, function()
+    for _=1, 5 do
+        collectgarbage("collect")
+    end
+
+    if #drawins > 0 then return end
+
+    -- Check if this is a no-op when the drawin is wiped.
+    for _, d in ipairs(drawables) do
+        d:refresh()
+    end
+
+    setmetatable(drawables, {__mode = "v"})
+
+    for _=1, 5 do
+        collectgarbage("collect")
+    end
+
+    return true
+end)
+
+table.insert(steps, function()
+    if #drawables > 0 then return end
+
+    return true
+end)
+
+-- This is flacky and I don't know why. `visible = true` removes the ref from
+-- `LUAA_OBJECT_REGISTRY_KEY`, but the GC doesn't seem to agree with that.
+
+-- Try again, but make the wibox visible this time.
+--
+-- This creates an internal reference and the weak table should hold them.
+-- table.insert(steps, function()
+--     for i=1, 5 do
+--         local d = drawin {
+--             visible = true,
+--             x       = 0,
+--             y       = 0,
+--             width   = 100,
+--             height  = 100,
+--         }
+--         table.insert(drawins, d)
+--     end
+--
+--     for _=1, 5 do
+--         collectgarbage("collect")
+--     end
+--
+--     assert(#drawins == 5)
+--
+--     return true
+-- end)
+--
+-- table.insert(steps, function()
+--     for _, d in ipairs(drawins) do
+--         -- This calls `unref`.
+--         d.visible = false
+--     end
+--
+--     return true
+-- end)
+--
+-- table.insert(steps, function()
+--     for _=1, 5 do
+--         collectgarbage("collect")
+--     end
+--
+--     while #drawins > 0 do return end
+--
+--     return true
+-- end)
+
+-- table.insert(steps, function()
+--     wibox = require("wibox")
+--
+--     for i=1, 5 do
+--         local w = wibox {
+--             visible = true,
+--             x       = 0,
+--             y       = 0,
+--             width   = 100,
+--             height  = 100,
+--         }
+--         table.insert(wiboxes, w)
+--         table.insert(drawins, w.drawin)
+--     end
+--
+--     return true
+-- end)
+--
+-- table.insert(steps, function()
+--     for _, w in ipairs(wiboxes) do
+--         w.visible = false
+--     end
+--
+--     return true
+-- end)
+--
+-- table.insert(steps, function()
+--     for _=1, 5 do
+--         collectgarbage("collect")
+--     end
+--
+--     while #wiboxes > 0 do return end
+--     while #drawins > 0 do return end
+--
+--     return true
+-- end)
+
+runner.run_steps(steps)
+
+-- vim: filetype=lua:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:textwidth=80