Merge #1279 #1282

1279: Improve XCBConnection atom lookup r=AlanGriffiths a=wmww Fixes a potential thread safety issue in the resolving of `XCBConnection::Atom` and starts caching atom names for faster lookup. 1282: [Wayland] Don't destroy objects we don't own r=wmww a=AlanGriffiths Don't destroy objects we don't own. (Fixes #1276) Co-authored-by: William Wold <[email protected]> Co-authored-by: Alan Griffiths <[email protected]>
canonical · Feb 14, 2020 · e3872e1 · e3872e1
3 parents 7d84c44 + dadbd8f + 68361a3
commit e3872e1
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 23 deletions.
diff --git a/src/server/frontend_wayland/wayland_connector.cpp b/src/server/frontend_wayland/wayland_connector.cpp
@@ -372,7 +372,7 @@ class WlShellSurface : public wayland::ShellSurface, public WindowWlSurfaceRole
 
     void handle_close_request() override
     {
-        destroy_wayland_object();
+        // It seems there is no way to request close of a wl_shell_surface
     }
 
     void set_fullscreen(

diff --git a/src/server/frontend_xwayland/xcb_connection.cpp b/src/server/frontend_xwayland/xcb_connection.cpp
@@ -56,20 +56,31 @@ mf::XCBConnection::Atom::Atom(std::string const& name, XCBConnection* connection
 
 mf::XCBConnection::Atom::operator xcb_atom_t() const
 {
-    if (!atom)
+    if (atom == XCB_ATOM_NONE)
     {
-        auto const reply = xcb_intern_atom_reply(*connection, cookie, nullptr);
-        if (!reply)
-            BOOST_THROW_EXCEPTION(std::runtime_error("Failed to look up atom " + name_));
-        atom = reply->atom;
-        free(reply);
+        std::lock_guard<std::mutex> lock{mutex};
+
+        // The initial atomic check is faster, but we need to check again once we've got the lock
+        if (atom == XCB_ATOM_NONE)
+        {
+            auto const reply = xcb_intern_atom_reply(*connection, cookie, nullptr);
+            if (!reply)
+                BOOST_THROW_EXCEPTION(std::runtime_error("Failed to look up atom " + name_));
+            atom = reply->atom;
+            free(reply);
+
+            std::lock_guard<std::mutex> lock{connection->atom_name_cache_mutex};
+            connection->atom_name_cache[atom] = name_;
+        }
     }
-    return atom.value();
+
+    return atom;
 }
 
 mf::XCBConnection::XCBConnection(int fd)
     : xcb_connection{xcb_connect_to_fd(fd, nullptr)},
       xcb_screen{xcb_setup_roots_iterator(xcb_get_setup(xcb_connection)).data},
+      atom_name_cache{{XCB_ATOM_NONE, "None/Any"}},
       wm_protocols{"WM_PROTOCOLS", this},
       wm_take_focus{"WM_TAKE_FOCUS", this},
       wm_delete_window{"WM_DELETE_WINDOW", this},
@@ -136,28 +147,31 @@ auto mf::XCBConnection::root_window() const -> xcb_window_t
 
 auto mf::XCBConnection::query_name(xcb_atom_t atom) const -> std::string
 {
-    // TODO: cache, for cheaper lookup
+    std::lock_guard<std::mutex>{atom_name_cache_mutex};
+    auto const iter = atom_name_cache.find(atom);
 
-    if (atom == XCB_ATOM_NONE)
-        return "None";
+    if (iter == atom_name_cache.end())
+    {
+        xcb_get_atom_name_cookie_t const cookie = xcb_get_atom_name(xcb_connection, atom);
+        xcb_get_atom_name_reply_t* const reply = xcb_get_atom_name_reply(xcb_connection, cookie, nullptr);
 
-    xcb_get_atom_name_cookie_t const cookie = xcb_get_atom_name(xcb_connection, atom);
-    xcb_get_atom_name_reply_t* const reply = xcb_get_atom_name_reply(xcb_connection, cookie, nullptr);
+        std::string name;
 
-    std::string name;
+        if (reply)
+            name = std::string{xcb_get_atom_name_name(reply), static_cast<size_t>(xcb_get_atom_name_name_length(reply))};
+        else
+            name = "Atom " + std::to_string(atom);
 
-    if (reply)
-    {
-        name = std::string{xcb_get_atom_name_name(reply), static_cast<size_t>(xcb_get_atom_name_name_length(reply))};
+        free(reply);
+
+        atom_name_cache[atom] = name;
+
+        return name;
     }
     else
     {
-        name = "Atom " + std::to_string(atom);
+        return iter->second;
     }
-
-    free(reply);
-
-    return name;
 }
 
 auto mf::XCBConnection::reply_contains_string_data(xcb_get_property_reply_t const* reply) const -> bool

diff --git a/src/server/frontend_xwayland/xcb_connection.h b/src/server/frontend_xwayland/xcb_connection.h
@@ -26,7 +26,10 @@
 #include <xcb/xcb.h>
 #include <string>
 #include <vector>
+#include <unordered_map>
 #include <functional>
+#include <mutex>
+#include <atomic>
 #include <experimental/optional>
 
 namespace mir
@@ -60,6 +63,9 @@ class XCBConnection
     xcb_connection_t* const xcb_connection;
     xcb_screen_t* const xcb_screen;
 
+    std::mutex mutable atom_name_cache_mutex;
+    std::unordered_map<xcb_atom_t, std::string> mutable atom_name_cache;
+
 public:
     class Atom
     {
@@ -76,7 +82,11 @@ class XCBConnection
         XCBConnection* const connection;
         std::string const name_;
         xcb_intern_atom_cookie_t const cookie;
-        std::experimental::optional<xcb_atom_t> mutable atom;
+        std::mutex mutable mutex;
+
+        /// Accessed locklessly, but only set under lock
+        /// Once set to a value other than XCB_ATOM_NONE, not changed again
+        std::atomic<xcb_atom_t> mutable atom{XCB_ATOM_NONE};
     };
 
     explicit XCBConnection(int fd);