Merge pull request #2834 from sigprof/titlebar-widget-gc

Fix GC for titlebar widgets

lib/awful/titlebar.lua

@@ -13,6 +13,8 @@
 ---------------------------------------------------------------------------
 
 local error = error
+local pairs = pairs
+local table = table
 local type = type
 local gmath = require("gears.math")
 local abutton = require("awful.button")
@@ -601,6 +603,33 @@ function titlebar.toggle(c, position)
     end
 end
 
+local instances = {}
+
+-- Do the equivalent of
+--     c:connect_signal(signal, widget.update)
+-- without keeping a strong reference to the widget.
+local function update_on_signal(c, signal, widget)
+    local sig_instances = instances[signal]
+    if sig_instances == nil then
+        sig_instances = setmetatable({}, { __mode = "k" })
+        instances[signal] = sig_instances
+        capi.client.connect_signal(signal, function(cl)
+            local widgets = sig_instances[cl]
+            if widgets then
+                for _, w in pairs(widgets) do
+                    w.update()
+                end
+            end
+        end)
+    end
+    local widgets = sig_instances[c]
+    if widgets == nil then
+        widgets = setmetatable({}, { __mode = "v" })
+        sig_instances[c] = widgets
+    end
+    table.insert(widgets, widget)
+end
+
 --- Create a new titlewidget. A title widget displays the name of a client.
 -- Please note that this returns a textbox and all of textbox' API is available.
 -- This way, you can e.g. modify the font that is used.
@@ -612,7 +641,8 @@ function titlebar.widget.titlewidget(c)
     local function update()
         ret:set_text(c.name or titlebar.fallback_name)
     end
-    c:connect_signal("property::name", update)
+    ret.update = update
+    update_on_signal(c, "property::name", ret)
     update()
 
     return ret
@@ -717,8 +747,8 @@ function titlebar.widget.button(c, name, selector, action)
 
     -- We do magic based on whether a client is focused above, so we need to
     -- connect to the corresponding signal here.
-    c:connect_signal("focus", update)
+    update_on_signal(c, "focus", ret)
-    c:connect_signal("unfocus", update)
+    update_on_signal(c, "unfocus", ret)
 
     return ret
 end
@@ -728,7 +758,7 @@ end
 -- @staticfct awful.titlebar.widget.floatingbutton
 function titlebar.widget.floatingbutton(c)
     local widget = titlebar.widget.button(c, "floating", aclient.object.get_floating, aclient.floating.toggle)
-    c:connect_signal("property::floating", widget.update)
+    update_on_signal(c, "property::floating", widget)
     return widget
 end
 
@@ -741,7 +771,7 @@ function titlebar.widget.maximizedbutton(c)
     end, function(cl, state)
         cl.maximized = not state
     end)
-    c:connect_signal("property::maximized", widget.update)
+    update_on_signal(c, "property::maximized", widget)
     return widget
 end
 
@@ -752,7 +782,7 @@ function titlebar.widget.minimizebutton(c)
     local widget = titlebar.widget.button(c, "minimize",
                                           function() return "" end,
                                           function(cl) cl.minimized = not cl.minimized end)
-    c:connect_signal("property::minimized", widget.update)
+    update_on_signal(c, "property::minimized", widget)
     return widget
 end
 
@@ -770,7 +800,7 @@ function titlebar.widget.ontopbutton(c)
     local widget = titlebar.widget.button(c, "ontop",
                                           function(cl) return cl.ontop end,
                                           function(cl, state) cl.ontop = not state end)
-    c:connect_signal("property::ontop", widget.update)
+    update_on_signal(c, "property::ontop", widget)
     return widget
 end
 
@@ -781,7 +811,7 @@ function titlebar.widget.stickybutton(c)
     local widget = titlebar.widget.button(c, "sticky",
                                           function(cl) return cl.sticky end,
                                           function(cl, state) cl.sticky = not state end)
-    c:connect_signal("property::sticky", widget.update)
+    update_on_signal(c, "property::sticky", widget)
     return widget
 end
 

tests/test-leak-client.lua

@@ -4,8 +4,9 @@ local awful = require("awful")
 local wibox = require("wibox")
 local gtable = require("gears.table")
 
--- "Enable" titlebars (so that the titlebar can prevent garbage collection)
+-- Create a titlebar and return a table with references to its member widgets.
-client.connect_signal("manage", function (c)
+local function create_titlebar(c)
+    local parts = {}
     local buttons = gtable.join(
         awful.button({ }, 1, function()
             client.focus = c
@@ -20,25 +21,39 @@ client.connect_signal("manage", function (c)
     )
 
     -- Widgets that are aligned to the left
-    local left_layout = wibox.layout.fixed.horizontal(awful.titlebar.widget.iconwidget(c))
+    parts.icon = awful.titlebar.widget.iconwidget(c)
+    local left_layout = wibox.layout.fixed.horizontal(parts.icon)
     left_layout:buttons(buttons)
 
     -- The title goes in the middle
-    local title = awful.titlebar.widget.titlewidget(c)
+    parts.title = awful.titlebar.widget.titlewidget(c)
-    title:set_align("center")
+    parts.title:set_align("center")
-    local middle_layout = wibox.layout.flex.horizontal(title)
+    local middle_layout = wibox.layout.flex.horizontal(parts.title)
     middle_layout:buttons(buttons)
 
+    parts.floating_button  = awful.titlebar.widget.floatingbutton(c)
+    parts.maximized_button = awful.titlebar.widget.maximizedbutton(c)
+    parts.sticky_button    = awful.titlebar.widget.stickybutton(c)
+    parts.ontop_button     = awful.titlebar.widget.ontopbutton(c)
+    parts.close_button     = awful.titlebar.widget.closebutton(c)
+
     awful.titlebar(c):set_widget(wibox.layout.align.horizontal(
         left_layout,
         middle_layout,
         wibox.layout.fixed.horizontal(
-            awful.titlebar.widget.floatingbutton(c),
+            parts.floating_button,
-            awful.titlebar.widget.maximizedbutton(c),
+            parts.maximized_button,
-            awful.titlebar.widget.stickybutton(c),
+            parts.sticky_button,
-            awful.titlebar.widget.ontopbutton(c),
+            parts.ontop_button,
-            awful.titlebar.widget.closebutton(c)
+            parts.close_button
             )), { position = "bottom"})
+
+    return parts
+end
+
+-- "Enable" titlebars (so that the titlebar can prevent garbage collection)
+client.connect_signal("manage", function (c)
+    create_titlebar(c)
 end)
 
 -- We tell the garbage collector when to work, disable it
@@ -49,6 +64,7 @@ collectgarbage("stop")
 -- closes it and the last one checks that the client object is GC'able.
 local objs = nil
 local second_call = false
+local state
 local steps = {
     function(count)
         if count == 1 then
@@ -78,6 +94,55 @@ local steps = {
             return true
         end
     end,
+
+    -- Test that titlebar widgets are GC'able even when the corresponding client
+    -- still exists.
+    function(count)
+        if count == 1 then
+            awful.spawn("xterm")
+            state = 1
+        elseif state == 1 then
+            local c = client.get()[1]
+            if c then
+                -- Create the titlebar and store references to its widgets in a
+                -- weak table.
+                objs = setmetatable({}, { __mode = "v" })
+                gtable.crush(objs, create_titlebar(c))
+                state = 2
+            end
+        elseif state == 2 then
+            local c = client.get()[1]
+
+            -- Recreate the titlebar; at this point old titlebar widgets should
+            -- become unreferenced, except for some delayed calls.
+            create_titlebar(c)
+
+            -- Wait for one iteration so that delayed calls would complete and
+            -- release remaining references to widgets.
+            state = 3
+        elseif state == 3 then
+            -- Check that old titlebar widgets have been destroyed.
+            for _ = 1, 10 do
+                collectgarbage("collect")
+            end
+            local num_objs = 0
+            for k in pairs(objs) do
+                if num_objs == 0 then
+                    print("Error: titlebar widgets remaining after GC:")
+                end
+                num_objs = num_objs + 1
+                print(k)
+            end
+            assert(num_objs == 0)
+
+            client.get()[1]:kill()
+            state = 4
+        elseif state == 4 then
+            if #client.get() == 0 then
+                return true
+            end
+        end
+    end,
 }
 
 runner.run_steps(steps)