Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Commit bd47edb4e is (possibly) wrong #2212

Open
psychon opened this issue Mar 9, 2018 · 0 comments
Open

Commit bd47edb4e is (possibly) wrong #2212

psychon opened this issue Mar 9, 2018 · 0 comments

Comments

@psychon
Copy link
Member

psychon commented Mar 9, 2018

Commit bd47edb changed the widget_dependencies table in wibox.widget.base to have __mode = "kv". However, the v in there is wrong: This table will always have the only reference to its values. Its a list of widgets specific to this entry!
The effect of this should be that the fit and layout caches are not properly invalidated if an "inner" widget changes after a garbage collection cycle...

However, these are fit and layout caches. This means that they also get cleared by the garbage collector. Since all uses of these caches first call record_dependency, which in turn touches the existing widget_dependencies[child] table, I think the garbage collector cannot collect the dependencies table without also collecting the caches that are protected by this.

Another however: I am not really sure about the above and depending on special/magical GC behaviour like this just asks for trouble. Something like the following should be given some thought:

diff --git a/lib/wibox/widget/base.lua b/lib/wibox/widget/base.lua
index 80853c72..ae48bcfe 100644
--- a/lib/wibox/widget/base.lua
+++ b/lib/wibox/widget/base.lua
@@ -210,7 +210,7 @@ end
 -- {{{ Caches
 
 -- Indexes are widgets, allow them to be garbage-collected.
-local widget_dependencies = setmetatable({}, { __mode = "kv" })
+local widget_dependencies = setmetatable({}, { __mode = "k" })
 
 -- Get the cache of the given kind for this widget. This returns a gears.cache
 -- that calls the callback of kind `kind` on the widget.
@@ -239,7 +239,7 @@ local function record_dependency(parent, child)
     base.check_widget(parent)
     base.check_widget(child)
 
-    local deps = widget_dependencies[child] or {}
+    local deps = widget_dependencies[child] or setmetatable({}, {__mode="v"})
     deps[parent] = true
     widget_dependencies[child] = deps
 end
@@ -248,7 +248,7 @@ end
 local clear_caches
 function clear_caches(widget)
     local deps = widget_dependencies[widget] or {}
-    widget_dependencies[widget] = {}
+    widget_dependencies[widget] = setmetatable({}, {__mode="v"})
     widget._private.widget_caches = {}
     for w in pairs(deps) do
         clear_caches(w)

(To do: Instead of recreating the metatable all the time, it should only be created once; also, if record_dependency unconditionally set the metatable, then clear_caches could just be left alone.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant