Merge pull request #23499 from garbear/fix-port-dialog-segfault2

Port Dialog: Fix segfault when choosing a controller, and minor improvements

xbmc/games/addons/GameClient.cpp

@@ -599,8 +599,6 @@ void CGameClient::LogException(const char* strFunctionName) const
 
 void CGameClient::cb_close_game(KODI_HANDLE kodiInstance)
 {
-  using namespace MESSAGING;
-
   CServiceBroker::GetAppMessenger()->PostMsg(TMSG_GUI_ACTION, WINDOW_INVALID, -1,
                                              static_cast<void*>(new CAction(ACTION_STOP)));
 }

xbmc/games/addons/input/GameClientInput.cpp

@@ -61,6 +61,8 @@ void CGameClientInput::Initialize()
   // Send controller layouts to game client
   SetControllerLayouts(m_topology->GetControllerTree().GetControllers());
 
+  std::lock_guard<std::recursive_mutex> lock(m_portMutex);
+
   // Reset ports to default state (first accepted controller is connected)
   ActivateControllers(m_topology->GetControllerTree());
 
@@ -72,23 +74,27 @@ void CGameClientInput::Initialize()
 
 void CGameClientInput::Start(IGameInputCallback* input)
 {
-  m_inputCallback = input;
-
-  // Connect/disconnect active controllers
-  for (const CPortNode& port : GetActiveControllerTree().GetPorts())
   {
-    if (port.IsConnected())
+    std::lock_guard<std::recursive_mutex> lock(m_portMutex);
+
+    m_inputCallback = input;
+
+    // Connect/disconnect active controllers
+    for (const CPortNode& port : GetActiveControllerTree().GetPorts())
     {
-      const ControllerPtr& activeController = port.GetActiveController().GetController();
-      if (activeController)
-        ConnectController(port.GetAddress(), activeController);
+      if (port.IsConnected())
+      {
+        const ControllerPtr& activeController = port.GetActiveController().GetController();
+        if (activeController)
+          ConnectController(port.GetAddress(), activeController);
+      }
+      else
+        DisconnectController(port.GetAddress());
     }
-    else
-      DisconnectController(port.GetAddress());
-  }
 
-  // Ensure hardware is open to receive events
-  m_hardware.reset(new CGameClientHardware(m_gameClient));
+    // Ensure hardware is open to receive events
+    m_hardware.reset(new CGameClientHardware(m_gameClient));
+  }
 
   // Notify observers of the initial port configuration
   NotifyObservers(ObservableMessageGamePortsChanged);
@@ -105,6 +111,8 @@ void CGameClientInput::Deinitialize()
 
 void CGameClientInput::Stop()
 {
+  std::lock_guard<std::recursive_mutex> lock(m_portMutex);
+
   m_hardware.reset();
 
   CloseMouse();
@@ -312,6 +320,8 @@ bool CGameClientInput::ConnectController(const std::string& portAddress,
     return false;
   }
 
+  std::lock_guard<std::recursive_mutex> lock(m_portMutex);
+
   const CPortNode& currentPort = GetActiveControllerTree().GetPort(portAddress);
 
   // Close current ports if any are open
@@ -367,6 +377,8 @@ bool CGameClientInput::DisconnectController(const std::string& portAddress)
 {
   PERIPHERALS::EventLockHandlePtr inputHandlingLock;
 
+  std::lock_guard<std::recursive_mutex> lock(m_portMutex);
+
   // If port is a multitap, we need to deactivate its children
   const CPortNode& currentPort = GetActiveControllerTree().GetPort(portAddress);
   CloseJoysticks(currentPort, inputHandlingLock);
@@ -407,22 +419,30 @@ bool CGameClientInput::DisconnectController(const std::string& portAddress)
 
 void CGameClientInput::SavePorts()
 {
+  {
+    std::lock_guard<std::recursive_mutex> lock(m_portMutex);
+
+    // Save port state
+    m_portManager->SaveXMLAsync();
+  }
+
   // Let the observers know that ports have changed
   NotifyObservers(ObservableMessageGamePortsChanged);
-
-  // Save port state
-  m_portManager->SaveXML();
 }
 
 void CGameClientInput::ResetPorts()
 {
+  std::lock_guard<std::recursive_mutex> lock(m_portMutex);
+
   const CControllerTree& controllerTree = GetDefaultControllerTree();
   for (const CPortNode& port : controllerTree.GetPorts())
     ConnectController(port.GetAddress(), port.GetActiveController().GetController());
 }
 
 bool CGameClientInput::HasAgent() const
 {
+  std::lock_guard<std::recursive_mutex> lock(m_portMutex);
+
   if (!m_joysticks.empty())
     return true;
 
@@ -451,6 +471,8 @@ bool CGameClientInput::OpenKeyboard(const ControllerPtr& controller,
 
   bool bSuccess = false;
 
+  std::lock_guard<std::recursive_mutex> lock(m_portMutex);
+
   {
     std::unique_lock<CCriticalSection> lock(m_clientAccess);
 
@@ -480,11 +502,15 @@ bool CGameClientInput::OpenKeyboard(const ControllerPtr& controller,
 
 bool CGameClientInput::IsKeyboardOpen() const
 {
+  std::lock_guard<std::recursive_mutex> lock(m_portMutex);
+
   return static_cast<bool>(m_keyboard);
 }
 
 void CGameClientInput::CloseKeyboard()
 {
+  std::lock_guard<std::recursive_mutex> lock(m_portMutex);
+
   if (m_keyboard)
   {
     m_keyboard.reset();
@@ -521,6 +547,8 @@ bool CGameClientInput::OpenMouse(const ControllerPtr& controller,
 
   bool bSuccess = false;
 
+  std::lock_guard<std::recursive_mutex> lock(m_portMutex);
+
   {
     std::unique_lock<CCriticalSection> lock(m_clientAccess);
 
@@ -549,11 +577,15 @@ bool CGameClientInput::OpenMouse(const ControllerPtr& controller,
 
 bool CGameClientInput::IsMouseOpen() const
 {
+  std::lock_guard<std::recursive_mutex> lock(m_portMutex);
+
   return static_cast<bool>(m_mouse);
 }
 
 void CGameClientInput::CloseMouse()
 {
+  std::lock_guard<std::recursive_mutex> lock(m_portMutex);
+
   if (m_mouse)
   {
     m_mouse.reset();

xbmc/games/addons/input/GameClientInput.h

@@ -16,6 +16,7 @@
 
 #include <map>
 #include <memory>
+#include <mutex>
 #include <string>
 
 class CCriticalSection;
@@ -131,9 +132,19 @@ class CGameClientInput : protected CGameClientSubsystem, public Observable
    */
   JoystickMap m_joysticks;
 
-  // TODO: Guard with a mutex
+  /*!
+   * \brief Serializable port state
+   */
   std::unique_ptr<CPortManager> m_portManager;
 
+  /*!
+   * \brief Mutex for port state
+   *
+   * Mutex is recursive to allow for management of several ports within the
+   * function ResetPorts().
+   */
+  mutable std::recursive_mutex m_portMutex;
+
   /*!
    * \brief Keyboard handler
    *

xbmc/games/controllers/dialogs/ControllerSelect.cpp

@@ -39,6 +39,9 @@ void CControllerSelect::Initialize(ControllerVector controllers,
   if (!callback)
     return;
 
+  // Stop thread and reset state
+  Deinitialize();
+
   // Initialize state
   m_controllers = std::move(controllers);
   m_defaultController = std::move(defaultController);

xbmc/games/controllers/guicontrols/GUIFeatureButton.cpp

@@ -44,8 +44,6 @@ bool CGUIFeatureButton::DoPrompt(const std::string& strPrompt,
                                  const std::string& strFeature,
                                  CEvent& waitEvent)
 {
-  using namespace MESSAGING;
-
   bool bInterrupted = false;
 
   if (!HasFocus())

xbmc/games/controllers/windows/GUIControllerList.cpp

@@ -171,7 +171,6 @@ void CGUIControllerList::OnEvent(const ADDON::AddonEvent& event)
       typeid(event) == typeid(ADDON::AddonEvents::ReInstalled) ||
       typeid(event) == typeid(ADDON::AddonEvents::UnInstalled))
   {
-    using namespace MESSAGING;
     CGUIMessage msg(GUI_MSG_REFRESH_LIST, m_guiWindow->GetID(), CONTROL_CONTROLLER_LIST);
 
     // Focus installed add-on

xbmc/games/ports/input/PortManager.cpp

@@ -48,6 +48,11 @@ void CPortManager::Initialize(const std::string& profilePath)
 
 void CPortManager::Deinitialize()
 {
+  // Wait for save tasks
+  for (std::future<void>& task : m_saveFutures)
+    task.wait();
+  m_saveFutures.clear();
+
   m_controllerTree.Clear();
   m_xmlPath.clear();
 }
@@ -86,16 +91,32 @@ void CPortManager::LoadXML()
   DeserializePorts(pRootElement, m_controllerTree.GetPorts());
 }
 
-void CPortManager::SaveXML()
+void CPortManager::SaveXMLAsync()
 {
-  CXBMCTinyXML doc;
-  TiXmlElement node(XML_ROOT_PORTS);
+  PortVec ports = m_controllerTree.GetPorts();
+
+  // Prune any finished save tasks
+  m_saveFutures.erase(std::remove_if(m_saveFutures.begin(), m_saveFutures.end(),
+                                     [](std::future<void>& task) {
+                                       return task.wait_for(std::chrono::seconds(0)) ==
+                                              std::future_status::ready;
+                                     }),
+                      m_saveFutures.end());
+
+  // Save async
+  std::future<void> task = std::async(std::launch::async, [this, ports = std::move(ports)]() {
+    CXBMCTinyXML doc;
+    TiXmlElement node(XML_ROOT_PORTS);
+
+    SerializePorts(node, ports);
 
-  SerializePorts(node, m_controllerTree.GetPorts());
+    doc.InsertEndChild(node);
 
-  doc.InsertEndChild(node);
+    std::lock_guard<std::mutex> lock(m_saveMutex);
+    doc.SaveFile(m_xmlPath);
+  });
 
-  doc.SaveFile(m_xmlPath);
+  m_saveFutures.emplace_back(std::move(task));
 }
 
 void CPortManager::Clear()

xbmc/games/ports/input/PortManager.h

@@ -10,6 +10,10 @@
 
 #include "games/controllers/types/ControllerTree.h"
 
+#include <future>
+#include <mutex>
+#include <string>
+
 class TiXmlElement;
 
 namespace KODI
@@ -27,7 +31,7 @@ class CPortManager
 
   void SetControllerTree(const CControllerTree& controllerTree);
   void LoadXML();
-  void SaveXML();
+  void SaveXMLAsync();
   void Clear();
 
   void ConnectController(const std::string& portAddress,
@@ -69,6 +73,9 @@ class CPortManager
 
   CControllerTree m_controllerTree;
   std::string m_xmlPath;
+
+  std::vector<std::future<void>> m_saveFutures;
+  std::mutex m_saveMutex;
 };
 } // namespace GAME
 } // namespace KODI

xbmc/games/ports/types/PortNode.cpp

@@ -45,6 +45,7 @@ CPortNode& CPortNode::operator=(CPortNode&& rhs) noexcept
     m_portType = rhs.m_portType;
     m_portId = std::move(rhs.m_portId);
     m_address = std::move(rhs.m_address);
+    m_forceConnected = rhs.m_forceConnected;
     m_controllers = std::move(rhs.m_controllers);
   }
 

xbmc/games/ports/windows/GUIPortList.cpp

@@ -160,7 +160,6 @@ void CGUIPortList::ResetPorts()
     m_gameClient->Input().SavePorts();
 
     // Refresh the GUI
-    using namespace MESSAGING;
     CGUIMessage msg(GUI_MSG_REFRESH_LIST, m_guiWindow.GetID(), CONTROL_PORT_LIST);
     CServiceBroker::GetAppMessenger()->SendGUIMessage(msg, m_guiWindow.GetID());
   }
@@ -173,7 +172,6 @@ void CGUIPortList::OnEvent(const ADDON::AddonEvent& event)
       typeid(event) == typeid(ADDON::AddonEvents::ReInstalled) ||
       typeid(event) == typeid(ADDON::AddonEvents::UnInstalled))
   {
-    using namespace MESSAGING;
     CGUIMessage msg(GUI_MSG_REFRESH_LIST, m_guiWindow.GetID(), CONTROL_PORT_LIST);
     msg.SetStringParam(event.addonId);
     CServiceBroker::GetAppMessenger()->SendGUIMessage(msg, m_guiWindow.GetID());
@@ -210,6 +208,7 @@ bool CGUIPortList::AddItems(const CPortNode& port,
     for (const CPortNode& childPort : ports)
     {
       std::ostringstream childItemLabel;
+      childItemLabel << " - ";
       childItemLabel << controller->Layout().Label();
       childItemLabel << " - ";
       childItemLabel << GetLabel(childPort);
@@ -301,7 +300,6 @@ void CGUIPortList::OnControllerSelected(const CPortNode& port, const ControllerP
     }
 
     // Send a GUI message to reload the port list
-    using namespace MESSAGING;
     CGUIMessage msg(GUI_MSG_REFRESH_LIST, m_guiWindow.GetID(), CONTROL_PORT_LIST);
     CServiceBroker::GetAppMessenger()->SendGUIMessage(msg, m_guiWindow.GetID());
   }