Reland "Add async X11 response handling"

This is a reland of c2dee48d

Original change's description:
> Add async X11 response handling
> 
> This CL adds x11::Future::OnResponse() to install a response handler
> for X11 requests.  Previously you could only Sync() to get the
> response.
> 
> ui::X11WorkspaceHandler is changed to demo usage of async response
> handling.  Eventually it should be migrated to a higher-level property
> handler, like we currently have in x11_util.* for Xlib.
> 
> R=sky
> CC=​msisov,nickdiego
> BUG=1066670
> 
> Change-Id: Ifcab2553008395bcfd9090e6aa1ccc5b4e53ae2e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2164245
> Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#763458}

Bug: 1066670
Change-Id: Ia1411c79d41a22e8747bc47a4e16616448f8ce0d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2173152
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Auto-Submit: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#764019}

chrome/browser/chrome_browser_main_extra_parts_x11.cc

@@ -14,6 +14,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "ui/base/x/x11_util.h"
 #include "ui/base/x/x11_util_internal.h"
+#include "ui/gfx/x/xproto_util.h"
 
 using content::BrowserThread;
 
@@ -29,7 +30,7 @@ const int kWaitForUIThreadSeconds = 10;
 int BrowserX11ErrorHandler(Display* d, XErrorEvent* error) {
   if (!g_in_x11_io_error_handler) {
     base::SequencedTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(&ui::LogErrorEventDescription, d, *error));
+        FROM_HERE, base::BindOnce(&x11::LogErrorEventDescription, *error));
   }
   return 0;
 }

ui/base/x/x11_util.cc

@@ -60,6 +60,7 @@
 #include "ui/gfx/x/x11.h"
 #include "ui/gfx/x/x11_atom_cache.h"
 #include "ui/gfx/x/x11_error_tracker.h"
+#include "ui/gfx/x/xproto_util.h"
 
 #if defined(OS_FREEBSD)
 #include <sys/sysctl.h>
@@ -93,7 +94,7 @@ int DefaultX11ErrorHandler(XDisplay* d, XErrorEvent* e) {
 
   if (base::MessageLoopCurrent::Get()) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(&LogErrorEventDescription, d, *e));
+        FROM_HERE, base::BindOnce(&x11::LogErrorEventDescription, *e));
   } else {
     LOG(ERROR)
         << "X error received: "
@@ -1506,49 +1507,6 @@ void SetX11ErrorHandlers(XErrorHandler error_handler,
       io_error_handler ? io_error_handler : DefaultX11IOErrorHandler);
 }
 
-void LogErrorEventDescription(XDisplay* dpy,
-                              const XErrorEvent& error_event) {
-  char error_str[256];
-  char request_str[256];
-
-  XGetErrorText(dpy, error_event.error_code, error_str, sizeof(error_str));
-
-  strncpy(request_str, "Unknown", sizeof(request_str));
-  if (error_event.request_code < 128) {
-    std::string num = base::NumberToString(error_event.request_code);
-    XGetErrorDatabaseText(
-        dpy, "XRequest", num.c_str(), "Unknown", request_str,
-        sizeof(request_str));
-  } else {
-    int num_ext;
-    gfx::XScopedPtr<char* [],
-                    gfx::XObjectDeleter<char*, int, XFreeExtensionList>>
-        ext_list(XListExtensions(dpy, &num_ext));
-
-    for (int i = 0; i < num_ext; i++) {
-      int ext_code, first_event, first_error;
-      XQueryExtension(dpy, ext_list[i], &ext_code, &first_event, &first_error);
-      if (error_event.request_code == ext_code) {
-        std::string msg = base::StringPrintf(
-            "%s.%d", ext_list[i], error_event.minor_code);
-        XGetErrorDatabaseText(
-            dpy, "XRequest", msg.c_str(), "Unknown", request_str,
-            sizeof(request_str));
-        break;
-      }
-    }
-  }
-
-  LOG(WARNING)
-      << "X error received: "
-      << "serial " << error_event.serial << ", "
-      << "error_code " << static_cast<int>(error_event.error_code)
-      << " (" << error_str << "), "
-      << "request_code " << static_cast<int>(error_event.request_code) << ", "
-      << "minor_code " << static_cast<int>(error_event.minor_code)
-      << " (" << request_str << ")";
-}
-
 // static
 XVisualManager* XVisualManager::GetInstance() {
   return base::Singleton<XVisualManager>::get();

ui/base/x/x11_workspace_handler.cc

@@ -8,9 +8,23 @@
 #include "ui/base/x/x11_util.h"
 #include "ui/events/x/x11_window_event_manager.h"
 #include "ui/gfx/x/x11_atom_cache.h"
+#include "ui/gfx/x/xproto.h"
 
 namespace ui {
 
+namespace {
+
+x11::Future<x11::XProto::GetPropertyReply> GetWorkspace(XDisplay* display) {
+  return x11::XProto{display}.GetProperty({
+      .window = XDefaultRootWindow(display),
+      .property = gfx::GetAtom("_NET_CURRENT_DESKTOP"),
+      .type = gfx::GetAtom("CARDINAL"),
+      .long_length = 1,
+  });
+}
+
+}  // namespace
+
 X11WorkspaceHandler::X11WorkspaceHandler(Delegate* delegate)
     : xdisplay_(gfx::GetXDisplay()),
       x_root_window_(DefaultRootWindow(xdisplay_)),
@@ -30,19 +44,10 @@ X11WorkspaceHandler::~X11WorkspaceHandler() {
 
 std::string X11WorkspaceHandler::GetCurrentWorkspace() {
   if (workspace_.empty())
-    UpdateWorkspace();
+    OnWorkspaceResponse(GetWorkspace(xdisplay_).Sync());
   return workspace_;
 }
 
-bool X11WorkspaceHandler::UpdateWorkspace() {
-  int desktop;
-  if (ui::GetCurrentDesktop(&desktop)) {
-    workspace_ = base::NumberToString(desktop);
-    return true;
-  }
-  return false;
-}
-
 bool X11WorkspaceHandler::DispatchXEvent(XEvent* event) {
   if (event->type != PropertyNotify ||
       event->xproperty.window != x_root_window_) {
@@ -51,8 +56,9 @@ bool X11WorkspaceHandler::DispatchXEvent(XEvent* event) {
   switch (event->type) {
     case PropertyNotify: {
       if (event->xproperty.atom == gfx::GetAtom("_NET_CURRENT_DESKTOP")) {
-        if (UpdateWorkspace())
-          delegate_->OnCurrentWorkspaceChanged(workspace_);
+        GetWorkspace(xdisplay_).OnResponse(
+            base::BindOnce(&X11WorkspaceHandler::OnWorkspaceResponse,
+                           weak_factory_.GetWeakPtr()));
       }
       break;
     }
@@ -62,4 +68,18 @@ bool X11WorkspaceHandler::DispatchXEvent(XEvent* event) {
   return false;
 }
 
+void X11WorkspaceHandler::OnWorkspaceResponse(
+    x11::XProto::GetPropertyResponse response) {
+  if (!response)
+    return;
+  DCHECK_EQ(response->bytes_after, 0U);
+  DCHECK_EQ(response->format, 32);
+  DCHECK_EQ(response->type, gfx::GetAtom("CARDINAL"));
+
+  uint32_t workspace;
+  memcpy(&workspace, response->value.data(), 4);
+  workspace_ = base::NumberToString(workspace);
+  delegate_->OnCurrentWorkspaceChanged(workspace_);
+}
+
 }  // namespace ui

ui/base/x/x11_workspace_handler.h

@@ -8,8 +8,10 @@
 #include <memory>
 
 #include "base/component_export.h"
+#include "base/memory/weak_ptr.h"
 #include "ui/events/platform/x11/x11_event_source.h"
 #include "ui/gfx/x/x11.h"
+#include "ui/gfx/x/xproto.h"
 
 namespace ui {
 
@@ -40,8 +42,7 @@ class COMPONENT_EXPORT(UI_BASE_X) X11WorkspaceHandler
   // ui::XEventDispatcher
   bool DispatchXEvent(XEvent* event) override;
 
-  // Makes a round trip to the X server to get the current workspace.
-  bool UpdateWorkspace();
+  void OnWorkspaceResponse(x11::XProto::GetPropertyResponse response);
 
   // The display and the native X window hosting the root window.
   XDisplay* xdisplay_;
@@ -55,6 +56,8 @@ class COMPONENT_EXPORT(UI_BASE_X) X11WorkspaceHandler
   std::string workspace_;
 
   Delegate* const delegate_;
+
+  base::WeakPtrFactory<X11WorkspaceHandler> weak_factory_{this};
 };
 
 }  // namespace ui

ui/events/BUILD.gn

@@ -166,6 +166,13 @@ jumbo_component("events_base") {
 
   if (use_x11 || ozone_platform_x11) {
     public_deps += [ "//ui/events/keycodes:x11" ]
+
+    # TODO(https://crbug.com/1076277): This dependency on //ui/gfx/x
+    # is unnecessary and should be removed.  It is included for now
+    # to work around a bug in lld that introduces unnecessary
+    # nondeterminism in the DSO (see c#6 on the bug for an
+    # explanation).
+    deps += [ "//ui/gfx/x" ]
   }
 
   if (!is_ios) {

ui/events/platform/x11/x11_event_source.cc

@@ -4,7 +4,14 @@
 
 #include "ui/events/platform/x11/x11_event_source.h"
 
+#include <X11/Xlib-xcb.h>
+#include <xcb/xcb.h>
+#include <xcb/xcbext.h>
+
+#include <type_traits>
+
 #include "base/logging.h"
+#include "base/memory/free_deleter.h"
 #include "base/metrics/histogram_macros.h"
 #include "ui/events/devices/x11/device_data_manager_x11.h"
 #include "ui/events/devices/x11/touch_factory_x11.h"
@@ -32,6 +39,27 @@ namespace ui {
 
 namespace {
 
+// On the wire, sequence IDs are 16 bits.  In xcb, they're usually extended to
+// 32 and sometimes 64 bits.  In Xlib, they're extended to unsigned long, which
+// may be 32 or 64 bits depending on the platform.  This function is intended to
+// prevent bugs caused by comparing two differently sized sequences.  Also
+// handles rollover.  To use, compare the result of this function with 0.  For
+// example, to compare seq1 <= seq2, use CompareSequenceIds(seq1, seq2) <= 0.
+template <typename T, typename U>
+auto CompareSequenceIds(T t, U u) {
+  static_assert(std::is_unsigned<T>::value, "");
+  static_assert(std::is_unsigned<U>::value, "");
+  // Cast to the smaller of the two types so that comparisons will always work.
+  // If we casted to the larger type, then the smaller type will be zero-padded
+  // and may incorrectly compare less than the other value.
+  using SmallerType =
+      typename std::conditional<sizeof(T) <= sizeof(U), T, U>::type;
+  SmallerType t0 = static_cast<SmallerType>(t);
+  SmallerType u0 = static_cast<SmallerType>(u);
+  using SignedType = typename std::make_signed<SmallerType>::type;
+  return static_cast<SignedType>(t0 - u0);
+}
+
 bool InitializeXkb(XDisplay* display) {
   if (!display)
     return false;
@@ -101,6 +129,18 @@ x11::Bool IsPropertyNotifyForTimestamp(Display* display,
 
 }  // namespace
 
+X11EventSource::Request::Request(bool is_void,
+                                 unsigned int sequence,
+                                 ResponseCallback callback)
+    : is_void(is_void), sequence(sequence), callback(std::move(callback)) {}
+
+X11EventSource::Request::Request(Request&& other)
+    : is_void(other.is_void),
+      sequence(other.sequence),
+      callback(std::move(other.callback)) {}
+
+X11EventSource::Request::~Request() = default;
+
 #if defined(USE_GLIB)
 using X11EventWatcherImpl = X11EventWatcherGlib;
 #else
@@ -148,14 +188,57 @@ X11EventSource* X11EventSource::GetInstance() {
 
 void X11EventSource::DispatchXEvents() {
   DCHECK(display_);
-  // Handle all pending events.
-  // It may be useful to eventually align this event dispatch with vsync, but
-  // not yet.
-  continue_stream_ = true;
-  while (XPending(display_) && continue_stream_) {
+
+  auto process_next_response = [&]() {
+    xcb_connection_t* connection = XGetXCBConnection(display_);
+    auto request = std::move(requests_.front());
+    requests_.pop();
+
+    void* raw_reply = nullptr;
+    xcb_generic_error_t* raw_error = nullptr;
+    xcb_poll_for_reply(connection, request.sequence, &raw_reply, &raw_error);
+    DCHECK(request.is_void || raw_reply || raw_error);
+
+    std::move(request.callback)
+        .Run(Reply{reinterpret_cast<uint8_t*>(raw_reply)}, Error{raw_error});
+  };
+
+  auto process_next_event = [&]() {
     XEvent xevent;
     XNextEvent(display_, &xevent);
     ExtractCookieDataDispatchEvent(&xevent);
+  };
+
+  // Handle all pending events.
+  continue_stream_ = true;
+  while (continue_stream_) {
+    bool has_next_response =
+        !requests_.empty() &&
+        CompareSequenceIds(XLastKnownRequestProcessed(display_),
+                           requests_.front().sequence) >= 0;
+    bool has_next_event = XPending(display_);
+
+    if (has_next_response && has_next_event) {
+      auto next_response_sequence = requests_.front().sequence;
+
+      XEvent event;
+      XPeekEvent(display_, &event);
+      auto next_event_sequence = event.xany.serial;
+
+      // All events have the sequence number of the last processed request
+      // included in them.  So if a reply and an event have the same sequence,
+      // the reply must have been received first.
+      if (CompareSequenceIds(next_event_sequence, next_response_sequence) <= 0)
+        process_next_response();
+      else
+        process_next_event();
+    } else if (has_next_response) {
+      process_next_response();
+    } else if (has_next_event) {
+      process_next_event();
+    } else {
+      break;
+    }
   }
 }
 
@@ -430,6 +513,15 @@ void X11EventSource::OnDispatcherListChanged() {
   }
 }
 
+void X11EventSource::AddRequest(bool is_void,
+                                unsigned int sequence,
+                                ResponseCallback callback) {
+  DCHECK(requests_.empty() ||
+         CompareSequenceIds(requests_.back().sequence, sequence) < 0);
+
+  requests_.emplace(is_void, sequence, std::move(callback));
+}
+
 // ScopedXEventDispatcher implementation
 ScopedXEventDispatcher::ScopedXEventDispatcher(
     XEventDispatcher** scoped_dispatcher,

ui/events/platform/x11/x11_event_source.h

@@ -8,13 +8,16 @@
 #include <stdint.h>
 
 #include <memory>
+#include <queue>
 #include <random>
 
 #include "base/auto_reset.h"
+#include "base/callback.h"
 #include "base/macros.h"
 #include "base/optional.h"
 #include "ui/events/events_export.h"
 #include "ui/events/platform/platform_event_source.h"
+#include "ui/gfx/x/request_queue.h"
 #include "ui/gfx/x/x11_types.h"
 
 using Time = unsigned long;
@@ -120,7 +123,8 @@ class EVENTS_EXPORT ScopedXEventDispatcher {
 // Receives X11 events from X11EventWatcher and sends them to registered
 // {Platform,X}EventDispatchers. Handles receiving, pre-process, translation
 // and post-processing of XEvents.
-class EVENTS_EXPORT X11EventSource : public PlatformEventSource {
+class EVENTS_EXPORT X11EventSource : public PlatformEventSource,
+                                     x11::RequestQueue {
  public:
   explicit X11EventSource(XDisplay* display);
   ~X11EventSource() override;
@@ -194,6 +198,16 @@ class EVENTS_EXPORT X11EventSource : public PlatformEventSource {
  private:
   friend class ScopedXEventDispatcher;
 
+  struct Request {
+    Request(bool is_void, unsigned int sequence, ResponseCallback callback);
+    Request(Request&& other);
+    ~Request();
+
+    const bool is_void;
+    const unsigned int sequence;
+    ResponseCallback callback;
+  };
+
   // Tells XEventDispatchers, which can also have PlatformEventDispatchers, that
   // a translated event is going to be sent next, then dispatches the event and
   // notifies XEventDispatchers the event has been sent out and, most probably,
@@ -207,6 +221,11 @@ class EVENTS_EXPORT X11EventSource : public PlatformEventSource {
   void StopCurrentEventStream() override;
   void OnDispatcherListChanged() override;
 
+  // x11::RequestQueue
+  void AddRequest(bool is_void,
+                  unsigned int sequence,
+                  ResponseCallback callback) override;
+
   void RestoreOverridenXEventDispatcher();
 
   static X11EventSource* instance_;
@@ -243,6 +262,8 @@ class EVENTS_EXPORT X11EventSource : public PlatformEventSource {
   XEventDispatcher* overridden_dispatcher_ = nullptr;
   bool overridden_dispatcher_restored_ = false;
 
+  std::queue<Request> requests_;
+
   DISALLOW_COPY_AND_ASSIGN(X11EventSource);
 };
 

ui/gfx/x/BUILD.gn

@@ -89,6 +89,10 @@ component("xprotos") {
   sources = get_target_outputs(":gen_xprotos") + [
               "xproto_internal.h",
               "xproto_types.h",
+              "request_queue.h",
+              "request_queue.cc",
+              "xproto_util.h",
+              "xproto_util.cc",
             ]
   configs += [
     ":x11_private_config",

ui/gfx/x/gen_xproto.py

@@ -22,13 +22,13 @@
 # #include "base/component_export.h"
 # #include "ui/gfx/x/xproto_types.h"
 #
-# typedef struct xcb_connection_t xcb_connection_t;
+# typedef struct _XDisplay XDisplay;
 #
 # namespace x11 {
 #
 # class COMPONENT_EXPORT(X11) XProto {
 #  public:
-#   explicit XProto(xcb_connection_t* conn);
+#   explicit XProto(XDisplay* display);
 #
 #   struct RGB {
 #     uint16_t red{};
@@ -46,10 +46,12 @@
 #     std::vector<RGB> colors{};
 #   };
 #
+#   using QueryColorsResponse = Response<QueryColorsReply>;
+#
 #   Future<QueryColorsReply> QueryColors(const QueryColorsRequest& request);
 #
 #  private:
-#   xcb_connection_t* const conn_;
+#   XDisplay* display_;
 # };
 #
 # }  // namespace x11
@@ -66,7 +68,7 @@
 #
 # namespace x11 {
 #
-# XProto::XProto(xcb_connection_t* conn) : conn_(conn) {}
+# XProto::XProto(XDisplay* display) : display_(display) {}
 #
 # Future<XProto::QueryColorsReply>
 # XProto::QueryColors(
@@ -97,7 +99,7 @@
 #     Write(&pixels_elem, &buf);
 #   }
 #
-#   return x11::SendRequest<XProto::QueryColorsReply>(conn_, &buf);
+#   return x11::SendRequest<XProto::QueryColorsReply>(display_, &buf);
 # }
 #
 # template<> COMPONENT_EXPORT(X11)
@@ -151,6 +153,7 @@
 #
 #   }
 #
+#   Align(&buf, 4);
 #   DCHECK_EQ(buf.offset < 32 ? 0 : buf.offset - 32, 4 * length);
 #
 #   return reply;
@@ -464,8 +467,11 @@ class GenXproto:
                 # xcb uses void* in some places, but we prefer to use
                 # std::vector<T> when possible.  Use T=uint8_t instead of
                 # T=void for containers.
-                type_name = 'uint8_t'
-            type_name = 'std::vector<%s>' % type_name
+                type_name = 'std::vector<uint8_t>'
+            elif type_name == 'char':
+                type_name = 'std::string'
+            else:
+                type_name = 'std::vector<%s>' % type_name
         self.write('%s %s{};' % (type_name, name))
 
     def copy_list(self, field):
@@ -585,6 +591,10 @@ class GenXproto:
         else:
             reply_name = 'void'
 
+        self.write(
+            'using %sResponse = Response<%s>;' % (method_name, reply_name))
+        self.write()
+
         self.write('Future<%s> %s(' % (reply_name, method_name))
         self.write('    const %s& request);' % request_name)
         self.write()
@@ -607,7 +617,7 @@ class GenXproto:
             self.is_read = False
             self.copy_container(request, 'request')
             self.write(
-                'return x11::SendRequest<%s>(conn_, &buf);' % reply_name)
+                'return x11::SendRequest<%s>(display_, &buf);' % reply_name)
         self.write()
 
         if reply:
@@ -621,6 +631,7 @@ class GenXproto:
                 self.write()
                 self.is_read = True
                 self.copy_container(reply, '(*reply)')
+                self.write('Align(&buf, 4);')
                 offset = 'buf.offset < 32 ? 0 : buf.offset - 32'
                 self.write('DCHECK_EQ(%s, 4 * length);' % offset)
                 self.write()
@@ -659,7 +670,7 @@ class GenXproto:
         for direct_import in self.module.direct_imports:
             self.write('#include "%s.h"' % direct_import[-1])
         self.write()
-        self.write('typedef struct xcb_connection_t xcb_connection_t;')
+        self.write('typedef struct _XDisplay XDisplay;')
         self.write()
         self.write('namespace x11 {')
         self.write()
@@ -669,12 +680,12 @@ class GenXproto:
         with Indent(self, 'class COMPONENT_EXPORT(X11) %s {' % name, '};'):
             self.namespace = ['x11', self.class_name]
             self.write('public:')
-            self.write('explicit %s(xcb_connection_t* conn);' % name)
+            self.write('explicit %s(XDisplay* display);' % name)
             self.write()
             for (name, item) in self.module.all:
                 self.declare_type(item, name)
             self.write('private:')
-            self.write('xcb_connection_t* const conn_;')
+            self.write('XDisplay* const display_;')
 
         self.write()
         self.write('}  // namespace x11')
@@ -695,7 +706,7 @@ class GenXproto:
         self.write()
         name = self.class_name
         self.write(
-            '%s::%s(xcb_connection_t* conn) : conn_(conn) {}' % (name, name))
+            '%s::%s(XDisplay* display) : display_(display) {}' % (name, name))
         self.write()
         for (name, item) in self.module.all:
             if isinstance(item, self.xcbgen.xtypes.Request):

ui/gfx/x/request_queue.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/gfx/x/request_queue.h"
+
+#include "base/check_op.h"
+
+namespace x11 {
+
+// static
+RequestQueue* RequestQueue::instance_ = nullptr;
+
+RequestQueue::RequestQueue() {
+  DCHECK(!instance_);
+  instance_ = this;
+}
+
+RequestQueue::~RequestQueue() {
+  DCHECK_EQ(instance_, this);
+  instance_ = nullptr;
+}
+
+// static
+RequestQueue* RequestQueue::GetInstance() {
+  return instance_;
+}
+
+}  // namespace x11

ui/gfx/x/request_queue.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef UI_GFX_X_REQUEST_QUEUE_H_
+#define UI_GFX_X_REQUEST_QUEUE_H_
+
+#include <xcb/xcb.h>
+
+#include <memory>
+
+#include "base/callback_forward.h"
+#include "base/component_export.h"
+#include "base/memory/free_deleter.h"
+
+namespace ui {
+class X11EventSource;
+}
+
+namespace x11 {
+
+// This interface allows //ui/gfx/x to call into //ui/events/platform/x11 which
+// is at a higher layer.  It should not be used by client code.
+class COMPONENT_EXPORT(X11) RequestQueue {
+ private:
+  friend class ui::X11EventSource;
+  template <typename T>
+  friend class Future;
+
+  using Reply = std::unique_ptr<uint8_t, base::FreeDeleter>;
+  using Error = std::unique_ptr<xcb_generic_error_t, base::FreeDeleter>;
+  using ResponseCallback = base::OnceCallback<void(Reply reply, Error error)>;
+
+  RequestQueue();
+  virtual ~RequestQueue();
+
+  // Adds a request to the queue.  |is_void| indicates if a reply is generated
+  // for this request.  |sequence| is the ID of the request.  |callback| will
+  // be called upon request completion (or failure).
+  virtual void AddRequest(bool is_void,
+                          unsigned int sequence,
+                          ResponseCallback callback) = 0;
+
+  static RequestQueue* GetInstance();
+
+  static RequestQueue* instance_;
+};
+
+}  // namespace x11
+
+#endif  //  UI_GFX_X_REQUEST_QUEUE_H_

ui/gfx/x/xproto_internal.h

@@ -9,6 +9,7 @@
 #error "This file should only be included by generated xprotos"
 #endif
 
+#include <X11/Xlib-xcb.h>
 #include <xcb/xcb.h>
 #include <xcb/xcbext.h>
 
@@ -68,7 +69,7 @@ inline void Align(ReadBuffer* buf, size_t align) {
 }
 
 template <typename Reply>
-Future<Reply> SendRequest(xcb_connection_t* conn, WriteBuffer* buf) {
+Future<Reply> SendRequest(XDisplay* display, WriteBuffer* buf) {
   // Clang crashes when the value of |is_void| is inlined below,
   // so keep this variable outside of |xpr|.
   constexpr bool is_void = std::is_void<Reply>::value;
@@ -96,10 +97,11 @@ Future<Reply> SendRequest(xcb_connection_t* conn, WriteBuffer* buf) {
   io[2].iov_len = buf->size();
   auto flags = XCB_REQUEST_CHECKED | XCB_REQUEST_RAW;
 
+  xcb_connection_t* conn = XGetXCBConnection(display);
   auto sequence = xcb_send_request(conn, flags, &io[2], &xpr);
   if (xcb_connection_has_error(conn))
     return {nullptr, base::nullopt};
-  return {conn, sequence};
+  return {display, sequence};
 }
 
 // Helper function for xcbproto popcount.  Given an integral type, returns the

ui/gfx/x/xproto_types.h

@@ -5,14 +5,21 @@
 #ifndef UI_GFX_X_XPROTO_TYPES_H_
 #define UI_GFX_X_XPROTO_TYPES_H_
 
+#include <X11/Xlib-xcb.h>
 #include <xcb/xcb.h>
 #include <xcb/xcbext.h>
 
 #include <cstdint>
 #include <memory>
 
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/memory/free_deleter.h"
 #include "base/optional.h"
+#include "ui/gfx/x/request_queue.h"
+#include "ui/gfx/x/xproto_util.h"
+
+typedef struct _XDisplay XDisplay;
 
 namespace x11 {
 
@@ -55,12 +62,38 @@ struct Response<void> {
       : error(std::move(error)) {}
 };
 
+// An x11::Future wraps an asynchronous response from the X11 server.  The
+// response may be waited-for with Sync(), or asynchronously handled by
+// installing a response handler using OnResponse().
 template <typename Reply>
 class Future {
  public:
+  using Callback = base::OnceCallback<void(Response<Reply> response)>;
+  using RQ = RequestQueue;
+
+  // If a user-defined response-handler is not installed before this object goes
+  // out of scope, a default response handler will be installed.  The default
+  // handler throws away the reply and prints the error if there is one.
   ~Future() {
-    if (sequence_)
-      xcb_discard_reply(conn_, *sequence_);
+    if (!sequence_)
+      return;
+
+    EnqueueRequest(base::BindOnce(
+        [](XDisplay* display, RQ::Reply reply, RQ::Error error) {
+          if (!error)
+            return;
+
+          x11::LogErrorEventDescription(XErrorEvent({
+              .type = error->response_type,
+              .display = display,
+              .resourceid = error->resource_id,
+              .serial = error->full_sequence,
+              .error_code = error->error_code,
+              .request_code = error->major_code,
+              .minor_code = error->minor_code,
+          }));
+        },
+        display_));
   }
 
   Future(const Future&) = delete;
@@ -74,13 +107,16 @@ class Future {
     future.sequence_ = base::nullopt;
   }
 
+  xcb_connection_t* connection() { return XGetXCBConnection(display_); }
+
+  // Blocks until we receive the response from the server. Returns the response.
   Response<Reply> Sync() {
     if (!sequence_)
       return {{}, {}};
 
     Error* raw_error = nullptr;
     uint8_t* raw_reply = reinterpret_cast<uint8_t*>(
-        xcb_wait_for_reply(conn_, *sequence_, &raw_error));
+        xcb_wait_for_reply(connection(), *sequence_, &raw_error));
     sequence_ = base::nullopt;
 
     std::unique_ptr<Reply> reply;
@@ -96,23 +132,49 @@ class Future {
     return {std::move(reply), std::move(error)};
   }
 
+  // Installs |callback| to be run when the response is received.
+  void OnResponse(Callback callback) {
+    if (!sequence_)
+      return;
+
+    // This intermediate callback handles the conversion from |raw_reply| to a
+    // real Reply object before feeding the result to |callback|.  This means
+    // |callback| must be bound as the first argument of the intermediate
+    // function.
+    auto wrapper = [](Callback callback, RQ::Reply raw_reply, RQ::Error error) {
+      std::unique_ptr<Reply> reply =
+          raw_reply ? detail::ReadReply<Reply>(raw_reply.get()) : nullptr;
+      std::move(callback).Run({std::move(reply), std::move(error)});
+    };
+    EnqueueRequest(base::BindOnce(wrapper, std::move(callback)));
+
+    sequence_ = base::nullopt;
+  }
+
  private:
   template <typename R>
-  friend Future<R> SendRequest(xcb_connection_t*, std::vector<uint8_t>*);
+  friend Future<R> SendRequest(XDisplay*, std::vector<uint8_t>*);
 
-  Future(xcb_connection_t* conn, base::Optional<unsigned int> sequence)
-      : conn_(conn), sequence_(sequence) {}
+  Future(XDisplay* display, base::Optional<unsigned int> sequence)
+      : display_(display), sequence_(sequence) {}
 
-  xcb_connection_t* conn_;
+  void EnqueueRequest(RQ::ResponseCallback callback) {
+    RQ::GetInstance()->AddRequest(std::is_void<Reply>::value, *sequence_,
+                                  std::move(callback));
+  }
+
+  XDisplay* display_;
   base::Optional<unsigned int> sequence_;
 };
 
+// Sync() specialization for requests that don't generate replies.  The returned
+// response will only contain an error if there was one.
 template <>
 inline Response<void> Future<void>::Sync() {
   if (!sequence_)
     return Response<void>(nullptr);
 
-  Error* raw_error = xcb_request_check(conn_, {*sequence_});
+  Error* raw_error = xcb_request_check(connection(), {*sequence_});
   std::unique_ptr<Error, base::FreeDeleter> error;
   if (raw_error)
     error.reset(raw_error);
@@ -120,6 +182,24 @@ inline Response<void> Future<void>::Sync() {
   return Response<void>{std::move(error)};
 }
 
+// OnResponse() specialization for requests that don't generate replies.  The
+// response argument to |callback| will only contain an error if there was one.
+template <>
+inline void Future<void>::OnResponse(Callback callback) {
+  if (!sequence_)
+    return;
+
+  // See Future<Reply>::OnResponse() for an explanation of why
+  // this wrapper is necessary.
+  auto wrapper = [](Callback callback, RQ::Reply reply, RQ::Error error) {
+    DCHECK(!reply);
+    std::move(callback).Run(Response<void>{std::move(error)});
+  };
+  EnqueueRequest(base::BindOnce(wrapper, std::move(callback)));
+
+  sequence_ = base::nullopt;
+}
+
 }  // namespace x11
 
 #endif  //  UI_GFX_X_XPROTO_TYPES_H_

ui/gfx/x/xproto_util.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/gfx/x/xproto_util.h"
+
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/stringprintf.h"
+#include "ui/gfx/x/xproto.h"
+
+namespace x11 {
+
+void LogErrorEventDescription(const XErrorEvent& error_event) {
+  // This function may make some expensive round trips (XListExtensions,
+  // XQueryExtension), but the only effect this function has is LOG(WARNING),
+  // so early-return if the log would never be sent anyway.
+  if (!LOG_IS_ON(WARNING))
+    return;
+
+  char error_str[256];
+  char request_str[256];
+
+  XDisplay* dpy = error_event.display;
+  XProto conn{dpy};
+  XGetErrorText(dpy, error_event.error_code, error_str, sizeof(error_str));
+
+  strncpy(request_str, "Unknown", sizeof(request_str));
+  if (error_event.request_code < 128) {
+    std::string num = base::NumberToString(error_event.request_code);
+    XGetErrorDatabaseText(dpy, "XRequest", num.c_str(), "Unknown", request_str,
+                          sizeof(request_str));
+  } else {
+    if (auto response = conn.ListExtensions({}).Sync()) {
+      for (const auto& str : response->names) {
+        int ext_code, first_event, first_error;
+        const char* name = str.name.c_str();
+        XQueryExtension(dpy, name, &ext_code, &first_event, &first_error);
+        if (error_event.request_code == ext_code) {
+          std::string msg =
+              base::StringPrintf("%s.%d", name, error_event.minor_code);
+          XGetErrorDatabaseText(dpy, "XRequest", msg.c_str(), "Unknown",
+                                request_str, sizeof(request_str));
+          break;
+        }
+      }
+    }
+  }
+
+  LOG(WARNING) << "X error received: "
+               << "serial " << error_event.serial << ", "
+               << "error_code " << static_cast<int>(error_event.error_code)
+               << " (" << error_str << "), "
+               << "request_code " << static_cast<int>(error_event.request_code)
+               << ", "
+               << "minor_code " << static_cast<int>(error_event.minor_code)
+               << " (" << request_str << ")";
+}
+
+}  // namespace x11

ui/gfx/x/xproto_util.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef UI_GFX_X_XPROTO_UTIL_H_
+#define UI_GFX_X_XPROTO_UTIL_H_
+
+#include <X11/Xlib.h>
+
+#include "base/component_export.h"
+
+namespace x11 {
+
+COMPONENT_EXPORT(X11)
+void LogErrorEventDescription(const XErrorEvent& error_event);
+
+}  // namespace x11
+
+#endif  //  UI_GFX_X_XPROTO_UTIL_H_

ui/gl/gl_surface_egl_x11.cc

@@ -55,8 +55,7 @@ bool NativeViewGLSurfaceEGLX11::Initialize(GLSurfaceFormat format) {
   // Query all child windows and store them. ANGLE creates a child window when
   // eglCreateWidnowSurface is called on X11 and expose events from this window
   // need to be received by this class.
-  Display* x11_display = GetXNativeDisplay();
-  x11::XProto conn{XGetXCBConnection(x11_display)};
+  x11::XProto conn{GetXNativeDisplay()};
   if (auto reply = conn.QueryTree({window_}).Sync())
     children_ = std::move(reply->children);
 

ui/views/test/ui_controls_factory_desktop_aurax11.cc

@@ -44,6 +44,17 @@ using ui_controls::UP;
 // Mask of the buttons currently down.
 unsigned button_down_mask = 0;
 
+// Restore Xlib constants that were #undef'ed by gen/ui/gfx/x/xproto.h.
+constexpr int CopyFromParent = 0;
+constexpr int InputOnly = 1;
+constexpr int KeyPress = 2;
+constexpr int KeyRelease = 3;
+constexpr int ButtonPress = 4;
+constexpr int ButtonRelease = 5;
+constexpr int Button1 = 1;
+constexpr int Button2 = 2;
+constexpr int Button3 = 3;
+
 class UIControlsDesktopX11 : public UIControlsAura {
  public:
   UIControlsDesktopX11()
@@ -58,7 +69,7 @@ class UIControlsDesktopX11 : public UIControlsAura {
                                 0,               // border width
                                 CopyFromParent,  // depth
                                 InputOnly,
-                                CopyFromParent,  // visual
+                                reinterpret_cast<Visual*>(CopyFromParent),
                                 0,
                                 nullptr)) {
     XStoreName(x_display_, x_window_, "Chromium UIControlsDesktopX11 Window");