[XProto] Implement error handling

This is in preparation for removing Xlib error functions.

BUG=1066670
R=sky

Change-Id: I495065cabcefff11dc63fb282dc0af1bdcf41342
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2443790Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814328}

ui/gfx/x/BUILD.gn

@@ -59,6 +59,7 @@ action("gen_xprotos") {
   sources = []
   outputs = [
     "$target_gen_dir/read_event.cc",
+    "$target_gen_dir/read_error.cc",
     "$target_gen_dir/extension_manager.h",
     "$target_gen_dir/extension_manager.cc",
   ]
@@ -96,6 +97,8 @@ component("xprotos") {
               "connection.cc",
               "event.h",
               "event.cc",
+              "error.h",
+              "error.cc",
               "x11_switches.cc",
               "x11_switches.h",
               "x11.h",

ui/gfx/x/connection.cc

@@ -210,6 +210,32 @@ base::ThreadLocalOwnedPointer<Connection>& GetConnectionTLS() {
   return *tls;
 }
 
+class UnknownError : public Error {
+ public:
+  explicit UnknownError(FutureBase::RawError error_bytes)
+      : error_bytes_(error_bytes) {}
+
+  ~UnknownError() override = default;
+
+  std::string ToString() const override {
+    std::stringstream ss;
+    ss << "x11::UnknownError{";
+    // Errors are always a fixed 32 bytes.
+    for (size_t i = 0; i < 32; i++) {
+      char buf[3];
+      sprintf(buf, "%02x", error_bytes_->data()[i]);
+      ss << "0x" << buf;
+      if (i != 31)
+        ss << ", ";
+    }
+    ss << "}";
+    return ss.str();
+  }
+
+ private:
+  FutureBase::RawError error_bytes_;
+};
+
 }  // namespace
 
 // static
@@ -274,6 +300,8 @@ Connection::Connection(const std::string& address)
   }
 
   ResetKeyboardState();
+
+  InitErrorParsers();
 }
 
 Connection::~Connection() {
@@ -295,14 +323,30 @@ Connection::Request::Request(unsigned int sequence,
     : sequence(sequence), callback(std::move(callback)) {}
 
 Connection::Request::Request(Request&& other)
-    : sequence(other.sequence), callback(std::move(other.callback)) {}
+    : sequence(other.sequence),
+      callback(std::move(other.callback)),
+      have_response(other.have_response),
+      reply(std::move(other.reply)),
+      error(std::move(other.error)) {}
 
 Connection::Request::~Request() = default;
 
-bool Connection::HasNextResponse() const {
-  return !requests_.empty() &&
-         CompareSequenceIds(XLastKnownRequestProcessed(display_),
-                            requests_.front().sequence) >= 0;
+bool Connection::HasNextResponse() {
+  if (requests_.empty())
+    return false;
+  auto& request = requests_.front();
+  if (request.have_response)
+    return true;
+
+  void* reply = nullptr;
+  xcb_generic_error_t* error = nullptr;
+  request.have_response =
+      xcb_poll_for_reply(XcbConnection(), request.sequence, &reply, &error);
+  if (reply)
+    request.reply = base::MakeRefCounted<MallocedRefCountedMemory>(reply);
+  if (error)
+    request.error = base::MakeRefCounted<MallocedRefCountedMemory>(error);
+  return request.have_response;
 }
 
 int Connection::GetFd() {
@@ -374,7 +418,7 @@ Event Connection::WaitForNextEvent() {
   return Event();
 }
 
-bool Connection::HasPendingResponses() const {
+bool Connection::HasPendingResponses() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return !events_.empty() || HasNextResponse();
 }
@@ -425,18 +469,12 @@ void Connection::Dispatch(Delegate* delegate) {
   DCHECK(display_);
 
   auto process_next_response = [&] {
-    xcb_connection_t* connection = XGetXCBConnection(display_);
-    auto request = std::move(requests_.front());
-    requests_.pop();
+    DCHECK(!requests_.empty());
+    DCHECK(requests_.front().have_response);
 
-    void* raw_reply = nullptr;
-    xcb_generic_error_t* raw_error = nullptr;
-    xcb_poll_for_reply(connection, request.sequence, &raw_reply, &raw_error);
-
-    scoped_refptr<MallocedRefCountedMemory> reply;
-    if (raw_reply)
-      reply = base::MakeRefCounted<MallocedRefCountedMemory>(raw_reply);
-    std::move(request.callback).Run(reply, FutureBase::RawError{raw_error});
+    Request request = std::move(requests_.front());
+    requests_.pop();
+    std::move(request.callback).Run(request.reply, request.error);
   };
 
   auto process_next_event = [&] {
@@ -689,4 +727,19 @@ KeySym Connection::TranslateKey(uint32_t key, unsigned int modifiers) const {
   return upper;
 }
 
+std::unique_ptr<Error> Connection::ParseError(
+    FutureBase::RawError error_bytes) {
+  if (!error_bytes)
+    return nullptr;
+  struct ErrorHeader {
+    uint8_t response_type;
+    uint8_t error_code;
+    uint16_t sequence;
+  };
+  auto error_code = error_bytes->front_as<ErrorHeader>()->error_code;
+  if (auto parser = error_parsers_[error_code])
+    return parser(error_bytes);
+  return std::make_unique<UnknownError>(error_bytes);
+}
+
 }  // namespace x11

ui/gfx/x/connection.h

@@ -113,7 +113,7 @@ class COMPONENT_EXPORT(X11) Connection : public XProto,
   Event WaitForNextEvent();
 
   // Are there any events, errors, or replies already buffered?
-  bool HasPendingResponses() const;
+  bool HasPendingResponses();
 
   // Dispatch any buffered events, errors, or replies.
   void Dispatch(Delegate* delegate);
@@ -153,13 +153,16 @@ class COMPONENT_EXPORT(X11) Connection : public XProto,
 
     const unsigned int sequence;
     FutureBase::ResponseCallback callback;
+    bool have_response = false;
+    FutureBase::RawReply reply;
+    FutureBase::RawError error;
   };
 
   void InitRootDepthAndVisual();
 
   void AddRequest(unsigned int sequence, FutureBase::ResponseCallback callback);
 
-  bool HasNextResponse() const;
+  bool HasNextResponse();
 
   void PreDispatchEvent(const Event& event);
 
@@ -171,6 +174,11 @@ class COMPONENT_EXPORT(X11) Connection : public XProto,
 
   KeySym TranslateKey(uint32_t keycode, unsigned int modifiers) const;
 
+  // This function is implemented in the generated read_error.cc.
+  void InitErrorParsers();
+
+  std::unique_ptr<Error> ParseError(FutureBase::RawError error_bytes);
+
   XDisplay* const display_;
 
   bool synchronous_ = false;
@@ -198,6 +206,10 @@ class COMPONENT_EXPORT(X11) Connection : public XProto,
 
   std::queue<Request> requests_;
 
+  using ErrorParser =
+      std::unique_ptr<Error> (*)(FutureBase::RawError error_bytes);
+  std::array<ErrorParser, 256> error_parsers_{};
+
   SEQUENCE_CHECKER(sequence_checker_);
 };
 

ui/gfx/x/connection_unittest.cc

@@ -101,9 +101,11 @@ TEST(X11ConnectionTest, Error) {
 
   auto geometry = connection.GetGeometry({invalid_window}).Sync();
   ASSERT_FALSE(geometry);
-  xcb_generic_error_t* error = geometry.error.get();
-  EXPECT_EQ(error->error_code, XCB_DRAWABLE);
-  EXPECT_EQ(error->resource_id, static_cast<uint32_t>(invalid_window));
+  auto* error = geometry.error.get();
+  ASSERT_TRUE(error);
+  // TODO(thomasanderson): Implement As<> for errors, similar to events.
+  auto* drawable_error = reinterpret_cast<x11::DrawableError*>(error);
+  EXPECT_EQ(drawable_error->bad_value, static_cast<uint32_t>(invalid_window));
 }
 
 }  // namespace x11

ui/gfx/x/error.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/error.h"
+
+namespace x11 {
+
+Error::Error() = default;
+
+Error::~Error() = default;
+
+}  // namespace x11

ui/gfx/x/error.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_ERROR_H_
+#define UI_GFX_X_ERROR_H_
+
+#include <string>
+
+#include "base/component_export.h"
+
+namespace x11 {
+
+class COMPONENT_EXPORT(X11) Error {
+ public:
+  Error();
+  virtual ~Error();
+
+  virtual std::string ToString() const = 0;
+
+ private:
+};
+
+}  // namespace x11
+
+#endif  // UI_GFX_X_EVENT_H_

ui/gfx/x/gen_xproto.py

@@ -919,6 +919,14 @@ class GenXproto(FileWriter):
             self.write('x11::Window* GetWindow() { return %s; }' % ret)
         self.write()
 
+    def declare_error(self, error, name):
+        name = adjust_type_name(name[-1] + 'Error')
+        with Indent(self, 'struct %s : public x11::Error {' % name, '};'):
+            self.declare_fields(error.fields)
+            self.write()
+            self.write('std::string ToString() const override;')
+        self.write()
+
     def declare_container(self, struct, struct_name):
         name = struct_name[-1] + self.type_suffix(struct)
         with Indent(self, 'struct %s {' % adjust_type_name(name), '};'):
@@ -1025,8 +1033,9 @@ class GenXproto(FileWriter):
             self.write('Align(&buf, 4);')
             self.write()
             reply_has_fds = reply and any(field.isfd for field in reply.fields)
-            self.write('return x11::SendRequest<%s>(connection_, &buf, %s);' %
-                       (reply_name, 'true' if reply_has_fds else 'false'))
+            self.write(
+                'return x11::SendRequest<%s>(connection_, &buf, %s, "%s");' %
+                (reply_name, 'true' if reply_has_fds else 'false', prefix))
         self.write()
 
         if not reply:
@@ -1066,6 +1075,29 @@ class GenXproto(FileWriter):
                 self.write('DCHECK_LE(buf.offset, 32ul);')
         self.write()
 
+    def define_error(self, error, name):
+        self.namespace = ['x11']
+        name = self.qualtype(error, name)
+        with Indent(self, 'std::string %s::ToString() const {' % name, '}'):
+            self.write('std::stringstream ss_;')
+            self.write('ss_ << "x11::%s{";' % name)
+            fields = [field for field in error.fields if field.visible]
+            for i, field in enumerate(fields):
+                terminator = '' if i == len(fields) - 1 else ' << ", "'
+                self.write('ss_ << ".%s = " << static_cast<uint64_t>(%s)%s;' %
+                           (field.field_name, field.field_name, terminator))
+            self.write('ss_ << "}";')
+            self.write('return ss_.str();')
+        self.write()
+        self.write('template <>')
+        self.write('void ReadError<%s>(' % name)
+        with Indent(self, '    %s* error_, ReadBuffer* buffer) {' % name, '}'):
+            self.write('auto& buf = *buffer;')
+            self.write()
+            self.is_read = True
+            self.copy_container(error, '(*error_)')
+            self.write('DCHECK_LE(buf.offset, 32ul);')
+
     def define_type(self, item, name):
         if name in READ_SPECIAL:
             self.read_special_container(item, name)
@@ -1075,6 +1107,8 @@ class GenXproto(FileWriter):
             self.define_request(item)
         elif item.is_event:
             self.define_event(item, name)
+        elif isinstance(item, self.xcbgen.xtypes.Error):
+            self.define_error(item, name)
 
     def declare_type(self, item, name):
         if item.is_union:
@@ -1083,6 +1117,8 @@ class GenXproto(FileWriter):
             self.declare_request(item)
         elif item.is_event:
             self.declare_event(item, name)
+        elif isinstance(item, self.xcbgen.xtypes.Error):
+            self.declare_error(item, name)
         elif item.is_container:
             self.declare_container(item, name)
         elif isinstance(item, self.xcbgen.xtypes.Enum):
@@ -1314,6 +1350,7 @@ class GenXproto(FileWriter):
         self.write('#include "base/memory/scoped_refptr.h"')
         self.write('#include "base/optional.h"')
         self.write('#include "base/files/scoped_file.h"')
+        self.write('#include "ui/gfx/x/error.h"')
         self.write('#include "ui/gfx/x/xproto_types.h"')
         imports = set(self.module.direct_imports)
         if self.module.namespace.is_ext:
@@ -1601,6 +1638,90 @@ class GenReadEvent(FileWriter):
         self.write('}  // namespace x11')
 
 
+class GenReadError(FileWriter):
+    def __init__(self, gen_dir, genprotos, xcbgen):
+        FileWriter.__init__(self)
+
+        self.gen_dir = gen_dir
+        self.genprotos = genprotos
+        self.xcbgen = xcbgen
+
+    def get_errors_for_proto(self, proto):
+        errors = {}
+        for _, item in proto.module.all:
+            if isinstance(item, self.xcbgen.xtypes.Error):
+                for name in item.opcodes:
+                    id = int(item.opcodes[name])
+                    if id < 0:
+                        continue
+                    name = [adjust_type_name(part) for part in name[1:]]
+                    typename = '::'.join(name) + 'Error'
+                    errors[id] = typename
+        return errors
+
+    def gen_errors_for_proto(self, errors, proto):
+        if proto.module.namespace.is_ext:
+            cond = 'if (%s().present()) {' % proto.proto
+            first_error = '%s().first_error()' % proto.proto
+        else:
+            cond = '{'
+            first_error = '0'
+        with Indent(self, cond, '}'):
+            self.write('uint8_t first_error = %s;' % first_error)
+            for id, name in sorted(errors.items()):
+                with Indent(self, '{', '}'):
+                    self.write('auto error_code = first_error + %d;' % id)
+                    self.write('auto parse = MakeError<%s>;' % name)
+                    self.write('add_parser(error_code, first_error, parse);')
+        self.write()
+
+    def gen_init_error_parsers(self):
+        self.write('uint8_t first_errors[256];')
+        self.write('memset(first_errors, 0, sizeof(first_errors));')
+        self.write()
+        args = 'uint8_t error_code, uint8_t first_error, ErrorParser parser'
+        with Indent(self, 'auto add_parser = [&](%s) {' % args, '};'):
+            cond = ('!error_parsers_[error_code] || ' +
+                    'first_error > first_errors[error_code]')
+            with Indent(self, 'if (%s) {' % cond, '}'):
+                self.write('first_errors[error_code] = error_code;')
+                self.write('error_parsers_[error_code] = parser;')
+        self.write()
+        for proto in self.genprotos:
+            errors = self.get_errors_for_proto(proto)
+            if errors:
+                self.gen_errors_for_proto(errors, proto)
+
+    def gen_source(self):
+        self.file = open(os.path.join(self.gen_dir, 'read_error.cc'), 'w')
+        self.write('#include "ui/gfx/x/connection.h"')
+        self.write('#include "ui/gfx/x/error.h"')
+        self.write('#include "ui/gfx/x/xproto_internal.h"')
+        self.write()
+        for genproto in self.genprotos:
+            self.write('#include "ui/gfx/x/%s.h"' % genproto.proto)
+        self.write()
+        self.write('namespace x11 {')
+        self.write()
+        self.write('namespace {')
+        self.write()
+        self.write('template <typename T>')
+        sig = 'std::unique_ptr<Error> MakeError(FutureBase::RawError error_)'
+        with Indent(self, '%s {' % sig, '}'):
+            self.write('ReadBuffer buf(error_);')
+            self.write('auto error = std::make_unique<T>();')
+            self.write('ReadError(error.get(), &buf);')
+            self.write('return error;')
+        self.write()
+        self.write('}  // namespace')
+        self.write()
+        with Indent(self, 'void Connection::InitErrorParsers() {', '}'):
+            self.gen_init_error_parsers()
+
+        self.write()
+        self.write('}  // namespace x11')
+
+
 def main():
     parser = argparse.ArgumentParser()
     parser.add_argument('xcbproto_dir', type=str)
@@ -1639,8 +1760,9 @@ def main():
     gen_extension_manager.gen_header()
     gen_extension_manager.gen_source()
 
-    gen_read_event = GenReadEvent(args.gen_dir, genprotos)
-    gen_read_event.gen_source()
+    GenReadEvent(args.gen_dir, genprotos).gen_source()
+
+    GenReadError(args.gen_dir, genprotos, xcbgen).gen_source()
 
     return 0
 

ui/gfx/x/x11.h

@@ -56,7 +56,6 @@ int XCloseDisplay(Display*);
 int XFlush(Display*);
 xcb_connection_t* XGetXCBConnection(Display* dpy);
 void XSetEventQueueOwner(Display* dpy, enum XEventQueueOwner owner);
-unsigned long XLastKnownRequestProcessed(Display*);
 int (*XSynchronize(Display*, Bool))(Display*);
 int XGetErrorDatabaseText(Display*,
                           const char*,
@@ -67,7 +66,6 @@ int XGetErrorDatabaseText(Display*,
 int XGetErrorText(Display*, int, char*, int);
 XErrorHandler XSetErrorHandler(XErrorHandler);
 XIOErrorHandler XSetIOErrorHandler(XIOErrorHandler);
-int XStoreName(Display*, Window, const char*);
 }
 
 #endif  // UI_GFX_X_X11_H_

ui/gfx/x/xproto_internal.h

@@ -37,6 +37,9 @@ struct EnumBase<T, typename std::enable_if_t<std::is_enum<T>::value>> {
 template <typename T>
 using EnumBaseType = typename EnumBase<T>::type;
 
+template <typename T>
+void ReadError(T* error, ReadBuffer* buf);
+
 // Calls free() on the underlying data when the count drops to 0.
 class COMPONENT_EXPORT(X11) MallocedRefCountedMemory
     : public base::RefCountedMemory {
@@ -137,10 +140,12 @@ base::Optional<unsigned int> SendRequestImpl(x11::Connection* connection,
 template <typename Reply>
 Future<Reply> SendRequest(x11::Connection* connection,
                           WriteBuffer* buf,
-                          bool reply_has_fds) {
+                          bool reply_has_fds,
+                          const char* request_name) {
   auto sequence = SendRequestImpl(connection, buf, std::is_void<Reply>::value,
                                   reply_has_fds);
-  return {sequence ? connection : nullptr, sequence};
+  return {sequence ? connection : nullptr, sequence,
+          sequence ? request_name : nullptr};
 }
 
 // Helper function for xcbproto popcount.  Given an integral type, returns the

ui/gfx/x/xproto_types.cc

@@ -80,8 +80,11 @@ void WriteBuffer::AppendCurrentBuffer() {
 }
 
 FutureBase::FutureBase(Connection* connection,
-                       base::Optional<unsigned int> sequence)
-    : connection_(connection), sequence_(sequence) {}
+                       base::Optional<unsigned int> sequence,
+                       const char* request_name)
+    : connection_(connection),
+      sequence_(sequence),
+      request_name_(request_name) {}
 
 // 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
@@ -91,45 +94,53 @@ FutureBase::~FutureBase() {
     return;
 
   OnResponseImpl(base::BindOnce(
-      [](Connection* connection, RawReply reply, RawError error) {
+      [](Connection* connection, const char* request_name, RawReply reply,
+         RawError error) {
         if (!error)
           return;
 
-        x11::LogErrorEventDescription(error->full_sequence, error->error_code,
-                                      error->major_code, error->minor_code);
+        LOG(WARNING) << "X error received.  Request: x11::" << request_name
+                     << "Request, Error: "
+                     << connection->ParseError(error)->ToString();
       },
-      connection_));
+      connection_, request_name_));
 }
 
 FutureBase::FutureBase(FutureBase&& future)
-    : connection_(future.connection_), sequence_(future.sequence_) {
-  future.connection_ = nullptr;
-  future.sequence_ = base::nullopt;
+    : connection_(future.connection_),
+      sequence_(future.sequence_),
+      request_name_(future.request_name_) {
+  future.Reset();
 }
 
 FutureBase& FutureBase::operator=(FutureBase&& future) {
   connection_ = future.connection_;
   sequence_ = future.sequence_;
-  future.connection_ = nullptr;
-  future.sequence_ = base::nullopt;
+  request_name_ = future.request_name_;
+  future.Reset();
   return *this;
 }
 
-void FutureBase::SyncImpl(Error** raw_error,
-                          scoped_refptr<base::RefCountedMemory>* raw_reply) {
+void FutureBase::SyncImpl(RawError* raw_error, RawReply* raw_reply) {
   if (!sequence_)
     return;
+  xcb_generic_error_t* error = nullptr;
   auto* reply = reinterpret_cast<uint8_t*>(
-      xcb_wait_for_reply(connection_->XcbConnection(), *sequence_, raw_error));
+      xcb_wait_for_reply(connection_->XcbConnection(), *sequence_, &error));
   if (reply)
     *raw_reply = base::MakeRefCounted<MallocedRefCountedMemory>(reply);
+  if (error)
+    *raw_error = base::MakeRefCounted<MallocedRefCountedMemory>(error);
   sequence_ = base::nullopt;
 }
 
-void FutureBase::SyncImpl(Error** raw_error) {
+void FutureBase::SyncImpl(RawError* raw_error) {
   if (!sequence_)
     return;
-  *raw_error = xcb_request_check(connection_->XcbConnection(), {*sequence_});
+  if (xcb_generic_error_t* error =
+          xcb_request_check(connection_->XcbConnection(), {*sequence_})) {
+    *raw_error = base::MakeRefCounted<MallocedRefCountedMemory>(error);
+  }
   sequence_ = base::nullopt;
 }
 
@@ -140,4 +151,18 @@ void FutureBase::OnResponseImpl(ResponseCallback callback) {
   sequence_ = base::nullopt;
 }
 
+// static
+std::unique_ptr<Error> FutureBase::ParseErrorImpl(x11::Connection* connection,
+                                                  RawError raw_error) {
+  if (!raw_error)
+    return nullptr;
+  return connection->ParseError(raw_error);
+}
+
+void FutureBase::Reset() {
+  connection_ = nullptr;
+  sequence_ = base::nullopt;
+  request_name_ = nullptr;
+}
+
 }  // namespace x11

ui/gfx/x/xproto_types.h

@@ -18,6 +18,7 @@
 #include "base/memory/ref_counted_memory.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/optional.h"
+#include "ui/gfx/x/error.h"
 
 typedef struct _XDisplay XDisplay;
 
@@ -100,8 +101,6 @@ std::unique_ptr<Reply> ReadReply(ReadBuffer* buffer);
 
 }  // namespace detail
 
-using Error = xcb_generic_error_t;
-
 template <class Reply>
 class Future;
 
@@ -121,31 +120,29 @@ struct Response {
   Reply* operator->() { return reply.get(); }
 
   std::unique_ptr<Reply> reply;
-  std::unique_ptr<Error, base::FreeDeleter> error;
+  std::unique_ptr<Error> error;
 
  private:
   friend class Future<Reply>;
 
-  Response(std::unique_ptr<Reply> reply,
-           std::unique_ptr<Error, base::FreeDeleter> error)
+  Response(std::unique_ptr<Reply> reply, std::unique_ptr<Error> error)
       : reply(std::move(reply)), error(std::move(error)) {}
 };
 
 template <>
 struct Response<void> {
-  std::unique_ptr<Error, base::FreeDeleter> error;
+  std::unique_ptr<Error> error;
 
  private:
   friend class Future<void>;
 
-  explicit Response(std::unique_ptr<Error, base::FreeDeleter> error)
-      : error(std::move(error)) {}
+  explicit Response(std::unique_ptr<Error> error) : error(std::move(error)) {}
 };
 
 class COMPONENT_EXPORT(X11) FutureBase {
  public:
   using RawReply = scoped_refptr<base::RefCountedMemory>;
-  using RawError = std::unique_ptr<xcb_generic_error_t, base::FreeDeleter>;
+  using RawError = scoped_refptr<base::RefCountedMemory>;
   using ResponseCallback =
       base::OnceCallback<void(RawReply reply, RawError error)>;
 
@@ -153,21 +150,30 @@ class COMPONENT_EXPORT(X11) FutureBase {
   FutureBase& operator=(const FutureBase&) = delete;
 
  protected:
-  FutureBase(Connection* connection, base::Optional<unsigned int> sequence);
+  FutureBase(Connection* connection,
+             base::Optional<unsigned int> sequence,
+             const char* request_name);
   ~FutureBase();
 
   FutureBase(FutureBase&& future);
   FutureBase& operator=(FutureBase&& future);
 
-  void SyncImpl(Error** raw_error,
-                scoped_refptr<base::RefCountedMemory>* raw_reply);
-  void SyncImpl(Error** raw_error);
+  void SyncImpl(RawError* raw_error, RawReply* raw_reply);
+  void SyncImpl(RawError* raw_error);
 
   void OnResponseImpl(ResponseCallback callback);
 
+  x11::Connection* connection() { return connection_; }
+
+  static std::unique_ptr<Error> ParseErrorImpl(x11::Connection* connection,
+                                               RawError raw_error);
+
  private:
-  Connection* connection_;
+  void Reset();
+
+  Connection* connection_ = nullptr;
   base::Optional<unsigned int> sequence_;
+  const char* request_name_ = nullptr;
 };
 
 // An x11::Future wraps an asynchronous response from the X11 server.  The
@@ -178,12 +184,12 @@ class Future : public FutureBase {
  public:
   using Callback = base::OnceCallback<void(Response<Reply> response)>;
 
-  Future() : FutureBase(nullptr, base::nullopt) {}
+  Future() : FutureBase(nullptr, base::nullopt, nullptr) {}
 
   // Blocks until we receive the response from the server. Returns the response.
   Response<Reply> Sync() {
-    Error* raw_error = nullptr;
-    scoped_refptr<base::RefCountedMemory> raw_reply;
+    RawError raw_error;
+    RawReply raw_reply;
     SyncImpl(&raw_error, &raw_reply);
 
     std::unique_ptr<Reply> reply;
@@ -192,9 +198,7 @@ class Future : public FutureBase {
       reply = detail::ReadReply<Reply>(&buf);
     }
 
-    std::unique_ptr<Error, base::FreeDeleter> error;
-    if (raw_error)
-      error.reset(raw_error);
+    std::unique_ptr<Error> error = ParseErrorImpl(connection(), raw_error);
 
     return {std::move(reply), std::move(error)};
   }
@@ -205,13 +209,17 @@ class Future : public FutureBase {
     // 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, RawReply raw_reply, RawError error) {
-      ReadBuffer buf(raw_reply);
-      std::unique_ptr<Reply> reply =
-          raw_reply ? detail::ReadReply<Reply>(&buf) : nullptr;
+    auto wrapper = [](x11::Connection* connection, Callback callback,
+                      RawReply raw_reply, RawError raw_error) {
+      std::unique_ptr<Reply> reply;
+      if (raw_reply) {
+        ReadBuffer buf(raw_reply);
+        reply = detail::ReadReply<Reply>(&buf);
+      }
+      std::unique_ptr<Error> error = ParseErrorImpl(connection, raw_error);
       std::move(callback).Run({std::move(reply), std::move(error)});
     };
-    OnResponseImpl(base::BindOnce(wrapper, std::move(callback)));
+    OnResponseImpl(base::BindOnce(wrapper, connection(), std::move(callback)));
   }
 
   void IgnoreError() {
@@ -220,24 +228,21 @@ class Future : public FutureBase {
 
  private:
   template <typename R>
-  friend Future<R> SendRequest(Connection*, WriteBuffer*, bool);
+  friend Future<R> SendRequest(Connection*, WriteBuffer*, bool, const char*);
 
-  Future(Connection* connection, base::Optional<unsigned int> sequence)
-      : FutureBase(connection, sequence) {}
+  Future(Connection* connection,
+         base::Optional<unsigned int> sequence,
+         const char* request_name)
+      : FutureBase(connection, sequence, request_name) {}
 };
 
 // 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() {
-  Error* raw_error = nullptr;
+  RawError raw_error;
   SyncImpl(&raw_error);
-
-  std::unique_ptr<Error, base::FreeDeleter> error;
-  if (raw_error)
-    error.reset(raw_error);
-
-  return Response<void>{std::move(error)};
+  return Response<void>{ParseErrorImpl(connection(), raw_error)};
 }
 
 // OnResponse() specialization for requests that don't generate replies.  The
@@ -246,11 +251,12 @@ template <>
 inline void Future<void>::OnResponse(Callback callback) {
   // See Future<Reply>::OnResponse() for an explanation of why
   // this wrapper is necessary.
-  auto wrapper = [](Callback callback, RawReply reply, RawError error) {
+  auto wrapper = [](x11::Connection* connection, Callback callback,
+                    RawReply reply, RawError error) {
     DCHECK(!reply);
-    std::move(callback).Run(Response<void>{std::move(error)});
+    std::move(callback).Run(Response<void>{ParseErrorImpl(connection, error)});
   };
-  OnResponseImpl(base::BindOnce(wrapper, std::move(callback)));
+  OnResponseImpl(base::BindOnce(wrapper, connection(), std::move(callback)));
 }
 
 template <>