Merge pull request #10228 from Filoppi/fix_android_input_deadlock

ControllerInterface: fix UpdateReferences() deadlock

Source/Core/DolphinQt/Settings.cpp

@@ -80,8 +80,7 @@ Settings::Settings()
       // they'd continue living until the queued event has run, but some devices can't be recreated
       // until they are destroyed.
       // This is safe from any thread. Devices will be refreshed and re-acquired and in
-      // DevicesChanged(). Waiting on QueueOnObject() to have finished running was not feasible as
+      // DevicesChanged(). Calling it without queueing shouldn't cause any deadlocks but is slow.
-      // it would cause deadlocks without heavy workarounds.
       emit ReleaseDevices();
 
       QueueOnObject(this, [this] { emit DevicesChanged(); });

Source/Core/InputCommon/ControllerEmu/ControllerEmu.cpp

@@ -18,6 +18,8 @@
 
 namespace ControllerEmu
 {
+// This should theoretically be per EmulatedController instance,
+// though no EmulatedController usually run in parallel, so it makes little difference
 static std::recursive_mutex s_get_state_mutex;
 
 std::string EmulatedController::GetDisplayName() const
@@ -77,6 +79,7 @@ void EmulatedController::UpdateSingleControlReference(const ControllerInterface&
 {
   ciface::ExpressionParser::ControlEnvironment env(devi, GetDefaultDevice(), m_expression_vars);
 
+  const auto lock = GetStateLock();
   ref->UpdateReference(env);
 
   env.CleanUnusedVariables();

Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp

@@ -54,8 +54,6 @@ void ControllerInterface::Initialize(const WindowSystemInfo& wsi)
 
   m_populating_devices_counter = 1;
 
-  m_devices_mutex.lock();
-
 #ifdef CIFACE_USE_WIN32
   ciface::Win32::Init(wsi.render_window);
 #endif
@@ -87,8 +85,9 @@ void ControllerInterface::Initialize(const WindowSystemInfo& wsi)
 
   RefreshDevices();
 
+  // Devices writes are already protected by m_devices_population_mutex but this won't hurt
+  m_devices_mutex.lock();
   const bool devices_empty = m_devices.empty();
-
   m_devices_mutex.unlock();
 
   if (m_populating_devices_counter.fetch_sub(1) == 1 && !devices_empty)
@@ -121,17 +120,17 @@ void ControllerInterface::RefreshDevices(RefreshReason reason)
     std::lock_guard lk_pre_population(m_pre_population_mutex);
     // This is needed to stop its threads before locking our mutexes, to avoid deadlocks
     // (in case it tried to add a device after we had locked m_devices_population_mutex).
-    // There doesn't seem to be an easy to way to repopulate OSX devices without restarting
+    // There doesn't seem to be an easy to way to repopulate OSX devices without restarting its
-    // its hotplug thread. This will not release its devices, that's still done below.
+    // hotplug thread. This should not remove its devices, and if it did, calls should be ignored.
     ciface::OSX::DeInit();
   }
 #endif
 
-  // This lock has two main functions:
+  // We lock m_devices_population_mutex here to make everything simpler.
-  // -Avoid a deadlock between m_devices_mutex and ControllerEmu::s_state_mutex when
+  // Multiple devices classes have their own "hotplug" thread, and can add/remove devices at any
-  // InvokeDevicesChangedCallbacks() is called concurrently by two different threads.
+  // time, while actual writes to "m_devices" are safe, the order in which they happen is not. That
-  // -Avoid devices being destroyed while others of the same type are being created.
+  // means a thread could be adding devices while we are removing them, or removing them as we are
-  // This wasn't thread safe in multiple device sources.
+  // populating them (causing missing or duplicate devices).
   std::lock_guard lk_population(m_devices_population_mutex);
 
 #if defined(CIFACE_USE_WIN32) && !defined(CIFACE_USE_XLIB) && !defined(CIFACE_USE_OSX)
@@ -141,12 +140,9 @@ void ControllerInterface::RefreshDevices(RefreshReason reason)
   {
     m_populating_devices_counter.fetch_add(1);
 
-    {
-      std::lock_guard lk(m_devices_mutex);
     // No need to do anything else in this case.
     // Only (Win32) DInput needs the window handle to be updated.
     ciface::Win32::ChangeWindow(m_wsi.render_window);
-    }
 
     if (m_populating_devices_counter.fetch_sub(1) == 1)
       InvokeDevicesChangedCallbacks();
@@ -156,13 +152,6 @@ void ControllerInterface::RefreshDevices(RefreshReason reason)
 
   m_populating_devices_counter.fetch_add(1);
 
-  // We lock m_devices_mutex here to make everything simpler.
-  // Multiple devices classes have their own "hotplug" thread, and can add/remove devices at any
-  // time, while actual writes to "m_devices" are safe, the order in which they happen is not. That
-  // means a thread could be adding devices while we are removing them, or removing them as we are
-  // populating them (causing missing or duplicate devices).
-  m_devices_mutex.lock();
-
   // Make sure shared_ptr<Device> objects are released before repopulating.
   ClearDevices();
 
@@ -206,8 +195,6 @@ void ControllerInterface::RefreshDevices(RefreshReason reason)
 
   WiimoteReal::PopulateDevices();
 
-  m_devices_mutex.unlock();
-
   if (m_populating_devices_counter.fetch_sub(1) == 1)
     InvokeDevicesChangedCallbacks();
 }
@@ -221,11 +208,7 @@ void ControllerInterface::PlatformPopulateDevices(std::function<void()> callback
 
   m_populating_devices_counter.fetch_add(1);
 
-  {
-    std::lock_guard lk(m_devices_mutex);
-
   callback();
-  }
 
   if (m_populating_devices_counter.fetch_sub(1) == 1)
     InvokeDevicesChangedCallbacks();
@@ -270,8 +253,8 @@ void ControllerInterface::Shutdown()
 
   // Make sure no devices had been added within Shutdown() in the time
   // between checking they checked atomic m_is_init bool and we changed it.
-  // We couldn't have locked m_devices_mutex nor m_devices_population_mutex for the whole Shutdown()
+  // We couldn't have locked m_devices_population_mutex for the whole Shutdown()
-  // as they could cause deadlocks. Note that this is still not 100% safe as some backends are
+  // as it could cause deadlocks. Note that this is still not 100% safe as some backends are
   // shut down in other places, possibly adding devices after we have shut down, but the chances of
   // that happening are basically zero.
   ClearDevices();
@@ -294,8 +277,8 @@ void ControllerInterface::ClearDevices()
         o->SetState(0);
     }
 
-    // Devices will still be alive after this: there are shared ptrs around the code holding them,
+    // Devices could still be alive after this as there might be shared ptrs around holding them.
-    // but InvokeDevicesChangedCallbacks() will clean all of them.
+    // The InvokeDevicesChangedCallbacks() underneath should always clean all of them (it needs to).
     m_devices.clear();
   }