b=1045482 make XErrorTrap installation and removal thread-safe r=jesup a=sylvestre
authorKarl Tomlinson <karlt+@karlt.net>
Fri, 01 Aug 2014 17:58:24 +1200
changeset 217424 396277e5792595100fede96c005155f9ac2f5463
parent 217423 81631d5c98b422e1d6a719ec9311abc83d6d1cea
child 217425 c3ee282759eb68df2f7c00499cb2329a1bf869fe
push id515
push userraliiev@mozilla.com
push dateMon, 06 Oct 2014 12:51:51 +0000
treeherdermozilla-release@267c7a481bef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjesup, sylvestre
bugs1045482
milestone33.0a2
b=1045482 make XErrorTrap installation and removal thread-safe r=jesup a=sylvestre Error handling is now applied to the Display using async_handlers, instead of replacing and trying to reinstate the XSetErrorHandler() global handler for all Xlib Displays. Inspired by use of the same Xlibint.h structures at https://git.gnome.org/browse/gtk+/tree/gdk/x11/gdkasync.c?id=0e1a4248#n252 https://git.gnome.org/browse/gtk+/tree/gdk/x11/gdkasync.c?id=0e1a4248#n150 Compare use of _XAsyncErrorHandler in libX11.
media/webrtc/trunk/webrtc/modules/desktop_capture/x11/x_error_trap.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/x11/x_error_trap.h
--- a/media/webrtc/trunk/webrtc/modules/desktop_capture/x11/x_error_trap.cc
+++ b/media/webrtc/trunk/webrtc/modules/desktop_capture/x11/x_error_trap.cc
@@ -6,64 +6,64 @@
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/modules/desktop_capture/x11/x_error_trap.h"
 
 #include <assert.h>
-
-#if defined(TOOLKIT_GTK)
-#include <gdk/gdk.h>
-#endif  // !defined(TOOLKIT_GTK)
+#include <limits>
 
 namespace webrtc {
 
-namespace {
-
-#if !defined(TOOLKIT_GTK)
+// static
+Bool XErrorTrap::XServerErrorHandler(Display* display, xReply* rep,
+                                     char* /* buf */, int /* len */,
+                                     XPointer data) {
+  XErrorTrap* self = reinterpret_cast<XErrorTrap*>(data);
 
-// TODO(sergeyu): This code is not thread safe. Fix it. Bug 2202.
-static bool g_xserver_error_trap_enabled = false;
-static int g_last_xserver_error_code = 0;
+  if (rep->generic.type != X_Error ||
+      // Overflow-safe last_request_read <= last_ignored_request_ for skipping
+      // async replies from requests before XErrorTrap was created.
+      self->last_ignored_request_ - display->last_request_read <
+      std::numeric_limits<unsigned long>::max() >> 1)
+    return False;
 
-int XServerErrorHandler(Display* display, XErrorEvent* error_event) {
-  assert(g_xserver_error_trap_enabled);
-  g_last_xserver_error_code = error_event->error_code;
-  return 0;
+  self->last_xserver_error_code_ = rep->error.errorCode;
+  return True;
 }
 
-#endif  // !defined(TOOLKIT_GTK)
-
-}  // namespace
-
 XErrorTrap::XErrorTrap(Display* display)
-    : original_error_handler_(NULL),
+    : display_(display),
+      last_xserver_error_code_(0),
       enabled_(true) {
-#if defined(TOOLKIT_GTK)
-  gdk_error_trap_push();
-#else  // !defined(TOOLKIT_GTK)
-  assert(!g_xserver_error_trap_enabled);
-  original_error_handler_ = XSetErrorHandler(&XServerErrorHandler);
-  g_xserver_error_trap_enabled = true;
-  g_last_xserver_error_code = 0;
-#endif  // !defined(TOOLKIT_GTK)
+  // Use async_handlers instead of XSetErrorHandler().  async_handlers can
+  // remain in place and then be safely removed at the right time even if a
+  // handler change happens concurrently on another thread.  async_handlers
+  // are processed first and so can prevent errors reaching the global
+  // XSetErrorHandler handler.  They also will not see errors from or affect
+  // handling of errors on other Displays, which may be processed on other
+  // threads.
+  LockDisplay(display);
+  async_handler_.next = display->async_handlers;
+  async_handler_.handler = XServerErrorHandler;
+  async_handler_.data = reinterpret_cast<XPointer>(this);
+  display->async_handlers = &async_handler_;
+  last_ignored_request_ = display->request;
+  UnlockDisplay(display);
 }
 
 int XErrorTrap::GetLastErrorAndDisable() {
+  assert(enabled_);
   enabled_ = false;
-#if defined(TOOLKIT_GTK)
-  return gdk_error_trap_push();
-#else  // !defined(TOOLKIT_GTK)
-  assert(g_xserver_error_trap_enabled);
-  XSetErrorHandler(original_error_handler_);
-  g_xserver_error_trap_enabled = false;
-  return g_last_xserver_error_code;
-#endif  // !defined(TOOLKIT_GTK)
+  LockDisplay(display_);
+  DeqAsyncHandler(display_, &async_handler_);
+  UnlockDisplay(display_);
+  return last_xserver_error_code_;
 }
 
 XErrorTrap::~XErrorTrap() {
   if (enabled_)
     GetLastErrorAndDisable();
 }
 
 }  // namespace webrtc
--- a/media/webrtc/trunk/webrtc/modules/desktop_capture/x11/x_error_trap.h
+++ b/media/webrtc/trunk/webrtc/modules/desktop_capture/x11/x_error_trap.h
@@ -6,34 +6,47 @@
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #ifndef WEBRTC_MODULES_DESKTOP_CAPTURE_X11_X_ERROR_TRAP_H_
 #define WEBRTC_MODULES_DESKTOP_CAPTURE_X11_X_ERROR_TRAP_H_
 
-#include <X11/Xlib.h>
+#include <X11/Xlibint.h>
+#undef max // Xlibint.h defines this and it breaks std::max
+#undef min // Xlibint.h defines this and it breaks std::min
 
 #include "webrtc/system_wrappers/interface/constructor_magic.h"
 
 namespace webrtc {
 
 // Helper class that registers X Window error handler. Caller can use
 // GetLastErrorAndDisable() to get the last error that was caught, if any.
+// An XErrorTrap may be constructed on any thread, but errors are collected
+// from all threads and so |display| should be used only on one thread.
+// Other Displays are unaffected.
 class XErrorTrap {
  public:
   explicit XErrorTrap(Display* display);
   ~XErrorTrap();
 
   // Returns last error and removes unregisters the error handler.
+  // Must not be called more than once.
   int GetLastErrorAndDisable();
 
  private:
-  XErrorHandler original_error_handler_;
+  static Bool XServerErrorHandler(Display* display, xReply* rep,
+                                  char* /* buf */, int /* len */,
+                                  XPointer data);
+
+  _XAsyncHandler async_handler_;
+  Display* display_;
+  unsigned long last_ignored_request_;
+  int last_xserver_error_code_;
   bool enabled_;
 
   DISALLOW_COPY_AND_ASSIGN(XErrorTrap);
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_DESKTOP_CAPTURE_X11_X_ERROR_TRAP_H_