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

ui/base/x/x11_util.cc

Index: ui/base/x/x11_util.cc
diff --git a/ui/base/x/x11_util.cc b/ui/base/x/x11_util.cc
index 82cdab78f54bc87e603501517f112e3db282c54e..b587a07f62b0e23313bcf80022c6e8e033ea7b3e 100644
--- a/ui/base/x/x11_util.cc
+++ b/ui/base/x/x11_util.cc
@@ -68,6 +68,18 @@ int DefaultX11IOErrorHandler(Display* d) {
   _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,
@@ -90,6 +102,49 @@ bool GetProperty(XID window, const std::string& property_name, long max_length,
                             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() {
@@ -679,6 +734,7 @@ bool ChangeWindowDesktop(XID window, XID destination) {
 }
 
 void SetDefaultX11ErrorHandlers() {
+  XSetErrorHandler(BaseX11ErrorHandler);
   SetX11ErrorHandlers(NULL, NULL);
 }
 
@@ -710,6 +766,15 @@ bool IsX11WindowFullScreen(XID window) {
          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.
@@ -788,51 +853,15 @@ XRenderPictFormat* GetRenderVisualFormat(Display* dpy, Visual* visual) {
 
 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);
 }
 
 // ----------------------------------------------------------------------------