Change X11 error handler override to allow easy X11 error checking.

ui/base/x/x11_util.cc

 // Copyright (c) 2011 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.
 
 // This file defines utility functions for X11 (Linux only). This code has been
 // ported from XCB since we can't use XCB on Ubuntu while its 32-bit support
 // remains woefully incomplete.
 
 #include "ui/base/x/x11_util.h"
 
@@ skipping 50 matching lines @@
        FROM_HERE, NewRunnableFunction(LogErrorEventDescription, d, *e));
   return 0;
 }
 
 int DefaultX11IOErrorHandler(Display* d) {
   // If there's an IO error it likely means the X server has gone away
   LOG(ERROR) << "X IO Error detected";
   _exit(1);
 }
 
+XErrorHandler current_error_handler = DefaultX11ErrorHandler;
+
+XErrorEvent* last_error_event = NULL;

[Ami GONE FROM CHROMIUM, 2011/09/20 23:45:51]

Why not simply in-line?
static XErrorEvent last_error_event = { 0, NULL, 0, 0, 0, 0, 0 };
and you can check last_error_event.display==NULL to see if it's been set?

That makes BaseX11ErrorHandler simpler: no need to new() anything, and no need
to commentate about leaking (b/c it's a file-static; it's *supposed* to leak!).

[dominich, 2011/09/21 19:47:30]

I wasn't sure if that was allowed, given that theoretically XErrorEvent could
become non-POD in the future. It is much cleaner though and it should be
relatively obvious from a compile error should the definition of XErrorEvent
change.

+
+int BaseX11ErrorHandler(Display* d, XErrorEvent* e) {
+  // If an error is reported but never checked, this will leak.
+  if (!last_error_event)
+    last_error_event = new XErrorEvent();
+  *last_error_event = *e;
+  return current_error_handler(d, e);
+}
+
 // Note: The caller should free the resulting value data.
 bool GetProperty(XID window, const std::string& property_name, long max_length,
                  Atom* type, int* format, unsigned long* num_items,
                  unsigned char** property) {
   Atom property_atom = gdk_x11_get_xatom_by_name_for_display(
       gdk_display_get_default(), property_name.c_str());
 
   unsigned long remaining_bytes = 0;
   return XGetWindowProperty(GetXDisplay(),
                             window,
                             property_atom,
                             0,          // offset into property data to read
                             max_length, // max length to get
                             False,      // deleted
                             AnyPropertyType,
                             type,
                             format,
                             num_items,
                             &remaining_bytes,
                             property);
 }
 
+std::string BuildX11ErrorString(Display* dpy, const XErrorEvent& error_event) {

[Ami GONE FROM CHROMIUM, 2011/09/20 23:45:51]

|dpy| is unnecessary since you can use |error_event.display|, no?

[dominich, 2011/09/21 19:47:30]

Yes, but adding a DCHECK that these are the same. I don't think they ever can be
different, but I'm not 100% certain.

+  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::UintToString(error_event.request_code);
+    XGetErrorDatabaseText(
+        dpy, "XRequest", num.c_str(), "Unknown", request_str,
+        sizeof(request_str));
+  } else {
+    int num_ext;
+    char** 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 = StringPrintf(
+            "%s.%d", ext_list[i], error_event.minor_code);
+        XGetErrorDatabaseText(
+            dpy, "XRequest", msg.c_str(), "Unknown", request_str,
+            sizeof(request_str));
+        break;
+      }
+    }
+    XFreeExtensionList(ext_list);
+  }
+
+  std::ostringstream error_ss;
+  error_ss << "X Error detected: "
+           << "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 << ")";
+  return error_ss.str();
+}
+
 }  // namespace
 
 bool XDisplayExists() {
   return (gdk_display_get_default() != NULL);
 }
 
 Display* GetXDisplay() {
   return base::MessagePumpForUI::GetDefaultXDisplay();
 }
 
@@ skipping 569 matching lines @@
   event.xclient.format = 32;
   event.xclient.data.l[0] = desktop;
   event.xclient.data.l[1] = 1;  // source indication
 
   int result = XSendEvent(GetXDisplay(), GetX11RootWindow(), False,
                           SubstructureNotifyMask, &event);
   return result == Success;
 }
 
 void SetDefaultX11ErrorHandlers() {
+  XSetErrorHandler(BaseX11ErrorHandler);
   SetX11ErrorHandlers(NULL, NULL);
 }
 
 bool IsX11WindowFullScreen(XID window) {
   // First check if _NET_WM_STATE property contains _NET_WM_STATE_FULLSCREEN.
   static Atom atom = gdk_x11_get_xatom_by_name_for_display(
       gdk_display_get_default(), "_NET_WM_STATE_FULLSCREEN");
 
   std::vector<Atom> atom_properties;
   if (GetAtomArrayProperty(window,
@@ skipping 11 matching lines @@
   gfx::Rect window_rect;
   if (!ui::GetWindowRect(window, &window_rect))
     return false;
 
   return monitor_rect.x == window_rect.x() &&
          monitor_rect.y == window_rect.y() &&
          monitor_rect.width == window_rect.width() &&
          monitor_rect.height == window_rect.height();
 }
 
+void CheckFailOnX11Error() {
+  DCHECK(XDisplayExists());
+  Display* display = GetXDisplay();
+  XSync(display, False);

[Ami GONE FROM CHROMIUM, 2011/09/20 23:45:51]

The Xlib calls you make here and from BuildX11ErrorString() tell me that this
function must be called on the (only) thread talking to Xlib.  What
enforces/documents this?

(besides xlib's thread-unsafety, there's the fact that the mechanism used to
store/retrieve the last error isn't thread-safe; so access from multiple threads
is likely a bug)

[dominich, 2011/09/21 19:47:30]

Nothing enforces it, though it is documented in the x11_util_internal.h for
LogErrorEventDescription. This is called from the error handler but through a
task posted to the message loop.

We could do the same for storing and retrieving the last error, but it would
require call-sites to wait for that task to complete before continuing which
will complicate the initialization code significantly. Another option is to
determine which thread it can be safely called on and DCHECK that it's being
called from that thread only. This is more fragile but is used in other places
in the code-base for similar reasons.

I think the second option is the best option, even though it is fragile as it is
less disruptive to the call-sites.

[Ami GONE FROM CHROMIUM, 2011/09/21 22:21:42]

I think you're talking about out-of-line execution (which is fine).
My point was about thread-safety; none of this is safe to call from any thread
other than the Xlib thread.  Maybe that's obvious from the "ui" namespace?  IDK.

+  CHECK(!last_error_event) << BuildX11ErrorString(display, *last_error_event);
+  delete last_error_event;
+  last_error_event = NULL;
+}
+
 // ----------------------------------------------------------------------------
 // These functions are declared in x11_util_internal.h because they require
 // XLib.h to be included, and it conflicts with many other headers.
 XRenderPictFormat* GetRenderARGB32Format(Display* dpy) {
   static XRenderPictFormat* pictformat = NULL;
   if (pictformat)
     return pictformat;
 
   // First look for a 32-bit format which ignores the alpha value
   XRenderPictFormat templ;
@@ skipping 58 matching lines @@
     // always blowing away the cache. If we are, then we should figure out why
     // and make it bigger.
     NOTREACHED();
   }
 
   return pictformat;
 }
 
 void SetX11ErrorHandlers(XErrorHandler error_handler,
                          XIOErrorHandler io_error_handler) {
-  XSetErrorHandler(error_handler ? error_handler : DefaultX11ErrorHandler);
-  XSetIOErrorHandler(
-      io_error_handler ? io_error_handler : DefaultX11IOErrorHandler);
+  current_error_handler = error_handler ?
+      error_handler : DefaultX11ErrorHandler;
+  XSetIOErrorHandler(io_error_handler ?
+                     io_error_handler : DefaultX11IOErrorHandler);
 }
 
 void LogErrorEventDescription(Display* 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::UintToString(error_event.request_code);
-    XGetErrorDatabaseText(
-        dpy, "XRequest", num.c_str(), "Unknown", request_str,
-        sizeof(request_str));
-  } else {
-    int num_ext;
-    char** 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 = StringPrintf(
-            "%s.%d", ext_list[i], error_event.minor_code);
-        XGetErrorDatabaseText(
-            dpy, "XRequest", msg.c_str(), "Unknown", request_str,
-            sizeof(request_str));
-        break;
-      }
-    }
-    XFreeExtensionList(ext_list);
-  }
-
-  LOG(ERROR) 
-      << "X Error detected: "
-      << "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 << ")";
+  LOG(ERROR) << BuildX11ErrorString(dpy, error_event);
 }
 
 // ----------------------------------------------------------------------------
 // End of x11_util_internal.h
 
 
 }  // namespace ui