Merge pull request #10354 from zzmp/fix/tablet-deadlock

Fix deadlock from accessing an initializing tablet

libraries/script-engine/src/TabletScriptingInterface.cpp

@@ -21,6 +21,8 @@
 #include <InfoView.h>
 #include "SoundEffect.h"
 
+const QString SYSTEM_TOOLBAR = "com.highfidelity.interface.toolbar.system";
+const QString SYSTEM_TABLET = "com.highfidelity.interface.tablet.system";
 
 QScriptValue tabletToScriptValue(QScriptEngine* engine, TabletProxy* const &in) {
     return engine->newQObject(in, QScriptEngine::QtOwnership, QScriptEngine::ExcludeDeleteLater | QScriptEngine::ExcludeChildObjects);
@@ -35,11 +37,11 @@ TabletScriptingInterface::TabletScriptingInterface() {
 }
 
 QObject* TabletScriptingInterface::getSystemToolbarProxy() {
-    const QString SYSTEM_TOOLBAR = "com.highfidelity.interface.toolbar.system";
     Qt::ConnectionType connectionType = Qt::AutoConnection;
     if (QThread::currentThread() != _toolbarScriptingInterface->thread()) {
         connectionType = Qt::BlockingQueuedConnection;
     }
+
     QObject* toolbarProxy = nullptr;
     bool hasResult = QMetaObject::invokeMethod(_toolbarScriptingInterface, "getToolbar", connectionType, Q_RETURN_ARG(QObject*, toolbarProxy), Q_ARG(QString, SYSTEM_TOOLBAR));
     if (hasResult) {
@@ -51,28 +53,38 @@ QObject* TabletScriptingInterface::getSystemToolbarProxy() {
 }
 
 TabletProxy* TabletScriptingInterface::getTablet(const QString& tabletId) {
+    TabletProxy* tabletProxy = nullptr;
+    {
+        // the only thing guarded should be map mutation
+        // this avoids a deadlock with the Main thread
+        // from Qt::BlockingQueuedEvent invocations later in the call-tree
+        std::lock_guard<std::mutex> guard(_mapMutex);
 
-    std::lock_guard<std::mutex> guard(_mutex);
+        auto iter = _tabletProxies.find(tabletId);
-
+        if (iter != _tabletProxies.end()) {
-    // look up tabletId in the map.
+            // tablet already exists
-    auto iter = _tabletProxies.find(tabletId);
+            return iter->second;
-    if (iter != _tabletProxies.end()) {
+        } else {
-        // tablet already exists, just return it.
+            // tablet must be created
-        return iter->second;
+            tabletProxy = new TabletProxy(this, tabletId);
-    } else {
+            _tabletProxies[tabletId] = tabletProxy;
-        // allocate a new tablet, add it to the map then return it.
+        }
-        auto tabletProxy = new TabletProxy(tabletId);
-        tabletProxy->setParent(this);
-        _tabletProxies[tabletId] = tabletProxy;
-        tabletProxy->setToolbarMode(_toolbarMode);
-        return tabletProxy;
     }
+
+    assert(tabletProxy);
+    // initialize new tablet
+    tabletProxy->setToolbarMode(_toolbarMode);
+    return tabletProxy;
 }
 
 void TabletScriptingInterface::setToolbarMode(bool toolbarMode) {
-    std::lock_guard<std::mutex> guard(_mutex);
+    {
-
+        // the only thing guarded should be _toolbarMode
-    _toolbarMode = toolbarMode;
+        // this avoids a deadlock with the Main thread
+        // from Qt::BlockingQueuedEvent invocations later in the call-tree
+        std::lock_guard<std::mutex> guard(_mapMutex);
+        _toolbarMode = toolbarMode;
+    }
 
     for (auto& iter : _tabletProxies) {
         iter.second->setToolbarMode(toolbarMode);
@@ -89,8 +101,9 @@ void TabletScriptingInterface::setQmlTabletRoot(QString tabletId, QQuickItem* qm
 }
 
 QQuickWindow* TabletScriptingInterface::getTabletWindow() {
-    TabletProxy* tablet = qobject_cast<TabletProxy*>(getTablet("com.highfidelity.interface.tablet.system"));
+    TabletProxy* tablet = qobject_cast<TabletProxy*>(getTablet(SYSTEM_TABLET));
     QObject* qmlSurface = tablet->getTabletSurface();
+
     OffscreenQmlSurface* surface = dynamic_cast<OffscreenQmlSurface*>(qmlSurface);
 
     if (!surface) {
@@ -156,7 +169,7 @@ void TabletScriptingInterface::processTabletEvents(QObject* object, const QKeyEv
 
 
 void TabletScriptingInterface::processEvent(const QKeyEvent* event) {
-    TabletProxy* tablet = qobject_cast<TabletProxy*>(getTablet("com.highfidelity.interface.tablet.system"));
+    TabletProxy* tablet = qobject_cast<TabletProxy*>(getTablet(SYSTEM_TABLET));
     QObject* qmlTablet = tablet->getQmlTablet();
     QObject* qmlMenu = tablet->getQmlMenu();
 
@@ -185,10 +198,12 @@ class TabletRootWindow : public QmlWindowClass {
     virtual QString qmlSource() const override { return "hifi/tablet/WindowRoot.qml"; }
 };
 
-TabletProxy::TabletProxy(QString name) : _name(name) {
+TabletProxy::TabletProxy(QObject* parent, QString name) : QObject(parent), _name(name) {
 }
 
 void TabletProxy::setToolbarMode(bool toolbarMode) {
+    std::lock_guard<std::mutex> guard(_tabletMutex);
+
     if (toolbarMode == _toolbarMode) {
         return;
     }
@@ -288,7 +303,7 @@ bool TabletProxy::isPathLoaded(QVariant path) {
     return path.toString() == _currentPathLoaded.toString();
 }
 void TabletProxy::setQmlTabletRoot(QQuickItem* qmlTabletRoot, QObject* qmlOffscreenSurface) {
-    std::lock_guard<std::mutex> guard(_mutex);
+    std::lock_guard<std::mutex> guard(_tabletMutex);
     _qmlOffscreenSurface = qmlOffscreenSurface;
     _qmlTabletRoot = qmlTabletRoot;
     if (_qmlTabletRoot && _qmlOffscreenSurface) {
@@ -519,7 +534,7 @@ void TabletProxy::gotoWebScreen(const QString& url, const QString& injectedJavaS
 
 QObject* TabletProxy::addButton(const QVariant& properties) {
     auto tabletButtonProxy = QSharedPointer<TabletButtonProxy>(new TabletButtonProxy(properties.toMap()));
-    std::lock_guard<std::mutex> guard(_mutex);
+    std::lock_guard<std::mutex> guard(_tabletMutex);
     _tabletButtonProxies.push_back(tabletButtonProxy);
     if (!_toolbarMode && _qmlTabletRoot) {
         auto tablet = getQmlTablet();
@@ -555,7 +570,7 @@ bool TabletProxy::onHomeScreen() {
 }
 
 void TabletProxy::removeButton(QObject* tabletButtonProxy) {
-    std::lock_guard<std::mutex> guard(_mutex);
+    std::lock_guard<std::mutex> guard(_tabletMutex);
 
     auto tablet = getQmlTablet();
     if (!tablet) {
@@ -743,12 +758,12 @@ TabletButtonProxy::TabletButtonProxy(const QVariantMap& properties) :
 }
 
 void TabletButtonProxy::setQmlButton(QQuickItem* qmlButton) {
-    std::lock_guard<std::mutex> guard(_mutex);
+    std::lock_guard<std::mutex> guard(_buttonMutex);
     _qmlButton = qmlButton;
 }
 
 void TabletButtonProxy::setToolbarButtonProxy(QObject* toolbarButtonProxy) {
-    std::lock_guard<std::mutex> guard(_mutex);
+    std::lock_guard<std::mutex> guard(_buttonMutex);
     _toolbarButtonProxy = toolbarButtonProxy;
     if (_toolbarButtonProxy) {
         QObject::connect(_toolbarButtonProxy, SIGNAL(clicked()), this, SLOT(clickedSlot()));
@@ -756,12 +771,12 @@ void TabletButtonProxy::setToolbarButtonProxy(QObject* toolbarButtonProxy) {
 }
 
 QVariantMap TabletButtonProxy::getProperties() const {
-    std::lock_guard<std::mutex> guard(_mutex);
+    std::lock_guard<std::mutex> guard(_buttonMutex);
     return _properties;
 }
 
 void TabletButtonProxy::editProperties(QVariantMap properties) {
-    std::lock_guard<std::mutex> guard(_mutex);
+    std::lock_guard<std::mutex> guard(_buttonMutex);
 
     QVariantMap::const_iterator iter = properties.constBegin();
     while (iter != properties.constEnd()) {

libraries/script-engine/src/TabletScriptingInterface.h

@@ -71,7 +71,7 @@ private:
     void processTabletEvents(QObject* object, const QKeyEvent* event);
 
 protected:
-    std::mutex _mutex;
+    std::mutex _mapMutex;
     std::map<QString, TabletProxy*> _tabletProxies;
     QObject* _toolbarScriptingInterface { nullptr };
     bool _toolbarMode { false };
@@ -90,7 +90,7 @@ class TabletProxy : public QObject {
     Q_PROPERTY(bool landscape READ getLandscape WRITE setLandscape)
     Q_PROPERTY(bool tabletShown MEMBER _tabletShown NOTIFY tabletShownChanged)
 public:
-    TabletProxy(QString name);
+    TabletProxy(QObject* parent, QString name);
 
     void setQmlTabletRoot(QQuickItem* qmlTabletRoot, QObject* qmlOffscreenSurface);
 
@@ -248,7 +248,7 @@ protected:
     QVariant _initialPath { "" };
     QVariant _currentPathLoaded { "" };
     QString _name;
-    std::mutex _mutex;
+    std::mutex _tabletMutex;
     std::vector<QSharedPointer<TabletButtonProxy>> _tabletButtonProxies;
     QQuickItem* _qmlTabletRoot { nullptr };
     QObject* _qmlOffscreenSurface { nullptr };
@@ -309,7 +309,7 @@ signals:
 protected:
     QUuid _uuid;
     int _stableOrder;
-    mutable std::mutex _mutex;
+    mutable std::mutex _buttonMutex;
     QQuickItem* _qmlButton { nullptr };
     QObject* _toolbarButtonProxy { nullptr };
     QVariantMap _properties;