Merge lp:~renatofilho/ubuntu-filemanager-app/fix-network-crash into lp:ubuntu-filemanager-app

[Renato Araujo Oliveira Filho]

Make sure that NetworkListWorker get invalidated if the parent DirItemInfo was destroyed.

App was crashing because DirItemInfo was destroyed and NetworkListWorker was trying to use an invalid pointer.

[lp:~renatofilho/ubuntu-filemanager-app/fix-network-crash]

587. By Renato Araujo Oliveira Filho on 2017-01-18

Fix grammar.

[Pat McGowan (pat-mcgowan)]

Can you explain how this change addresses the issue?

[Carlos Jose Mazieri (carlos-mazieri)]

Renato,

The bug description appears to say that it always crashes.

Can you confirm that it always crashes or it happens under some special condition?

If it happens under special condition can you describe that scenario?

review: Needs Information

[Renato Araujo Oliveira Filho (renatofilho)]

> Renato,
>
> The bug description appears to say that it always crashes.
>
> Can you confirm that it always crashes or it happens under some special
> condition?
>
> If it happens under special condition can you describe that scenario?

It always crash for me.

[Carlos Jose Mazieri (carlos-mazieri)]

Does it crash on both phone and desktop or just phone?

review: Needs Information

[Carlos Jose Mazieri (carlos-mazieri)]

OK, Let's have a review on it,

1. perhaps the slot onParentDestroyed() on location.h lines 94-96 is not being used.

2. on line 38 from networklistworker.cpp we should check if m_parent != 0,
   the default value for 'parent' is 0 on networklistworker.h

   38: parent->connectDestructionSignal(this, SLOT(onParentDestroyed()));

3. are you sure the code from networklistworker.cpp lines 57-58 are really necessary?
   the default value for 'parent' is 0 on networklistworker.h, and, if onParentDestroyed() is called
   m_parent is set to 0 and the "m_parent != 0" is checked on line 61.

   57: if (!m_parent)
   58: return netContent;

   61: bool is_parent_of_smb_url = m_parent != 0 && m_parent->urlPath().startsWith(LocationUrl::SmbURL);

   if you want keep lines 57-58, please change to "if (m_parent != 0)" as m_parent is not a boolean type.

4. can you explain why you removed the call to "refreshInfo()" from Location::currentInfo()
   on location.cpp line 329?

   I see that both Location::refreshInfo() and Location::becomeParent() delete the 'm_info',
   this is the 'parent' used in NetworkListWorker class on the second thread.
   That may be the real problem, perhaps just making an attribution into 'm_info'
   from the temporary item could solve the entire problem, see a grep output:

     ./src/plugin/folderlistmodel/trash/trashlocation.cpp:57: delete m_info;
     ./src/plugin/folderlistmodel/trash/trashlocation.cpp:176: delete m_info;
     ./src/plugin/folderlistmodel/location.cpp:64: delete m_info;
     ./src/plugin/folderlistmodel/location.cpp:96: delete m_info;
     ./src/plugin/folderlistmodel/location.cpp:238: delete m_info;
     ./src/plugin/folderlistmodel/location.cpp:252: delete m_info;

   Would you consider (adding this) or reconsider for this item #4 only?

review: Needs Information

[Pat McGowan (pat-mcgowan)]

It always crashes and on both phone and desktop

[Renato Araujo Oliveira Filho (renatofilho)]

Thanks for reviewing it.

> OK, Let's have a review on it,
>
> 1. perhaps the slot onParentDestroyed() on location.h lines 94-96 is not being
> used.
REMOVED

>
> 2. on line 38 from networklistworker.cpp we should check if m_parent != 0,
> the default value for 'parent' is 0 on networklistworker.h
>
> 38: parent->connectDestructionSignal(this, SLOT(onParentDestroyed()));
FIXED

>
> 3. are you sure the code from networklistworker.cpp lines 57-58 are really
> necessary?
> the default value for 'parent' is 0 on networklistworker.h, and, if
> onParentDestroyed() is called
> m_parent is set to 0 and the "m_parent != 0" is checked on line 61.
>
> 57: if (!m_parent)
> 58: return netContent;
>
> 61: bool is_parent_of_smb_url = m_parent != 0 &&
> m_parent->urlPath().startsWith(LocationUrl::SmbURL);
>
> if you want keep lines 57-58, please change to "if (m_parent != 0)" as
> m_parent is not a boolean type.
This is not necessary (REMOVED)

>
> 4. can you explain why you removed the call to "refreshInfo()" from
> Location::currentInfo()
> on location.cpp line 329?
>
> I see that both Location::refreshInfo() and Location::becomeParent() delete
> the 'm_info',
> this is the 'parent' used in NetworkListWorker class on the second thread.
> That may be the real problem, perhaps just making an attribution into
> 'm_info'
> from the temporary item could solve the entire problem, see a grep output:
>
> ./src/plugin/folderlistmodel/trash/trashlocation.cpp:57:
> delete m_info;
> ./src/plugin/folderlistmodel/trash/trashlocation.cpp:176: delete
> m_info;
> ./src/plugin/folderlistmodel/location.cpp:64: delete m_info;
> ./src/plugin/folderlistmodel/location.cpp:96: delete m_info;
> ./src/plugin/folderlistmodel/location.cpp:238: delete m_info;
> ./src/plugin/folderlistmodel/location.cpp:252: delete m_info;
>
> Would you consider (adding this) or reconsider for this item #4 only?

We not not have a pointer to the temporary object (NetworkWorker) from Location class. Or do you have a way to access it?

[lp:~renatofilho/ubuntu-filemanager-app/fix-network-crash]

588. By Renato Araujo Oliveira Filho on 2017-01-25

Remove unused code.
Check for parent before use it.

[Carlos Jose Mazieri (carlos-mazieri)]

Renato,

by temporary objects I mean a temporary DirItemInfo created in Location?? classes, example:

void Location::refreshInfo()
{
    if (m_info)
    {
        DirItemInfo *item = newItemInfo(m_info->absoluteFilePath());
        delete m_info;
        m_info = item;
    }
}

Would be changed by:

void Location::refreshInfo()
{
    if (m_info)
    {
        DirItemInfo *item = newItemInfo(m_info->absoluteFilePath());
        *m_info = *item; // current m_info item receives current information
        delete item;
    }
}

I am not sure the operator "=" works properly for all DirItemInfo descendant classes.

[Carlos Jose Mazieri (carlos-mazieri)]

Another and easy approach would be if the class NetworkListWorker receive again a const DirItemInfo *parent, but sets m_parent as its own instance using the operator '=', in this case it never gets deleted outside the worker thread.

Suppose the code changed:

NetworkListWorker::NetworkListWorker(LocationItemDirIterator * dirIterator,
                                     DirItemInfo * mainItemInfo,
                                     const DirItemInfo *parent) :
    DirListWorker(dirIterator->path(),
                  dirIterator->filters(),
                  dirIterator->flags() == QDirIterator::Subdirectories ? true : false),
    m_dirIterator(dirIterator),
    m_mainItemInfo(mainItemInfo),
    m_parent(0)
{
     if (parent != 0)
     {
        m_parent = new UrlItemInfo(); // UrlItemInfo is for remote
        *m_parent = *parent; // not sure it works, needs be reviewed
     }
     mLoaderType = NetworkLoader;

     // this would not be necessary and can be removed
     parent->connectDestructionSignal(this, SLOT(onParentDestroyed()));
}

NetworkListWorker::~NetworkListWorker()
{
    delete m_dirIterator;
    delete m_mainItemInfo;
    if (m_parent != 0)
    {
       delete m_parent;
    }
}

[Arto Jalkanen (ajalkane)]

I'm a bit dumbfounded that this kind of Qt metaprogramming would be necessary for solving a problem as basic as what we have here. At the minimum I think this code would need to be commented, because if anyone stumbles upon this code in the future it'd be a big WTF moment without clarifying comments. Comments should explain what it fixes, why it had to be done this way.

[Renato Araujo Oliveira Filho (renatofilho)]

> Another and easy approach would be if the class NetworkListWorker receive
> again a const DirItemInfo *parent, but sets m_parent as its own instance using
> the operator '=', in this case it never gets deleted outside the worker
> thread.
>
> Suppose the code changed:
>
>
> NetworkListWorker::NetworkListWorker(LocationItemDirIterator * dirIterator,
> DirItemInfo * mainItemInfo,
> const DirItemInfo *parent) :
> DirListWorker(dirIterator->path(),
> dirIterator->filters(),
> dirIterator->flags() == QDirIterator::Subdirectories ? true
> : false),
> m_dirIterator(dirIterator),
> m_mainItemInfo(mainItemInfo),
> m_parent(0)
> {
> if (parent != 0)
> {
> m_parent = new UrlItemInfo(); // UrlItemInfo is for remote
> *m_parent = *parent; // not sure it works, needs be reviewed
> }
> mLoaderType = NetworkLoader;
>
> // this would not be necessary and can be removed
> parent->connectDestructionSignal(this, SLOT(onParentDestroyed()));
> }
>
>
> NetworkListWorker::~NetworkListWorker()
> {
> delete m_dirIterator;
> delete m_mainItemInfo;
> if (m_parent != 0)
> {
> delete m_parent;
> }
> }

With that code the operation will be executed even after the "parent" object be destroyed what we do not want. It is waste of resource.

[Renato Araujo Oliveira Filho (renatofilho)]

> I'm a bit dumbfounded that this kind of Qt metaprogramming would be necessary
> for solving a problem as basic as what we have here. At the minimum I think
> this code would need to be commented, because if anyone stumbles upon this
> code in the future it'd be a big WTF moment without clarifying comments.
> Comments should explain what it fixes, why it had to be done this way.

The Qt metaprogramming make things easy here. Avoid big changes. Without that we will need to keep a list of child objects on the parent and invalidate all the children when parent get destroyed.

Instead of that I am keep all the changes on child object and keeping track of parent life. Any suggestion are welcome.

[Carlos Jose Mazieri (carlos-mazieri)]

I would remove all the Qt metaprogramming as it is not necessary,

Only a small change in the NetworkListWorker as exposed above should solve the problem, NetworkListWorker class can create its own instance by doing a copy.

[Carlos Jose Mazieri (carlos-mazieri)]

Renato,

Can you please test that proposal of that NetworkListWorker creating its own instance of the parent item?

There is a proposal https://code.launchpad.net/~carlos-mazieri/ubuntu-filemanager-app//fix-network-crash-1609051

Also renamed "parent" to "parentItemInfo".

[Renato Araujo Oliveira Filho (renatofilho)]

> Renato,
>
> Can you please test that proposal of that NetworkListWorker creating its own
> instance of the parent item?
>
> There is a proposal https://code.launchpad.net/~carlos-mazieri/ubuntu-
> filemanager-app//fix-network-crash-1609051
>
> Also renamed "parent" to "parentItemInfo".

Thanks, Carlos will be a pleasure test that.

But as mentioned before if the parent get destroyed the operation will not be canceled and will be executed until the end. (In my opinion this is a wast of resources, but if you are ok with that)

Based on that information I do not think you need to check for (m_parentItemInfo != 0) in line 61. Since m_parentItemInfo will never be null.

[Carlos Jose Mazieri (carlos-mazieri)]

Renato,

The check in line 61 is necessary.

The 'parentItemInfo' keeps the information from the parent, which can be: directory/share/host/workgroup.

When the 'mainItemInfo' is root like "smb://" there is no 'parentItemInfo', it can be null.

Another reason it might be used in my regression tests (I am not sure if it is), you can use if you want its in "src/plugin/test_folderlistmodel/regression/regression_folderlilstmodel.pro"

[Renato Araujo Oliveira Filho (renatofilho)]

Closing this MR

Replaced by: https://code.launchpad.net/~carlos-mazieri/ubuntu-filemanager-app/fix-network-crash-1609051/+merge/315772

review: Disapprove

[Jenkins Bot (ubuntu-core-apps-jenkins-bot)]

FAILED: Continuous integration, rev:588
https://core-apps-jenkins.ubuntu.com/job/filemanager-app-ci/96/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/1001/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/filemanager-app-ci/96/rebuild

review: Needs Fixing (continuous-integration)

Unmerged revisions

588. By Renato Araujo Oliveira Filho on 2017-01-25

Remove unused code.
Check for parent before use it.

587. By Renato Araujo Oliveira Filho on 2017-01-18

Fix grammar.

586. By Renato Araujo Oliveira Filho on 2017-01-17

Remove extra debug messages.

585. By Renato Araujo Oliveira Filho on 2017-01-17

Fixed memory leak;

Make sure that works are deleted.

584. By Renato Araujo Oliveira Filho on 2017-01-16

Avoid crash when aborting a network operation.

Clear parent pointer on worker if parent get destroyed.

=== modified file 'src/plugin/folderlistmodel/diriteminfo.cpp'
--- src/plugin/folderlistmodel/diriteminfo.cpp	2015-12-12 12:30:47 +0000
+++ src/plugin/folderlistmodel/diriteminfo.cpp	2017-01-25 16:54:04 +0000
@@ -23,6 +23,8 @@
 #include "locationurl.h"
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <QMetaMethod>
+#include <QDebug>
 
 
 QMimeDatabase DirItemInfoPrivate::mimeDatabase;
@@ -151,6 +153,13 @@
 
 DirItemInfo::~DirItemInfo()
 {
+    Q_FOREACH(const DestructionSlot &slot, d_ptr->_destructionSlots) {
+        if (slot.first) {
+            const QMetaObject *mObject = slot.first->metaObject();
+            QMetaMethod method = mObject->method(slot.second);
+            method.invoke(slot.first.data(), Qt::DirectConnection);
+        }
+    }
 }
 
 
@@ -602,3 +611,13 @@
 {
     d_ptr->_isNetworkShare = true;
 }
+
+void DirItemInfo::connectDestructionSignal(QObject *obj, const QByteArray &slot)
+{
+    int index = obj->metaObject()->indexOfSlot(slot.constData()+1);
+    if (index == -1) {
+        qWarning() << "Object" << obj << "does not have slot named:" << slot;
+    } else {
+        d_ptr->_destructionSlots.append(qMakePair(QPointer<QObject>(obj), index));
+    }
+}

=== modified file 'src/plugin/folderlistmodel/diriteminfo.h'
--- src/plugin/folderlistmodel/diriteminfo.h	2015-12-12 12:30:47 +0000
+++ src/plugin/folderlistmodel/diriteminfo.h	2017-01-25 16:54:04 +0000
@@ -30,6 +30,8 @@
 #include <QDir>
 #include <QMimeType>
 #include <QMimeDatabase>
+#include <QPair>
+#include <QPointer>
 
 class DirItemInfoPrivate;
 
@@ -114,6 +116,7 @@
     void              fillFromStatBuf(const struct stat& statBuffer);
     void              setAsHost();
     void              setAsShare();
+    void              connectDestructionSignal(QObject *obj, const QByteArray &slot);
 
 public:
     static QString    removeExtraSlashes(const QString &url, int firstSlashIndex = -1);
@@ -134,6 +137,7 @@
 };
 
 typedef QVector<DirItemInfo>   DirItemInfoList;
+typedef QPair<QPointer<QObject>, int> DestructionSlot;
 
 Q_DECLARE_SHARED(DirItemInfo)
 Q_DECLARE_METATYPE(DirItemInfo)
@@ -180,6 +184,8 @@
     QString   _normalizedPath;
     QString   _authenticationPath;
 
+    QVector<DestructionSlot> _destructionSlots;
+
     static QMimeDatabase mimeDatabase;
 };
 

=== modified file 'src/plugin/folderlistmodel/iorequestworker.cpp'
--- src/plugin/folderlistmodel/iorequestworker.cpp	2013-10-12 19:25:17 +0000
+++ src/plugin/folderlistmodel/iorequestworker.cpp	2017-01-25 16:54:04 +0000
@@ -36,6 +36,8 @@
 #include <QDateTime>
 #include <QDebug>
 
+#define ORIGNAL_THREAD "ORIGINAL_THREAD"
+
 /*!
   Lives on an IOWorkerThread.
 
@@ -55,6 +57,7 @@
                  << Q_FUNC_INFO;
 #endif
 
+    request->setProperty(ORIGNAL_THREAD, QVariant::fromValue<QThread*>(request->thread()));
     request->moveToThread(this);
 
     // TODO: queue requests so we run the most important one first
@@ -82,7 +85,11 @@
             lock.unlock();
 
             request->run();
+
+            // transfer back to main loop to make sure that will be destoyed by deleteLater
+            request->moveToThread(request->property(ORIGNAL_THREAD).value<QThread*>());
             request->deleteLater();
+
             lock.relock();
         }
     }

=== modified file 'src/plugin/folderlistmodel/location.cpp'
--- src/plugin/folderlistmodel/location.cpp	2015-12-08 16:55:41 +0000
+++ src/plugin/folderlistmodel/location.cpp	2017-01-25 16:54:04 +0000
@@ -326,6 +326,5 @@
     {
         m_info = new DirItemInfo();
     }
-    refreshInfo(); //update information
     return m_info;
 }

=== modified file 'src/plugin/folderlistmodel/location.h'
--- src/plugin/folderlistmodel/location.h	2015-12-12 13:59:49 +0000
+++ src/plugin/folderlistmodel/location.h	2017-01-25 16:54:04 +0000
@@ -91,7 +91,6 @@
     virtual void setAuthentication(const QString& user,
                                    const QString& password);
 
-
 public: //pure functions
     /*!
      * \brief newItemInfo()  returns a Location suitable DirItemInfo object

=== 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-25 16:54:04 +0000
@@ -23,8 +23,10 @@
 #include "locationitemdiriterator.h"
 #include "locationurl.h"
 
+#include <QDebug>
+
 NetworkListWorker::NetworkListWorker(LocationItemDirIterator * dirIterator,
-                                     DirItemInfo * mainItemInfo, const DirItemInfo *parent) :
+                                     DirItemInfo * mainItemInfo, DirItemInfo *parent) :
     DirListWorker(dirIterator->path(),
                   dirIterator->filters(),
                   dirIterator->flags() == QDirIterator::Subdirectories ? true : false),
@@ -33,6 +35,8 @@
     m_parent(parent)
 {
      mLoaderType =  NetworkLoader;
+     if (parent)
+        parent->connectDestructionSignal(this, SLOT(onParentDestroyed()));
 }
 
 
@@ -42,10 +46,15 @@
     delete m_mainItemInfo;
 }
 
+void NetworkListWorker::onParentDestroyed()
+{
+    m_parent = 0;
+}
 
 DirItemInfoList NetworkListWorker::getNetworkContent()
 {
      DirItemInfoList netContent;
+
      m_dirIterator->load();
      bool is_parent_of_smb_url = m_parent != 0 && m_parent->urlPath().startsWith(LocationUrl::SmbURL);
      while (m_dirIterator->hasNext())

=== 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-25 16:54:04 +0000
@@ -40,11 +40,15 @@
 public:
     NetworkListWorker(LocationItemDirIterator * dirIterator,
                       DirItemInfo             * mainItemInfo,
-                      const DirItemInfo       * parent = 0);
+                      DirItemInfo             * parent = 0);
     ~NetworkListWorker();
+public slots:
+    void    onParentDestroyed();
+
 protected:
     virtual DirItemInfoList getNetworkContent();
     void                    setSmbItemAttributes();
+
 protected:
      LocationItemDirIterator     * m_dirIterator;
      DirItemInfo                 * m_mainItemInfo;