From 750a1df1c725d0886868e4c9b72fc4ea4a39bb26 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Fri, 23 Sep 2016 09:51:21 +0200 Subject: [PATCH 1/9] Re-set wallpaper on screen's property::geometry Imagine that you have two screens of different resolution and you change their position (xrandr --output first --left-of second). Of course, the wallpaper has to be updated afterwards. This commit makes the default config do that. Fixes: https://github.com/awesomeWM/awesome/issues/1102 Signed-off-by: Uli Schlachter --- awesomerc.lua | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/awesomerc.lua b/awesomerc.lua index 11dcc93ea..7edb28278 100755 --- a/awesomerc.lua +++ b/awesomerc.lua @@ -165,8 +165,7 @@ mytasklist.buttons = awful.util.table.join( awful.client.focus.byidx(-1) end)) -awful.screen.connect_for_each_screen(function(s) - -- Wallpaper +local function set_wallpaper(s) if beautiful.wallpaper then local wallpaper = beautiful.wallpaper -- If wallpaper is a function, call it with the screen @@ -175,6 +174,14 @@ awful.screen.connect_for_each_screen(function(s) end gears.wallpaper.maximized(wallpaper, s, true) end +end + +-- Re-set wallpaper when a screen's geometry changes (e.g. different resolution) +screen.connect_signal("property::geometry", set_wallpaper) + +awful.screen.connect_for_each_screen(function(s) + -- Wallpaper + set_wallpaper(s) -- Each screen has its own tag table. awful.tag({ "1", "2", "3", "4", "5", "6", "7", "8", "9" }, s, awful.layout.layouts[1]) From 803264a488fa3f717b2f67616ff6d49f297ecfaf Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Fri, 23 Sep 2016 09:52:46 +0200 Subject: [PATCH 2/9] Fix magnifier layout when focus is on another screen The magnifier layout wants to ignore floating clients. Before 82342f0 this was done by calling awful.client.floating.get(focus). If "focus" was nil, this might have checked the floating status of a wrong client (if some other client was focused, and the code in magnifier set focus=nil before). This issue can easily be missed and might exist since forever. After 82342f, floating status is checked via "focus.floating" and this now causes an "attempt to index nil value" error instead. Much easier to notice. Fix this by adding the missing nil check and while touching the code, merge this with the previous "if" and correct another error (the wrong thing happened if we had #cls=0). Fixes: https://github.com/awesomeWM/awesome/issues/1103 Signed-off-by: Uli Schlachter --- lib/awful/layout/suit/magnifier.lua | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/lib/awful/layout/suit/magnifier.lua b/lib/awful/layout/suit/magnifier.lua index 089bbdd08..02226658b 100644 --- a/lib/awful/layout/suit/magnifier.lua +++ b/lib/awful/layout/suit/magnifier.lua @@ -62,13 +62,8 @@ function magnifier.arrange(p) -- Check that the focused window is on the right screen if focus and focus.screen ~= p.screen then focus = nil end - if not focus and #cls > 0 then - focus = cls[1] - fidx = 1 - end - - -- If focused window is not tiled, take the first one which is tiled. - if focus.floating then + -- If no window is focused or focused window is not tiled, take the first tiled one. + if (not focus or focus.floating) and #cls > 0 then focus = cls[1] fidx = 1 end From 8d2dde3a346f84f19b55a2bfbafe1bdd7fa68ff9 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Fri, 23 Sep 2016 09:56:45 +0200 Subject: [PATCH 3/9] wibox: Remove some dead code widget_at() no longer exists since 0aa4304bda (and the surrounding commits stopped us using this function). Signed-off-by: Uli Schlachter --- lib/wibox/drawable.lua | 3 --- lib/wibox/init.lua | 3 --- 2 files changed, 6 deletions(-) diff --git a/lib/wibox/drawable.lua b/lib/wibox/drawable.lua index 0212efae9..ad349368a 100644 --- a/lib/wibox/drawable.lua +++ b/lib/wibox/drawable.lua @@ -46,9 +46,6 @@ local function get_widget_context(self) screen = s, dpi = dpi, drawable = self, - widget_at = function(_, ...) - self:widget_at(...) - end } for k, v in pairs(self._widget_context_skeleton) do context[k] = v diff --git a/lib/wibox/init.lua b/lib/wibox/init.lua index 7d3735781..a078534e9 100644 --- a/lib/wibox/init.lua +++ b/lib/wibox/init.lua @@ -177,9 +177,6 @@ local function new(args) setup_signals(ret) ret.draw = ret._drawable.draw - ret.widget_at = function(_, widget, x, y, width, height) - return ret._drawable:widget_at(widget, x, y, width, height) - end -- Set the default background ret:set_bg(args.bg or beautiful.bg_normal) From e0a3ecba01fb49d7c3091c839f10c05b687f0664 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Fri, 23 Sep 2016 10:00:28 +0200 Subject: [PATCH 4/9] wibox.hierarchy: Update when the context changes When the context for widget changes (e.g. we are on a new different screen or have a different DPI value), widgets might change their appearance even though they didn't emit widget::layout_changed. Thus, update the hierarchy in these cases. Signed-off-by: Uli Schlachter --- lib/wibox/hierarchy.lua | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/wibox/hierarchy.lua b/lib/wibox/hierarchy.lua index b21885055..ffdc9924a 100644 --- a/lib/wibox/hierarchy.lua +++ b/lib/wibox/hierarchy.lua @@ -23,6 +23,7 @@ local function hierarchy_new(redraw_callback, layout_callback, callback_arg) _matrix_to_device = matrix.identity, _need_update = true, _widget = nil, + _context = nil, _redraw_callback = redraw_callback, _layout_callback = layout_callback, _callback_arg = callback_arg, @@ -73,6 +74,7 @@ end local hierarchy_update function hierarchy_update(self, context, widget, width, height, region, matrix_to_parent, matrix_to_device) if (not self._need_update) and self._widget == widget and + self._context == context and self._size.width == width and self._size.height == height and matrix.equals(self._matrix, matrix_to_parent) and matrix.equals(self._matrix_to_device, matrix_to_device) then @@ -101,6 +103,7 @@ function hierarchy_update(self, context, widget, width, height, region, matrix_t -- Save the arguments we need to save self._widget = widget + self._context = context self._size.width = width self._size.height = height self._matrix = matrix_to_parent From 752d49ed47d1ab7e44272a31357d9eed9e52af82 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Fri, 23 Sep 2016 10:03:12 +0200 Subject: [PATCH 5/9] wibox.drawable: Force full repaint when the context changes The previous commit made the hierarchy do a re-layout when the context changes. However, widgets could change their appearance depending on the context without changing their layout. Thus, the previous commit is not enough. This commit also makes the drawable redraw everything when the context changes. Signed-off-by: Uli Schlachter --- lib/wibox/drawable.lua | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/wibox/drawable.lua b/lib/wibox/drawable.lua index ad349368a..a7fd0efda 100644 --- a/lib/wibox/drawable.lua +++ b/lib/wibox/drawable.lua @@ -51,6 +51,9 @@ local function get_widget_context(self) context[k] = v end self._widget_context = context + + -- Give widgets a chance to react to the new context + self._need_complete_repaint = true end return context end From f6761e662c7e4168959957ccc5f4a7d723e6d76a Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Fri, 23 Sep 2016 10:07:29 +0200 Subject: [PATCH 6/9] wibox.drawable: React to screen changes The previous commit made wibox.drawable turn a "normal redraw" into a complete repaint when it was moved to another screen. However, nothing happened until that normal redraw. This commit triggers a normal redraw when we are (possibly) moved to another screen. More precise, this means that whenever a screen appears, disappears or changes its geometry and when the drawable is moved, we trigger a normal redraw. This redraw will likely do nothing, because no relayout is pending and no part of the surface needs a redraw, so it is cheap. However, if the drawable really ends up on another screen, then the code from the previous commits makes us do a full relayout and redraw. This commit likely fixes the current instability of test-screen-changes.lua. See https://github.com/awesomeWM/awesome/issues/982#issuecomment-231712056. As explained there, the test fails because the fake screen that it created is still referenced, so cannot be garbage collected, but the test doesn't succeed unless the screen is garbage collected. So something is still referencing the screen that was removed. This something can be a client's titlebar, because the underlying drawable still has a context member referring to the old screen. This commit should fix that problem, because we now trigger a redraw which will compute a new context and thus the reference to the old screen is released. Signed-off-by: Uli Schlachter --- lib/wibox/drawable.lua | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/lib/wibox/drawable.lua b/lib/wibox/drawable.lua index a7fd0efda..ef4953b36 100644 --- a/lib/wibox/drawable.lua +++ b/lib/wibox/drawable.lua @@ -24,7 +24,8 @@ local matrix = require("gears.matrix") local hierarchy = require("wibox.hierarchy") local unpack = unpack or table.unpack -- luacheck: globals unpack (compatibility with Lua 5.1) -local drawables = setmetatable({}, { __mode = 'k' }) +local drawables_draw = setmetatable({}, { __mode = 'k' }) +local drawables_force_complete_repaint = setmetatable({}, { __mode = 'k' }) -- Get the widget context. This should always return the same table (if -- possible), so that our draw and fit caches can work efficiently. @@ -357,9 +358,18 @@ function drawable.new(d, widget_context_skeleton, drawable_name) ret._need_complete_repaint = true ret:draw() end - drawables[ret._do_complete_repaint] = true + drawables_draw[ret.draw] = true + drawables_force_complete_repaint[ret._do_complete_repaint] = true + + -- Do a full redraw if the surface changes (the new surface has no content yet) d:connect_signal("property::surface", ret._do_complete_repaint) + -- Do a normal redraw when the drawable moves. This will likely do nothing + -- in most cases, but it makes us do a complete repaint when we are moved to + -- a different screen. + d:connect_signal("property::x", ret.draw) + d:connect_signal("property::y", ret.draw) + -- Currently we aren't redrawing on move (signals not connected). -- :set_bg() will later recompute this. ret._redraw_on_move = false @@ -427,11 +437,21 @@ end -- Redraw all drawables when the wallpaper changes capi.awesome.connect_signal("wallpaper_changed", function() - for k in pairs(drawables) do + for k in pairs(drawables_force_complete_repaint) do k() end end) +-- Give drawables a chance to react to screen changes +local function draw_all() + for k in pairs(drawables_draw) do + k() + end +end +screen.connect_signal("property::geometry", draw_all) +screen.connect_signal("added", draw_all) +screen.connect_signal("removed", draw_all) + return setmetatable(drawable, { __call = function(_, ...) return drawable.new(...) end }) -- vim: filetype=lua:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:textwidth=80 From 489aa4dc24847d7e2983382ea68d7a80ed003c6d Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Fri, 23 Sep 2016 10:15:48 +0200 Subject: [PATCH 7/9] Improve behaviour of GC'd objects Before this commit: When we are GC'ing an object, we clear its metatable, since otherwise crashes could occur in various places. This means that if someone tries to use such an object, they get an unhelpful error message like "attempt to index userdata object" and they don't understand what the problem is. Also, this means that foo.valid does not actually work after GC. This commit changes this behaviour. Instead of setting an empty metatable, we now create a metatable with an __index and __newindex method. These metamethods produce better error messages that they sat the underlying object was already garbage collected. Better yet, the __index metamethod makes foo.valid be false instead of causing an error, so that the existing machinery for detecting invalid objects continues to work. This commit also adds a functional test that verifies this behaviour. Signed-off-by: Uli Schlachter --- common/luaclass.c | 31 +++++++++++++++++++++++++++++++ tests/test-use-after-gc.lua | 31 +++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 tests/test-use-after-gc.lua diff --git a/common/luaclass.c b/common/luaclass.c index 1771aaa2f..2f8f2b9cf 100644 --- a/common/luaclass.c +++ b/common/luaclass.c @@ -163,6 +163,32 @@ luaA_class_add_property(lua_class_t *lua_class, }); } +/** Newindex meta function for objects after they were GC'd. + * \param L The Lua VM state. + * \return The number of elements pushed on stack. + */ +static int +luaA_class_newindex_invalid(lua_State *L) +{ + return luaL_error(L, "attempt to index an object that was already garbage collected"); +} + +/** Index meta function for objects after they were GC'd. + * \param L The Lua VM state. + * \return The number of elements pushed on stack. + */ +static int +luaA_class_index_invalid(lua_State *L) +{ + const char *attr = luaL_checkstring(L, 2); + if (A_STREQ(attr, "valid")) + { + lua_pushboolean(L, false); + return 1; + } + return luaA_class_newindex_invalid(L); +} + /** Garbage collect a Lua object. * \param L The Lua VM state. * \return The number of elements pushed on stack. @@ -181,8 +207,13 @@ luaA_class_gc(lua_State *L) class->collector(item); /* Unset its metatable so that e.g. luaA_toudata() will no longer accept * this object. This is needed since other __gc methods can still use this. + * We also make sure that `item.valid == false`. */ lua_newtable(L); + lua_pushcfunction(L, luaA_class_index_invalid); + lua_setfield(L, -2, "__index"); + lua_pushcfunction(L, luaA_class_newindex_invalid); + lua_setfield(L, -2, "__newindex"); lua_setmetatable(L, 1); return 0; } diff --git a/tests/test-use-after-gc.lua b/tests/test-use-after-gc.lua new file mode 100644 index 000000000..29220568d --- /dev/null +++ b/tests/test-use-after-gc.lua @@ -0,0 +1,31 @@ +-- Test behaviour of C objects after they were finalized + +local runner = require("_runner") + +local done +do + local obj + local func = function() + assert(obj.valid == false) + assert(not pcall(function() + print(obj.visible) + end)) + assert(not pcall(function() + obj.visible = true + end)) + done = true + end + if _VERSION >= "Lua 5.2" then + setmetatable({}, { __gc = func }) + else + local newproxy = newproxy -- luacheck: globals newproxy + getmetatable(newproxy(true)).__gc = func + end + obj = drawin({}) +end +collectgarbage("collect") +assert(done) + +runner.run_steps({ function() return true end }) + +-- vim: filetype=lua:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:textwidth=80 From ab135fa7c9bc21b87aec5c2e83dfd56bebb3761e Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Fri, 23 Sep 2016 10:18:49 +0200 Subject: [PATCH 8/9] wibox.drawable: Don't redraw invalid drawables Twice now we had problems with the garbage collector which caused signals established via weak_connect_signal() not to be disconnected when we wanted them to be disconnected. The effect was that we tried to redraw a drawable after it was garbage collected which caused errors. Instead of playing whack-a-mole with all the various ways that might make us redraw a drawable after GC, let's just fix all of these issues by explicitly checking for this case and turning it into a no-op. Signed-off-by: Uli Schlachter --- lib/wibox/drawable.lua | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/wibox/drawable.lua b/lib/wibox/drawable.lua index ef4953b36..e7df58217 100644 --- a/lib/wibox/drawable.lua +++ b/lib/wibox/drawable.lua @@ -60,7 +60,7 @@ local function get_widget_context(self) end local function do_redraw(self) - if type(self.drawable) ~= "drawable" then return end --FIXME See #1070 + if not self.drawable.valid then return end local surf = surface.load_silently(self.drawable.surface, false) -- The surface can be nil if the drawable's parent was already finalized @@ -400,6 +400,15 @@ function drawable.new(d, widget_context_skeleton, drawable_name) -- Set up our callbacks for repaints ret._redraw_callback = function(hierar, arg) + -- XXX: lgi will lead us into memory-corruption-land when we use an + -- object after it was GC'd. Try to detect this situation by checking if + -- the drawable is still valid. This is only a weak indication, but it + -- seems to be the best that we can do. The problem is that the drawable + -- could not yet be GC'd, but is pending finalisation, while the + -- cairo.Region below was already GC'd. This would still lead to corruption. + if not ret.drawable.valid then + return + end if ret._widget_hierarchy_callback_arg ~= arg then return end From 93ed7fd46f81fa5017877f534180e8ca2ca85f5d Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Fri, 23 Sep 2016 10:51:24 +0200 Subject: [PATCH 9/9] client_geometry_refresh(): Ignore enter/leave events less This commit makes the function only call client_ignore_enterleave_events() when it actually has to. Since we expect that most of the time, no client's geometry is changed, this means that most of the time this function is not called. Fixes: https://github.com/awesomeWM/awesome/issues/1107 Signed-off-by: Uli Schlachter --- objects/client.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/objects/client.c b/objects/client.c index e561aae2b..bdb372662 100644 --- a/objects/client.c +++ b/objects/client.c @@ -1138,7 +1138,7 @@ client_border_refresh(void) static void client_geometry_refresh(void) { - client_ignore_enterleave_events(); + bool ignored_enterleave = false; foreach(_c, globalconf.clients) { client_t *c = *_c; @@ -1164,6 +1164,11 @@ client_geometry_refresh(void) && AREA_EQUAL(real_geometry, c->x11_client_geometry)) continue; + if (!ignored_enterleave) { + client_ignore_enterleave_events(); + ignored_enterleave = true; + } + xcb_configure_window(globalconf.connection, c->frame_window, XCB_CONFIG_WINDOW_X | XCB_CONFIG_WINDOW_Y | XCB_CONFIG_WINDOW_WIDTH | XCB_CONFIG_WINDOW_HEIGHT, (uint32_t[]) { geometry.x, geometry.y, geometry.width, geometry.height }); @@ -1177,7 +1182,8 @@ client_geometry_refresh(void) /* ICCCM 4.2.3 says something else, but Java always needs this... */ client_send_configure(c); } - client_restore_enterleave_events(); + if (ignored_enterleave) + client_restore_enterleave_events(); } void