Allow concurrent desktop_capture instances on X11

I would like to run a separate capturer for each desktop on Linux and
I ran into the DCHECK in XErrorTrap when I was prototyping that
solution.  I addressed it by using a Mutex and then experienced and
occasional hang when capturing which I traced down to
SharedXDisplay::ProcessPendingXEvents(), this is a shared display
instance used by each unique capturer instance so I added a mutex
there as well.

I ran 2 capturer instances concurrently for well over an hour and did
not experience any hangs or capture artifacts.

Bug: webrtc:2022
Change-Id: Ia6778cae4bbae48886fe45f2991f02e0ea08fef6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/271920
Reviewed-by: Alexander Cooper <alcooper@chromium.org>
Commit-Queue: Joe Downing <joedow@google.com>
Cr-Commit-Position: refs/heads/main@{#37892}

modules/desktop_capture/linux/x11/mouse_cursor_monitor_x11.cc

@@ -145,18 +145,21 @@ void MouseCursorMonitorX11::Capture() {
     Window child_window;
     unsigned int mask;
 
-    XErrorTrap error_trap(display());
-    Bool result = XQueryPointer(display(), window_, &root_window, &child_window,
-                                &root_x, &root_y, &win_x, &win_y, &mask);
     CursorState state;
-    if (!result || error_trap.GetLastErrorAndDisable() != 0) {
-      state = OUTSIDE;
-    } else {
-      // In screen mode (window_ == root_window) the mouse is always inside.
-      // XQueryPointer() sets `child_window` to None if the cursor is outside
-      // `window_`.
-      state =
-          (window_ == root_window || child_window != None) ? INSIDE : OUTSIDE;
+    {
+      XErrorTrap error_trap(display());
+      Bool result =
+          XQueryPointer(display(), window_, &root_window, &child_window,
+                        &root_x, &root_y, &win_x, &win_y, &mask);
+      if (!result || error_trap.GetLastErrorAndDisable() != 0) {
+        state = OUTSIDE;
+      } else {
+        // In screen mode (window_ == root_window) the mouse is always inside.
+        // XQueryPointer() sets `child_window` to None if the cursor is outside
+        // `window_`.
+        state =
+            (window_ == root_window || child_window != None) ? INSIDE : OUTSIDE;
+      }
     }
 
     // As the comments to GetTopLevelWindow() above indicate, in window capture,

modules/desktop_capture/linux/x11/shared_x_display.cc

@@ -48,10 +48,12 @@ rtc::scoped_refptr<SharedXDisplay> SharedXDisplay::CreateDefault() {
 }
 
 void SharedXDisplay::AddEventHandler(int type, XEventHandler* handler) {
+  MutexLock lock(&mutex_);
   event_handlers_[type].push_back(handler);
 }
 
 void SharedXDisplay::RemoveEventHandler(int type, XEventHandler* handler) {
+  MutexLock lock(&mutex_);
   EventHandlersMap::iterator handlers = event_handlers_.find(type);
   if (handlers == event_handlers_.end())
     return;
@@ -70,6 +72,10 @@ void SharedXDisplay::ProcessPendingXEvents() {
   // processing events.
   rtc::scoped_refptr<SharedXDisplay> self(this);
 
+  // Protect access to `event_handlers_` after incrementing the refcount for
+  // `this` to ensure the instance is still valid when the lock is acquired.
+  MutexLock lock(&mutex_);
+
   // Find the number of events that are outstanding "now."  We don't just loop
   // on XPending because we want to guarantee this terminates.
   int events_to_process = XPending(display());

modules/desktop_capture/linux/x11/shared_x_display.h

@@ -17,7 +17,9 @@
 #include "absl/strings/string_view.h"
 #include "api/ref_counted_base.h"
 #include "api/scoped_refptr.h"
+#include "rtc_base/synchronization/mutex.h"
 #include "rtc_base/system/rtc_export.h"
+#include "rtc_base/thread_annotations.h"
 
 // Including Xlib.h will involve evil defines (Bool, Status, True, False), which
 // easily conflict with other headers.
@@ -76,7 +78,9 @@ class RTC_EXPORT SharedXDisplay
 
   Display* display_;
 
-  EventHandlersMap event_handlers_;
+  Mutex mutex_;
+
+  EventHandlersMap event_handlers_ RTC_GUARDED_BY(mutex_);
 };
 
 }  // namespace webrtc

modules/desktop_capture/linux/x11/x_error_trap.cc

@@ -10,6 +10,8 @@
 
 #include "modules/desktop_capture/linux/x11/x_error_trap.h"
 
+#include <atomic>
+
 #include <stddef.h>
 
 #include "rtc_base/checks.h"
@@ -18,36 +20,40 @@ namespace webrtc {
 
 namespace {
 
-// 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;
+static std::atomic<Display*> g_display_for_error_handler = nullptr;
+
+Mutex* AcquireMutex() {
+  static Mutex* mutex = new Mutex();
+  return mutex;
+}
 
 int XServerErrorHandler(Display* display, XErrorEvent* error_event) {
-  RTC_DCHECK(g_xserver_error_trap_enabled);
+  RTC_DCHECK_EQ(display, g_display_for_error_handler.load());
   g_last_xserver_error_code = error_event->error_code;
   return 0;
 }
 
 }  // namespace
 
-XErrorTrap::XErrorTrap(Display* display)
-    : original_error_handler_(NULL), enabled_(true) {
-  RTC_DCHECK(!g_xserver_error_trap_enabled);
-  original_error_handler_ = XSetErrorHandler(&XServerErrorHandler);
-  g_xserver_error_trap_enabled = true;
+XErrorTrap::XErrorTrap(Display* display) : mutex_lock_(AcquireMutex()) {
+  // We don't expect this class to be used in a nested fashion so therefore
+  // g_display_for_error_handler should never be valid here.
+  RTC_DCHECK(!g_display_for_error_handler.load());
+  RTC_DCHECK(display);
+  g_display_for_error_handler.store(display);
   g_last_xserver_error_code = 0;
+  original_error_handler_ = XSetErrorHandler(&XServerErrorHandler);
 }
 
 int XErrorTrap::GetLastErrorAndDisable() {
-  enabled_ = false;
-  RTC_DCHECK(g_xserver_error_trap_enabled);
+  g_display_for_error_handler.store(nullptr);
   XSetErrorHandler(original_error_handler_);
-  g_xserver_error_trap_enabled = false;
   return g_last_xserver_error_code;
 }
 
 XErrorTrap::~XErrorTrap() {
-  if (enabled_)
+  if (g_display_for_error_handler.load() != nullptr)
     GetLastErrorAndDisable();
 }
 

modules/desktop_capture/linux/x11/x_error_trap.h

@@ -13,24 +13,28 @@
 
 #include <X11/Xlib.h>
 
+#include "rtc_base/synchronization/mutex.h"
+
 namespace webrtc {
 
-// Helper class that registers X Window error handler. Caller can use
+// Helper class that registers an X Window error handler. Caller can use
 // GetLastErrorAndDisable() to get the last error that was caught, if any.
 class XErrorTrap {
  public:
   explicit XErrorTrap(Display* display);
-  ~XErrorTrap();
 
   XErrorTrap(const XErrorTrap&) = delete;
   XErrorTrap& operator=(const XErrorTrap&) = delete;
 
-  // Returns last error and removes unregisters the error handler.
+  ~XErrorTrap();
+
+  // Returns the last error if one was caught, otherwise 0. Also unregisters the
+  // error handler and replaces it with `original_error_handler_`.
   int GetLastErrorAndDisable();
 
  private:
-  XErrorHandler original_error_handler_;
-  bool enabled_;
+  MutexLock mutex_lock_;
+  XErrorHandler original_error_handler_ = nullptr;
 };
 
 }  // namespace webrtc