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

ui/gfx/gl/gl_context_glx.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.
 
 extern "C" {
 #include <X11/Xlib.h>
 }
 
 #include "ui/gfx/gl/gl_context_glx.h"
 
 #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"
 
 namespace gfx {
 
 namespace {
 
 // scoped_ptr functor for XFree(). Use as follows:
 //   scoped_ptr_malloc<XVisualInfo, ScopedPtrXFree> foo(...);
@@ skipping 11 matching lines @@
   static Atom net_wm_cm_s0 = kNone;
   if (net_wm_cm_s0 == static_cast<Atom>(kNone)) {
     net_wm_cm_s0 = XInternAtom(display, "_NET_WM_CM_S0", True);
   }
   if (net_wm_cm_s0 == static_cast<Atom>(kNone)) {
     return false;
   }
   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)
   : GLContext(share_group),
     context_(NULL) {
 }
 
 GLContextGLX::~GLContextGLX() {
   Destroy();
 }
@@ skipping 40 matching lines @@
     if (surface_glx->IsOffscreen()) {
       context_ = glXCreateNewContext(
           GLSurfaceGLX::GetDisplay(),
           static_cast<GLXFBConfig>(surface_glx->GetConfig()),
           GLX_RGBA_TYPE,
           share_handle,
           True);
     } 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(
           display,
           reinterpret_cast<GLXDrawable>(surface_glx->GetHandle()),
           &attributes)) {
         LOG(ERROR) << "XGetWindowAttributes failed for window " <<
             reinterpret_cast<GLXDrawable>(surface_glx->GetHandle()) << ".";
         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);
 
       int visual_info_count = 0;
       scoped_ptr_malloc<XVisualInfo, ScopedPtrXFree> visual_info_list(
           XGetVisualInfo(display, VisualIDMask,
                          &visual_info_template,
                          &visual_info_count));
 
       DCHECK(visual_info_list.get());
@@ skipping 118 matching lines @@
   }
 
   return GLContext::GetExtensions();
 }
 
 bool GLContextGLX::WasAllocatedUsingARBRobustness() {
   return GLSurfaceGLX::IsCreateContextRobustnessSupported();
 }
 
 }  // namespace gfx