ubuntu-touch-coreapps-reviewers team mailing list archive

[Carlos Jose Mazieri <carlos.mazieri@xxxxxxxxx>]

Carlos Jose Mazieri has proposed merging lp:~carlos-mazieri/ubuntu-filemanager-app/fix-network-crash-1609051 into lp:ubuntu-filemanager-app.

Commit message:
It fixes a network crash bug #1609051 when Localtion::m_info is deleted in the main thread.

Requested reviews:
  Ubuntu File Manager Developers (ubuntu-filemanager-dev)

For more details, see:
https://code.launchpad.net/~carlos-mazieri/ubuntu-filemanager-app/fix-network-crash-1609051/+merge/315768

It fixes a network crash bug #1609051 when Localtion::m_info is deleted in the main thread.

It happens because Localtion::m_info is used in the worker thread as 'parent' item information.

Solution: NetworkListWorker creates its own DirItemInfo instance and copies the data from Localtion::m_info to avoid that problem.

Location and descendant classes should be checked further to avoid deleting Localtion::m_info, methods such as Location::refreshInfo() and  Location::becomeParent(), Location::setInfoItem(), TrashLocation::becomeParent() can be changed to copy data from a temporary DirItemInfo  object.
-- 
Your team Ubuntu File Manager Developers is requested to review the proposed merge of lp:~carlos-mazieri/ubuntu-filemanager-app/fix-network-crash-1609051 into lp:ubuntu-filemanager-app.

=== modified file 'src/plugin/folderlistmodel/networklistworker.cpp'
--- src/plugin/folderlistmodel/networklistworker.cpp	2015-12-12 14:32:18 +0000
+++ src/plugin/folderlistmodel/networklistworker.cpp	2017-01-27 12:57:59 +0000
@@ -24,15 +24,22 @@
 #include "locationurl.h"
 
 NetworkListWorker::NetworkListWorker(LocationItemDirIterator * dirIterator,
-                                     DirItemInfo * mainItemInfo, const DirItemInfo *parent) :
+                                     DirItemInfo             * mainItemInfo,
+                                     const DirItemInfo       * parentItemInfo) :
     DirListWorker(dirIterator->path(),
                   dirIterator->filters(),
                   dirIterator->flags() == QDirIterator::Subdirectories ? true : false),
     m_dirIterator(dirIterator),
-    m_mainItemInfo(mainItemInfo),
-    m_parent(parent)
+    m_mainItemInfo(mainItemInfo),  // m_mainItemInfo takes ownership of mainItemInfo
+    m_parentItemInfo(0)
 {
      mLoaderType =  NetworkLoader;
+     // create its own instance by doing a copy from parentItemInfo
+     if (parentItemInfo != 0)
+     {
+         m_parentItemInfo = new DirItemInfo();
+         *m_parentItemInfo = *parentItemInfo;
+     }
 }
 
 
@@ -40,6 +47,10 @@
 {
     delete m_dirIterator;
     delete m_mainItemInfo;
+    if (m_parentItemInfo != 0)
+    {
+        delete m_parentItemInfo;
+    }
 }
 
 
@@ -47,7 +58,7 @@
 {
      DirItemInfoList netContent;
      m_dirIterator->load();
-     bool is_parent_of_smb_url = m_parent != 0 && m_parent->urlPath().startsWith(LocationUrl::SmbURL);
+     bool is_parent_of_smb_url = m_parentItemInfo != 0 && m_parentItemInfo->urlPath().startsWith(LocationUrl::SmbURL);
      while (m_dirIterator->hasNext())
      {
          m_mainItemInfo->setFile(m_dirIterator->next());
@@ -68,7 +79,7 @@
  */
 void NetworkListWorker::setSmbItemAttributes()
 {    
-    if (m_parent->isHost())      { m_mainItemInfo->setAsShare(); }
+    if (m_parentItemInfo->isHost())      { m_mainItemInfo->setAsShare(); }
     else
-    if (m_parent->isWorkGroup()) { m_mainItemInfo->setAsHost(); }
+    if (m_parentItemInfo->isWorkGroup()) { m_mainItemInfo->setAsHost(); }
 }

=== modified file 'src/plugin/folderlistmodel/networklistworker.h'
--- src/plugin/folderlistmodel/networklistworker.h	2015-12-12 14:32:18 +0000
+++ src/plugin/folderlistmodel/networklistworker.h	2017-01-27 12:57:59 +0000
@@ -40,15 +40,15 @@
 public:
     NetworkListWorker(LocationItemDirIterator * dirIterator,
                       DirItemInfo             * mainItemInfo,
-                      const DirItemInfo       * parent = 0);
+                      const DirItemInfo       * parentItemInfo = 0);
     ~NetworkListWorker();
 protected:
     virtual DirItemInfoList getNetworkContent();
     void                    setSmbItemAttributes();
 protected:
      LocationItemDirIterator     * m_dirIterator;
-     DirItemInfo                 * m_mainItemInfo;
-     const DirItemInfo           * m_parent;
+     DirItemInfo                 * m_mainItemInfo;   //takes ownership from mainItemInfo
+     DirItemInfo                 * m_parentItemInfo; //create its own instance by doing a copy from parentItemInfo
 };
 
 #endif // NETWORKLISTWORKER_H

=== modified file 'src/plugin/folderlistmodel/networklocation.cpp'
--- src/plugin/folderlistmodel/networklocation.cpp	2015-12-12 14:32:18 +0000
+++ src/plugin/folderlistmodel/networklocation.cpp	2017-01-27 12:57:59 +0000
@@ -36,6 +36,6 @@
 
     LocationItemDirIterator *dirIterator  = newDirIterator(urlPath,filter,flags,LocationItemDirIterator::LoadLater);
     DirItemInfo             *baseitemInfo = newItemInfo(QLatin1String(0));
-
+    // the NetworkListWorker object takes ownership of baseitemInfo and also creates its own copy of m_info
     return new NetworkListWorker(dirIterator, baseitemInfo, m_info);
 }