Set Xlib error handler after GTK X11 initialization

The default GTK error handler exits the process, which we don't want.
We used to reset the error handler at some point after GTK
initialization, but this became lost after r815319.

Also refactor the XlibLoader instance to be global so it doesn't have
to be created multiple times for each connection.

R=sky

Bug: 1145929
Change-Id: I73a3f27551534c2262bd8994d33e30fc2a0a4e27
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2527743
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825585}

ui/gfx/x/connection.cc

@@ -189,8 +189,7 @@ XlibDisplayWrapper Connection::GetXlibDisplay(XlibDisplayType type) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!xlib_display_)
     xlib_display_ = base::WrapUnique(new XlibDisplay(display_string_));
-  return XlibDisplayWrapper(xlib_display_->xlib_loader_.get(),
-                            xlib_display_->display_, type);
+  return XlibDisplayWrapper(xlib_display_->display_, type);
 }
 
 Connection::Request::Request(unsigned int sequence,

ui/gfx/x/xlib_support.cc

@@ -7,6 +7,7 @@
 #include "base/check.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
+#include "base/no_destructor.h"
 #include "library_loaders/xlib_loader.h"
 
 namespace x11 {
@@ -18,41 +19,56 @@ int XlibErrorHandler(void*, void*) {
   return 0;
 }
 
+XlibLoader* GetXlibLoader() {
+  static base::NoDestructor<XlibLoader> xlib_loader;
+  return xlib_loader.get();
+}
+
 }  // namespace
 
-DISABLE_CFI_ICALL
-XlibDisplay::XlibDisplay(const std::string& address) {
-  xlib_loader_ = std::make_unique<XlibLoader>();
-  CHECK(xlib_loader_->Load("libX11.so.6"));
+void InitXlib() {
+  auto* xlib_loader = GetXlibLoader();
+  if (xlib_loader->loaded())
+    return;
+
+  CHECK(xlib_loader->Load("libX11.so.6"));
 
-  CHECK(xlib_loader_->XInitThreads());
+  CHECK(xlib_loader->XInitThreads());
 
   // The default Xlib error handler calls exit(1), which we don't want.  This
   // shouldn't happen in the browser process since only XProto requests are
   // made, but in the GPU process, GLX can make Xlib requests, so setting an
   // error handler is necessary.  Importantly, there's also an IO error handler,
   // and Xlib always calls exit(1) with no way to change this behavior.
-  xlib_loader_->XSetErrorHandler(XlibErrorHandler);
+  SetXlibErrorHandler();
+}
+
+void SetXlibErrorHandler() {
+  GetXlibLoader()->XSetErrorHandler(XlibErrorHandler);
+}
+
+DISABLE_CFI_ICALL
+XlibDisplay::XlibDisplay(const std::string& address) {
+  InitXlib();
 
-  display_ =
-      xlib_loader_->XOpenDisplay(address.empty() ? nullptr : address.c_str());
+  display_ = GetXlibLoader()->XOpenDisplay(address.empty() ? nullptr
+                                                           : address.c_str());
 }
 
 DISABLE_CFI_ICALL
 XlibDisplay::~XlibDisplay() {
   if (display_)
-    xlib_loader_->XCloseDisplay(display_);
+    GetXlibLoader()->XCloseDisplay(display_);
 }
 
 DISABLE_CFI_ICALL
-XlibDisplayWrapper::XlibDisplayWrapper(XlibLoader* xlib_loader,
-                                       struct _XDisplay* display,
+XlibDisplayWrapper::XlibDisplayWrapper(struct _XDisplay* display,
                                        XlibDisplayType type)
-    : xlib_loader_(xlib_loader), display_(display), type_(type) {
+    : display_(display), type_(type) {
   if (!display_)
     return;
   if (type == XlibDisplayType::kSyncing)
-    xlib_loader_->XSynchronize(display_, true);
+    GetXlibLoader()->XSynchronize(display_, true);
 }
 
 DISABLE_CFI_ICALL
@@ -60,25 +76,21 @@ XlibDisplayWrapper::~XlibDisplayWrapper() {
   if (!display_)
     return;
   if (type_ == XlibDisplayType::kFlushing)
-    xlib_loader_->XFlush(display_);
+    GetXlibLoader()->XFlush(display_);
   else if (type_ == XlibDisplayType::kSyncing)
-    xlib_loader_->XSynchronize(display_, false);
+    GetXlibLoader()->XSynchronize(display_, false);
 }
 
 XlibDisplayWrapper::XlibDisplayWrapper(XlibDisplayWrapper&& other) {
-  xlib_loader_ = other.xlib_loader_;
   display_ = other.display_;
   type_ = other.type_;
-  other.xlib_loader_ = nullptr;
   other.display_ = nullptr;
   other.type_ = XlibDisplayType::kNormal;
 }
 
 XlibDisplayWrapper& XlibDisplayWrapper::operator=(XlibDisplayWrapper&& other) {
-  xlib_loader_ = other.xlib_loader_;
   display_ = other.display_;
   type_ = other.type_;
-  other.xlib_loader_ = nullptr;
   other.display_ = nullptr;
   other.type_ = XlibDisplayType::kNormal;
   return *this;

ui/gfx/x/xlib_support.h

@@ -10,7 +10,6 @@
 #include "base/component_export.h"
 
 struct _XDisplay;
-class XlibLoader;
 
 namespace x11 {
 
@@ -26,6 +25,12 @@ enum class XlibDisplayType {
   kSyncing,
 };
 
+// Loads Xlib, initializes threads, and sets a default error handler.
+COMPONENT_EXPORT(X11) void InitXlib();
+
+// Sets an async error handler which only logs an error message.
+COMPONENT_EXPORT(X11) void SetXlibErrorHandler();
+
 // A scoped Xlib display.
 class COMPONENT_EXPORT(X11) XlibDisplay {
  public:
@@ -38,7 +43,6 @@ class COMPONENT_EXPORT(X11) XlibDisplay {
   explicit XlibDisplay(const std::string& address);
 
   struct _XDisplay* display_ = nullptr;
-  std::unique_ptr<XlibLoader> xlib_loader_;
 };
 
 // A temporary wrapper around an unowned Xlib display that adds behavior
@@ -56,13 +60,10 @@ class COMPONENT_EXPORT(X11) XlibDisplayWrapper {
   XlibDisplayWrapper& operator=(XlibDisplayWrapper&& other);
 
  private:
-  XlibDisplayWrapper(XlibLoader* xlib_loader,
-                     struct _XDisplay* display,
-                     XlibDisplayType type);
+  XlibDisplayWrapper(struct _XDisplay* display, XlibDisplayType type);
 
   friend class Connection;
 
-  XlibLoader* xlib_loader_;
   struct _XDisplay* display_;
   XlibDisplayType type_;
 };

ui/gtk/x/gtk_ui_delegate_x11.cc

@@ -10,6 +10,7 @@
 #include "ui/base/x/x11_util.h"
 #include "ui/events/platform/x11/x11_event_source.h"
 #include "ui/gfx/native_widget_types.h"
+#include "ui/gfx/x/xlib_support.h"
 #include "ui/gfx/x/xproto.h"
 #include "ui/gtk/x/gtk_event_loop_x11.h"
 #include "ui/platform_window/x11/x11_window.h"
@@ -31,6 +32,7 @@ GtkUiDelegateX11::GtkUiDelegateX11(x11::Connection* connection)
     : connection_(connection) {
   DCHECK(connection_);
   gdk_set_allowed_backends("x11");
+  x11::InitXlib();
 }
 
 GtkUiDelegateX11::~GtkUiDelegateX11() = default;
@@ -38,6 +40,11 @@ GtkUiDelegateX11::~GtkUiDelegateX11() = default;
 void GtkUiDelegateX11::OnInitialized() {
   // Ensure the singleton instance of GtkEventLoopX11 is created and started.
   GtkEventLoopX11::EnsureInstance();
+
+  // GTK sets an Xlib error handler that exits the process on any async errors.
+  // We don't want this behavior, so reset the error handler to something that
+  // just logs the error.
+  x11::SetXlibErrorHandler();
 }
 
 GdkKeymap* GtkUiDelegateX11::GetGdkKeymap() {