From a62062127e0d17ff10e961355ca3807fbd13811a Mon Sep 17 00:00:00 2001
From: Jim Broadus <jbroadus@gmail.com>
Date: Sat, 23 Feb 2019 22:12:07 -0800
Subject: [PATCH] Fix thread-safety issues when initially loading devices from
 the database.

When DeviceManager initializes, it creates a thread to load device information
from the database. Part of this process includes use of QPixMap for icons which
produced a warning message:

22:32:53.763 WARN  unknown                          QPixmap: It is not safe to use pixmaps outside the GUI thread

In addition, the device is added to the view using beginInsertRows and
endInsertRows. This could contend with a device added by a lister signaling
PhysicalDeviceAdded.

To solve these problems, this change moves the icon loading and insertion to the
main thread. LoadAllDevices reads the data from the database and creates the
DeviceInfo object, then sends a signal to the main thread. In the signal
handler, the icon is loaded and the device is added to the master list and view.
---
 src/devices/deviceinfo.cpp    | 11 +++--------
 src/devices/devicemanager.cpp | 24 ++++++++++++++++++++----
 src/devices/devicemanager.h   |  2 ++
 3 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/devices/deviceinfo.cpp b/src/devices/deviceinfo.cpp
index 9c1d83dac..85b148b43 100644
--- a/src/devices/deviceinfo.cpp
+++ b/src/devices/deviceinfo.cpp
@@ -58,14 +58,9 @@ void DeviceInfo::InitFromDb(const DeviceDatabaseBackend::Device& dev) {
   size_ = dev.size_;
   transcode_mode_ = dev.transcode_mode_;
   transcode_format_ = dev.transcode_format_;
-
-  QStringList icon_names = dev.icon_name_.split(',');
-  QVariantList icons;
-  for (const QString& icon_name : icon_names) {
-    icons << icon_name;
-  }
-
-  LoadIcon(icons, friendly_name_);
+  // Store the raw value for now. If it's a comma delimited list, it will be
+  // sorted out later.
+  icon_name_ = dev.icon_name_;
 
   QStringList unique_ids = dev.unique_id_.split(',');
   for (const QString& id : unique_ids) {
diff --git a/src/devices/devicemanager.cpp b/src/devices/devicemanager.cpp
index 6e911d23e..940907509 100644
--- a/src/devices/devicemanager.cpp
+++ b/src/devices/devicemanager.cpp
@@ -83,6 +83,8 @@ DeviceManager::DeviceManager(Application* app, QObject* parent)
   backend_->moveToThread(app_->database()->thread());
   backend_->Init(app_->database());
 
+  connect(this, SIGNAL(DeviceCreatedFromDb(DeviceInfo*)),
+          SLOT(AddDeviceFromDb(DeviceInfo*)));
   // This reads from the database and contends on the database mutex, which can
   // be very slow on startup.
   ConcurrentRun::Run<void>(&thread_pool_,
@@ -140,13 +142,27 @@ void DeviceManager::LoadAllDevices() {
   for (const DeviceDatabaseBackend::Device& device : devices) {
     DeviceInfo* info = new DeviceInfo(DeviceInfo::Type_Device, root_);
     info->InitFromDb(device);
-
-    beginInsertRows(ItemToIndex(root_), devices_.count(), devices_.count());
-    devices_ << info;
-    endInsertRows();
+    // Use of QPixMap and device insertion should only be done on the main
+    // thread. Send a signal to finish the device addition.
+    emit DeviceCreatedFromDb(info);
   }
 }
 
+void DeviceManager::AddDeviceFromDb(DeviceInfo* info) {
+  // At this point, icon_name_ contains the value from the database where the
+  // value is allowed to be a comma delimited list.
+  QStringList icon_names = info->icon_name_.split(',');
+  QVariantList icons;
+  for (const QString& icon_name : icon_names) {
+    icons << icon_name;
+  }
+  info->LoadIcon(icons, info->friendly_name_);
+
+  beginInsertRows(ItemToIndex(root_), devices_.count(), devices_.count());
+  devices_ << info;
+  endInsertRows();
+}
+
 QVariant DeviceManager::data(const QModelIndex& idx, int role) const {
   if (!idx.isValid() || idx.column() != 0) return QVariant();
 
diff --git a/src/devices/devicemanager.h b/src/devices/devicemanager.h
index 595a5d5c7..80044256b 100644
--- a/src/devices/devicemanager.h
+++ b/src/devices/devicemanager.h
@@ -101,6 +101,7 @@ class DeviceManager : public SimpleTreeModel<DeviceInfo> {
  signals:
   void DeviceConnected(QModelIndex idx);
   void DeviceDisconnected(QModelIndex idx);
+  void DeviceCreatedFromDb(DeviceInfo* info);
 
  private slots:
   void PhysicalDeviceAdded(const QString& id);
@@ -111,6 +112,7 @@ class DeviceManager : public SimpleTreeModel<DeviceInfo> {
   void DeviceSongCountUpdated(int count);
   void LoadAllDevices();
   void DeviceConnectFinished(const QString& id, bool success);
+  void AddDeviceFromDb(DeviceInfo* info);
 
  protected:
   void LazyPopulate(DeviceInfo* item) { LazyPopulate(item, true); }