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 = {0, NULL, 0, 0, 0, 0, 0 };
+
+int BaseX11ErrorHandler(Display* d, XErrorEvent* e) {
+  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(const XErrorEvent& error_event) {
+  char error_str[256];
+  char request_str[256];
+
+  XGetErrorText(error_event.display, 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(error_event.display, "XRequest", num.c_str(),
+                          "Unknown", request_str, sizeof(request_str));
+  } else {
+    int num_ext;
+    char** ext_list = XListExtensions(error_event.display, &num_ext);
+
+    for (int i = 0; i < num_ext; i++) {
+      int ext_code, first_event, first_error;
+      XQueryExtension(error_event.display, 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(error_event.display, "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 CheckFailOnReportedX11Error() {
+  if (last_error_event.display) {

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

Reversing the test lets you early-return and de-indent the rest of the function.

[dominich, 2011/09/22 17:00:15]

Done.

+    XSync(last_error_event.display, False);
+    LOG(FATAL) << BuildX11ErrorString(last_error_event);
+    last_error_event.display = NULL;
+  }

[sadrul, 2011/09/21 21:50:34]

Can this return a bool depending on whether there was an error or not? This
could perhaps then replace the gdk_error_trap_push/gdk_error_trap_pop calls in a
few places in this file?

[dominich, 2011/09/21 22:05:16]

It could, but there's a good chance this will have to check for an error
asynchronously due to Xlib not being thread-safe. In that case returning a value
would be useless.

One thing we might have to do is have a callback in the calling code that is
called with the success/failure value.

(insert xzibit-meme here).

+}
+
 // ----------------------------------------------------------------------------
 // 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 << ")";
+  DCHECK(dpy == error_event.display)

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

DCHECK_EQ work?

+      << "Attempt to log error for mismatching X11 display.";
+  LOG(ERROR) << BuildX11ErrorString(error_event);
 }
 
 // ----------------------------------------------------------------------------
 // End of x11_util_internal.h
 
 
 }  // namespace ui