Revert 102978 - Change X11 error handler override to allow easy X11 error checking.

BUG=None
TEST=None


Review URL: http://codereview.chromium.org/7889040

TBR=dominich@chromium.org
Review URL: http://codereview.chromium.org/8052015

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@102985 0039d316-1c4b-4281-b951-d872f2087c98

gpu/DEPS

@@ -8,8 +8,6 @@ include_rules = [
   "+../GLES2",
   "+../service",
 
-  # For ui::CheckFailOnReportedX11Error
-  "+ui/base",
   # For gfx::PluginWindowHandle
   "+ui/gfx",
 ]

gpu/command_buffer/common/gl_mock.h

@@ -9,11 +9,6 @@
 #define GPU_COMMAND_BUFFER_COMMON_GL_MOCK_H_
 #pragma once
 
-// Include gtest.h out of order because <X11/X.h> #define's Bool & None, which
-// gtest uses as struct names (inside a namespace).  This means that
-// #include'ing gtest after anything that pulls in X.h fails to compile.
-// This is http://code.google.com/p/googletest/issues/detail?id=371
-#include "testing/gtest/include/gtest/gtest.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "ui/gfx/gl/gl_interface.h"
 
@@ -455,3 +450,4 @@ class MockGLInterface : public GLInterface {
 }  // namespace gfx
 
 #endif  // GPU_COMMAND_BUFFER_COMMON_GL_MOCK_H_
+

gpu/command_buffer/service/buffer_manager_unittest.cc

@@ -2,13 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Include gtest.h out of order because <X11/X.h> #define's Bool & None, which
-// gtest uses as struct names (inside a namespace).  This means that
-// #include'ing gtest after anything that pulls in X.h fails to compile.
-// This is http://code.google.com/p/googletest/issues/detail?id=371
-#include "testing/gtest/include/gtest/gtest.h"
 #include "gpu/command_buffer/service/buffer_manager.h"
 #include "gpu/command_buffer/common/gl_mock.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 namespace gpu {
 namespace gles2 {
@@ -227,3 +223,5 @@ TEST_F(BufferManagerTest, GetMaxValueForRangeUint32) {
 
 }  // namespace gles2
 }  // namespace gpu
+
+

gpu/command_buffer/service/framebuffer_manager_unittest.cc

@@ -2,15 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Include gtest.h out of order because <X11/X.h> #define's Bool & None, which
-// gtest uses as struct names (inside a namespace).  This means that
-// #include'ing gtest after anything that pulls in X.h fails to compile.
-// This is http://code.google.com/p/googletest/issues/detail?id=371
-#include "testing/gtest/include/gtest/gtest.h"
-#include "gpu/command_buffer/common/gl_mock.h"
 #include "gpu/command_buffer/service/framebuffer_manager.h"
 #include "gpu/command_buffer/service/feature_info.h"
 
+#include "gpu/command_buffer/common/gl_mock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
 namespace gpu {
 namespace gles2 {
 
@@ -367,3 +364,5 @@ TEST_F(FramebufferInfoTest, AttachTexture) {
 
 }  // namespace gles2
 }  // namespace gpu
+
+

gpu/command_buffer/service/gles2_cmd_decoder.cc

@@ -15,7 +15,6 @@
 #include "base/atomicops.h"
 #include "base/at_exit.h"
 #include "base/callback.h"
-#include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "build/build_config.h"
@@ -41,13 +40,6 @@
 #include "ui/gfx/gl/gl_implementation.h"
 #include "ui/gfx/gl/gl_surface.h"
 
-#if defined(USE_X11)
-#include "ui/base/x/x11_util_internal.h"
-#define CHECK_X_ERROR() ui::CheckForReportedX11Error()
-#else   // USE_X11
-#define CHECK_X_ERROR() void(0)
-#endif  // USE_X11
-
 #if !defined(GL_DEPTH24_STENCIL8)
 #define GL_DEPTH24_STENCIL8 0x88F0
 #endif
@@ -1740,8 +1732,6 @@ bool GLES2DecoderImpl::Initialize(
   // Take ownership of the GLContext.
   context_ = context;
 
-  CHECK_X_ERROR();
-
   if (!MakeCurrent()) {
     LOG(ERROR) << "GLES2DecoderImpl::Initialize failed because "
                << "MakeCurrent failed.";
@@ -1756,7 +1746,6 @@ bool GLES2DecoderImpl::Initialize(
     return false;
   }
 
-  CHECK_X_ERROR();
   CHECK_GL_ERROR();
   disallowed_features_ = disallowed_features;
 

gpu/command_buffer/service/id_manager_unittest.cc

@@ -1,13 +1,9 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
-// Include gtest.h out of order because <X11/X.h> #define's Bool & None, which
-// gtest uses as struct names (inside a namespace).  This means that
-// #include'ing gtest after anything that pulls in X.h fails to compile.
-// This is http://code.google.com/p/googletest/issues/detail?id=371
-#include "testing/gtest/include/gtest/gtest.h"
 #include "gpu/command_buffer/service/id_manager.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 namespace gpu {
 namespace gles2 {
@@ -76,3 +72,5 @@ TEST_F(IdManagerTest, Basic) {
 
 }  // namespace gles2
 }  // namespace gpu
+
+

gpu/command_buffer/service/program_manager_unittest.cc

@@ -2,11 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Include gtest.h out of order because <X11/X.h> #define's Bool & None, which
-// gtest uses as struct names (inside a namespace).  This means that
-// #include'ing gtest after anything that pulls in X.h fails to compile.
-// This is http://code.google.com/p/googletest/issues/detail?id=371
-#include "testing/gtest/include/gtest/gtest.h"
 #include "gpu/command_buffer/service/program_manager.h"
 
 #include <algorithm>
@@ -905,3 +900,5 @@ TEST_F(ProgramManagerWithShaderTest, ProgramInfoGetProgramInfo) {
 
 }  // namespace gles2
 }  // namespace gpu
+
+

gpu/command_buffer/service/renderbuffer_manager_unittest.cc

@@ -1,12 +1,7 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
-// Include gtest.h out of order because <X11/X.h> #define's Bool & None, which
-// gtest uses as struct names (inside a namespace).  This means that
-// #include'ing gtest after anything that pulls in X.h fails to compile.
-// This is http://code.google.com/p/googletest/issues/detail?id=371
-#include "testing/gtest/include/gtest/gtest.h"
 #include "gpu/command_buffer/service/renderbuffer_manager.h"
 
 #include "gpu/command_buffer/common/gl_mock.h"
@@ -122,3 +117,5 @@ TEST_F(RenderbufferManagerTest, RenderbufferInfo) {
 
 }  // namespace gles2
 }  // namespace gpu
+
+

gpu/command_buffer/service/shader_manager_unittest.cc

@@ -2,16 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Include gtest.h out of order because <X11/X.h> #define's Bool & None, which
-// gtest uses as struct names (inside a namespace).  This means that
-// #include'ing gtest after anything that pulls in X.h fails to compile.
-// This is http://code.google.com/p/googletest/issues/detail?id=371
-#include "testing/gtest/include/gtest/gtest.h"
 #include "gpu/command_buffer/service/shader_manager.h"
 
 #include "base/memory/scoped_ptr.h"
 #include "gpu/command_buffer/common/gl_mock.h"
 #include "gpu/command_buffer/service/mocks.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::Return;
 using ::testing::ReturnRef;
@@ -246,3 +242,5 @@ TEST_F(ShaderManagerTest, ShaderInfoUseCount) {
 
 }  // namespace gles2
 }  // namespace gpu
+
+

gpu/command_buffer/service/texture_manager_unittest.cc

@@ -2,17 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Include gtest.h out of order because <X11/X.h> #define's Bool & None, which
-// gtest uses as struct names (inside a namespace).  This means that
-// #include'ing gtest after anything that pulls in X.h fails to compile.
-// This is http://code.google.com/p/googletest/issues/detail?id=371
-#include "testing/gtest/include/gtest/gtest.h"
 #include "gpu/command_buffer/service/texture_manager.h"
 
 #include "base/memory/scoped_ptr.h"
 #include "gpu/command_buffer/common/gl_mock.h"
 #include "gpu/command_buffer/service/feature_info.h"
 #include "gpu/command_buffer/service/test_helper.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::Pointee;
 using ::testing::_;
@@ -731,3 +727,5 @@ TEST_F(TextureInfoTest, EGLImageExternal) {
 
 }  // namespace gles2
 }  // namespace gpu
+
+

gpu/command_buffer/service/vertex_attrib_manager_unittest.cc

@@ -2,17 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Include gtest.h out of order because <X11/X.h> #define's Bool & None, which
-// gtest uses as struct names (inside a namespace).  This means that
-// #include'ing gtest after anything that pulls in X.h fails to compile.
-// This is http://code.google.com/p/googletest/issues/detail?id=371
-#include "testing/gtest/include/gtest/gtest.h"
 #include "gpu/command_buffer/service/vertex_attrib_manager.h"
 
 #include "base/memory/scoped_ptr.h"
 #include "gpu/command_buffer/common/gl_mock.h"
 #include "gpu/command_buffer/service/feature_info.h"
 #include "gpu/command_buffer/service/test_helper.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::Pointee;
 using ::testing::_;
@@ -180,3 +176,5 @@ TEST_F(VertexAttribManagerTest, CanAccess) {
 
 }  // namespace gles2
 }  // namespace gpu
+
+

ui/base/x/x11_util.cc

@@ -65,8 +65,6 @@ CachedPictFormats* get_cached_pict_formats() {
 const size_t kMaxCacheSize = 5;
 
 int DefaultX11ErrorHandler(Display* d, XErrorEvent* e) {
-  DCHECK(!MessageLoop::current() ||
-         MessageLoop::current()->type() == MessageLoop::TYPE_UI);
   MessageLoop::current()->PostTask(
        FROM_HERE, NewRunnableFunction(LogErrorEventDescription, d, *e));
   return 0;
@@ -78,14 +76,6 @@ int DefaultX11IOErrorHandler(Display* d) {
   _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);
-}
-
 Atom GetAtom(const char* name) {
 #if defined(TOOLKIT_USES_GTK)
   return gdk_x11_get_xatom_by_name_for_display(
@@ -115,49 +105,6 @@ bool GetProperty(XID window, const std::string& property_name, long max_length,
                             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() {
@@ -748,7 +695,6 @@ bool ChangeWindowDesktop(XID window, XID destination) {
 }
 
 void SetDefaultX11ErrorHandlers() {
-  XSetErrorHandler(BaseX11ErrorHandler);
   SetX11ErrorHandlers(NULL, NULL);
 }
 
@@ -784,15 +730,6 @@ bool IsX11WindowFullScreen(XID window) {
 #endif
 }
 
-void CheckForReportedX11Error() {
-  DCHECK(!MessageLoop::current() ||
-         MessageLoop::current()->type() == MessageLoop::TYPE_UI);
-  if (!last_error_event.display)
-    return;
-  XSync(last_error_event.display, False);
-  LOG(FATAL) << BuildX11ErrorString(last_error_event);
-}
-
 // ----------------------------------------------------------------------------
 // These functions are declared in x11_util_internal.h because they require
 // XLib.h to be included, and it conflicts with many other headers.
@@ -871,21 +808,51 @@ XRenderPictFormat* GetRenderVisualFormat(Display* dpy, Visual* visual) {
 
 void SetX11ErrorHandlers(XErrorHandler error_handler,
                          XIOErrorHandler io_error_handler) {
-  DCHECK(!MessageLoop::current() ||
-         MessageLoop::current()->type() == MessageLoop::TYPE_UI);
-  current_error_handler = error_handler ?
-      error_handler : DefaultX11ErrorHandler;
-  XSetIOErrorHandler(io_error_handler ?
-                     io_error_handler : DefaultX11IOErrorHandler);
+  XSetErrorHandler(error_handler ? error_handler : DefaultX11ErrorHandler);
+  XSetIOErrorHandler(
+      io_error_handler ? io_error_handler : DefaultX11IOErrorHandler);
 }
 
 void LogErrorEventDescription(Display* dpy,
                               const XErrorEvent& error_event) {
-  DCHECK(!MessageLoop::current() ||
-         MessageLoop::current()->type() == MessageLoop::TYPE_UI);
-  DCHECK_EQ(dpy, error_event.display)
-      << "Attempt to log error for mismatching X11 display.";
-  LOG(ERROR) << BuildX11ErrorString(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 << ")";
 }
 
 // ----------------------------------------------------------------------------

ui/base/x/x11_util_internal.h

@@ -50,11 +50,6 @@ UI_EXPORT void SetX11ErrorHandlers(XErrorHandler error_handler,
 UI_EXPORT void LogErrorEventDescription(Display* dpy,
                                         const XErrorEvent& error_event);
 
-// LOG(FATAL) if an X11 errors has been reported. Uses XSync to ensure that all
-// requests have been received and processed by the X server and uses the X
-// display contained in the reported XErrorEvent.
-UI_EXPORT void CheckForReportedX11Error();
-
 }  // namespace ui
 
 #endif  // UI_BASE_X_X11_UTIL_INTERNAL_H_

ui/gfx/gl/gl_bindings.h

@@ -26,6 +26,12 @@
 #elif defined(USE_X11)
 #include <GL/glx.h>
 #include <GL/glxext.h>
+
+// Undefine some macros defined by X headers. This is why this file should only
+// be included in .cc files.
+#undef Bool
+#undef None
+#undef Status
 #endif
 
 #if defined(OS_WIN)

ui/gfx/gl/gl_context_glx.cc

@@ -11,7 +11,6 @@ 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_internal.h"
 #include "ui/gfx/gl/gl_bindings.h"
 #include "ui/gfx/gl/gl_implementation.h"
 #include "ui/gfx/gl/gl_surface_glx.h"
@@ -89,8 +88,6 @@ bool GLContextGLX::Initialize(GLSurface* compatible_surface) {
     }
   }
 
-  ui::CheckForReportedX11Error();
-
   if (!context_) {
     // The means by which the context is created depends on whether
     // the drawable type works reliably with GLX 1.3. If it does not
@@ -116,8 +113,6 @@ bool GLContextGLX::Initialize(GLSurface* compatible_surface) {
         return false;
       }
 
-      ui::CheckForReportedX11Error();
-
       XVisualInfo visual_info_template;
       visual_info_template.visualid = XVisualIDFromVisual(attributes.visual);
 
@@ -141,7 +136,6 @@ bool GLContextGLX::Initialize(GLSurface* compatible_surface) {
           True);
     }
   }
-  ui::CheckForReportedX11Error();
 
   if (!context_) {
     LOG(ERROR) << "Couldn't create GL context.";