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

ui/gfx/gl/gl_context_glx.cc

Index: ui/gfx/gl/gl_context_glx.cc
diff --git a/ui/gfx/gl/gl_context_glx.cc b/ui/gfx/gl/gl_context_glx.cc
index a23f1a9b988063d7c7f84db3253fa23d6b3b4bc9..606c62fdb6568c5e497ab09ee9546a6f817e621a 100644
--- a/ui/gfx/gl/gl_context_glx.cc
+++ b/ui/gfx/gl/gl_context_glx.cc
@@ -11,6 +11,8 @@ extern "C" {
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "third_party/mesa/MesaLib/include/GL/osmesa.h"
+#include "ui/base/x/x11_util.h"
+#include "ui/base/x/x11_util_internal.h"
 #include "ui/gfx/gl/gl_bindings.h"
 #include "ui/gfx/gl/gl_implementation.h"
 #include "ui/gfx/gl/gl_surface_glx.h"
@@ -42,6 +44,14 @@ bool IsCompositingWindowManagerActive(Display* display) {
   return XGetSelectionOwner(display, net_wm_cm_s0) != static_cast<Atom>(kNone);
 }
 
+bool g_has_x_error_ = false;
+
+int GLContextGLXErrorHandler(Display* d, XErrorEvent* e) {
+  g_has_x_error_ = true;
+  ui::LogErrorEventDescription(d, *e);

[Ami GONE FROM CHROMIUM, 2011/09/14 17:11:13]

not legit per NOTE @ x11_util_internal.h:46.

[dominich, 2011/09/14 19:55:06]

Done.

+  return 0;
+}
+
 }  // namespace anonymous
 
 GLContextGLX::GLContextGLX(GLShareGroup* share_group)
@@ -102,6 +112,10 @@ bool GLContextGLX::Initialize(GLSurface* compatible_surface) {
     } else {
       Display* display = GLSurfaceGLX::GetDisplay();
 
+      // We only need to override the non-fatal X error handler.
+      ui::ScopedX11ErrorHandlerOverride handler_override(
+          GLContextGLXErrorHandler, NULL);
+

[marcheu, 2011/09/14 17:02:43]

Why not do this earlier in the function? glX functions can cause X errors just
like other X functions.

[dominich, 2011/09/14 19:55:06]

Done.

       // Get the visuals for the X drawable.
       XWindowAttributes attributes;
       if (!XGetWindowAttributes(
@@ -113,6 +127,10 @@ bool GLContextGLX::Initialize(GLSurface* compatible_surface) {
         return false;
       }
 
+      // Allow any errors to be caught.
+      XSync(display, False);
+      CHECK(!g_has_x_error_);

[Ami GONE FROM CHROMIUM, 2011/09/14 17:11:13]

If this is all you care about, why not make the default handler maintain the
last-reported XErrorEvent and add an accessor for it?

[dominich, 2011/09/14 17:15:57]

There are two sets of default handlers, one of which is set at the browser
level. Relying on the default handler not having been overridden is fragile.

[Ami GONE FROM CHROMIUM, 2011/09/14 17:20:50]

An alternative to depending on a specific handler is having the
SetX11ErrorHandlers() machinery always execute a base handler (such as tracking
the last error) and then dispatch the last-set handler.  Just throwing ideas out
there.

The idiom of a scoped error handler seems nice to me, but the fact that even in
its first use you don't want to use it as such makes me suspicious that this
isn't an optimal setup.

+

[marcheu, 2011/09/14 17:02:43]

Why not do this later in the function? There are more X calls and errors to
catch? Unless you want to insert more sync points of course.

[dominich, 2011/09/14 19:55:06]

Done.

       XVisualInfo visual_info_template;
       visual_info_template.visualid = XVisualIDFromVisual(attributes.visual);