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

Proposed by Renato Araujo Oliveira Filho on 2017-01-16
Status: Needs review
Proposed branch: lp:~renatofilho/ubuntu-filemanager-app/fix-network-crash
Merge into: lp:ubuntu-filemanager-app
Diff against target: 196 lines (+47/-4)
7 files modified
src/plugin/folderlistmodel/diriteminfo.cpp (+19/-0)
src/plugin/folderlistmodel/diriteminfo.h (+6/-0)
src/plugin/folderlistmodel/iorequestworker.cpp (+7/-0)
src/plugin/folderlistmodel/location.cpp (+0/-1)
src/plugin/folderlistmodel/location.h (+0/-1)
src/plugin/folderlistmodel/networklistworker.cpp (+10/-1)
src/plugin/folderlistmodel/networklistworker.h (+5/-1)
To merge this branch: bzr merge lp:~renatofilho/ubuntu-filemanager-app/fix-network-crash
Reviewer Review Type Date Requested Status
Jenkins Bot continuous-integration Needs Fixing on 2017-02-17
Renato Araujo Oliveira Filho (community) Disapprove on 2017-01-27
Carlos Jose Mazieri 2017-01-16 Needs Information on 2017-01-25
Review via email: mp+314870@code.launchpad.net

Commit message

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.

To post a comment you must log in.
587. By Renato Araujo Oliveira Filho on 2017-01-18

Fix grammar.

Pat McGowan (pat-mcgowan) wrote :

Can you explain how this change addresses the issue?

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,
>
> 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.

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

review: Needs Information

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) wrote :

It always crashes and on both phone and desktop

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?

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

Remove unused code.
Check for parent before use it.

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.

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) wrote :

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.

> 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.

> 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.

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.

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,
>
> 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.

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"

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/plugin/folderlistmodel/diriteminfo.cpp'
2--- src/plugin/folderlistmodel/diriteminfo.cpp 2015-12-12 12:30:47 +0000
3+++ src/plugin/folderlistmodel/diriteminfo.cpp 2017-01-25 16:54:04 +0000
4@@ -23,6 +23,8 @@
5 #include "locationurl.h"
6 #include <sys/types.h>
7 #include <sys/stat.h>
8+#include <QMetaMethod>
9+#include <QDebug>
10
11
12 QMimeDatabase DirItemInfoPrivate::mimeDatabase;
13@@ -151,6 +153,13 @@
14
15 DirItemInfo::~DirItemInfo()
16 {
17+ Q_FOREACH(const DestructionSlot &slot, d_ptr->_destructionSlots) {
18+ if (slot.first) {
19+ const QMetaObject *mObject = slot.first->metaObject();
20+ QMetaMethod method = mObject->method(slot.second);
21+ method.invoke(slot.first.data(), Qt::DirectConnection);
22+ }
23+ }
24 }
25
26
27@@ -602,3 +611,13 @@
28 {
29 d_ptr->_isNetworkShare = true;
30 }
31+
32+void DirItemInfo::connectDestructionSignal(QObject *obj, const QByteArray &slot)
33+{
34+ int index = obj->metaObject()->indexOfSlot(slot.constData()+1);
35+ if (index == -1) {
36+ qWarning() << "Object" << obj << "does not have slot named:" << slot;
37+ } else {
38+ d_ptr->_destructionSlots.append(qMakePair(QPointer<QObject>(obj), index));
39+ }
40+}
41
42=== modified file 'src/plugin/folderlistmodel/diriteminfo.h'
43--- src/plugin/folderlistmodel/diriteminfo.h 2015-12-12 12:30:47 +0000
44+++ src/plugin/folderlistmodel/diriteminfo.h 2017-01-25 16:54:04 +0000
45@@ -30,6 +30,8 @@
46 #include <QDir>
47 #include <QMimeType>
48 #include <QMimeDatabase>
49+#include <QPair>
50+#include <QPointer>
51
52 class DirItemInfoPrivate;
53
54@@ -114,6 +116,7 @@
55 void fillFromStatBuf(const struct stat& statBuffer);
56 void setAsHost();
57 void setAsShare();
58+ void connectDestructionSignal(QObject *obj, const QByteArray &slot);
59
60 public:
61 static QString removeExtraSlashes(const QString &url, int firstSlashIndex = -1);
62@@ -134,6 +137,7 @@
63 };
64
65 typedef QVector<DirItemInfo> DirItemInfoList;
66+typedef QPair<QPointer<QObject>, int> DestructionSlot;
67
68 Q_DECLARE_SHARED(DirItemInfo)
69 Q_DECLARE_METATYPE(DirItemInfo)
70@@ -180,6 +184,8 @@
71 QString _normalizedPath;
72 QString _authenticationPath;
73
74+ QVector<DestructionSlot> _destructionSlots;
75+
76 static QMimeDatabase mimeDatabase;
77 };
78
79
80=== modified file 'src/plugin/folderlistmodel/iorequestworker.cpp'
81--- src/plugin/folderlistmodel/iorequestworker.cpp 2013-10-12 19:25:17 +0000
82+++ src/plugin/folderlistmodel/iorequestworker.cpp 2017-01-25 16:54:04 +0000
83@@ -36,6 +36,8 @@
84 #include <QDateTime>
85 #include <QDebug>
86
87+#define ORIGNAL_THREAD "ORIGINAL_THREAD"
88+
89 /*!
90 Lives on an IOWorkerThread.
91
92@@ -55,6 +57,7 @@
93 << Q_FUNC_INFO;
94 #endif
95
96+ request->setProperty(ORIGNAL_THREAD, QVariant::fromValue<QThread*>(request->thread()));
97 request->moveToThread(this);
98
99 // TODO: queue requests so we run the most important one first
100@@ -82,7 +85,11 @@
101 lock.unlock();
102
103 request->run();
104+
105+ // transfer back to main loop to make sure that will be destoyed by deleteLater
106+ request->moveToThread(request->property(ORIGNAL_THREAD).value<QThread*>());
107 request->deleteLater();
108+
109 lock.relock();
110 }
111 }
112
113=== modified file 'src/plugin/folderlistmodel/location.cpp'
114--- src/plugin/folderlistmodel/location.cpp 2015-12-08 16:55:41 +0000
115+++ src/plugin/folderlistmodel/location.cpp 2017-01-25 16:54:04 +0000
116@@ -326,6 +326,5 @@
117 {
118 m_info = new DirItemInfo();
119 }
120- refreshInfo(); //update information
121 return m_info;
122 }
123
124=== modified file 'src/plugin/folderlistmodel/location.h'
125--- src/plugin/folderlistmodel/location.h 2015-12-12 13:59:49 +0000
126+++ src/plugin/folderlistmodel/location.h 2017-01-25 16:54:04 +0000
127@@ -91,7 +91,6 @@
128 virtual void setAuthentication(const QString& user,
129 const QString& password);
130
131-
132 public: //pure functions
133 /*!
134 * \brief newItemInfo() returns a Location suitable DirItemInfo object
135
136=== modified file 'src/plugin/folderlistmodel/networklistworker.cpp'
137--- src/plugin/folderlistmodel/networklistworker.cpp 2015-12-12 14:32:18 +0000
138+++ src/plugin/folderlistmodel/networklistworker.cpp 2017-01-25 16:54:04 +0000
139@@ -23,8 +23,10 @@
140 #include "locationitemdiriterator.h"
141 #include "locationurl.h"
142
143+#include <QDebug>
144+
145 NetworkListWorker::NetworkListWorker(LocationItemDirIterator * dirIterator,
146- DirItemInfo * mainItemInfo, const DirItemInfo *parent) :
147+ DirItemInfo * mainItemInfo, DirItemInfo *parent) :
148 DirListWorker(dirIterator->path(),
149 dirIterator->filters(),
150 dirIterator->flags() == QDirIterator::Subdirectories ? true : false),
151@@ -33,6 +35,8 @@
152 m_parent(parent)
153 {
154 mLoaderType = NetworkLoader;
155+ if (parent)
156+ parent->connectDestructionSignal(this, SLOT(onParentDestroyed()));
157 }
158
159
160@@ -42,10 +46,15 @@
161 delete m_mainItemInfo;
162 }
163
164+void NetworkListWorker::onParentDestroyed()
165+{
166+ m_parent = 0;
167+}
168
169 DirItemInfoList NetworkListWorker::getNetworkContent()
170 {
171 DirItemInfoList netContent;
172+
173 m_dirIterator->load();
174 bool is_parent_of_smb_url = m_parent != 0 && m_parent->urlPath().startsWith(LocationUrl::SmbURL);
175 while (m_dirIterator->hasNext())
176
177=== modified file 'src/plugin/folderlistmodel/networklistworker.h'
178--- src/plugin/folderlistmodel/networklistworker.h 2015-12-12 14:32:18 +0000
179+++ src/plugin/folderlistmodel/networklistworker.h 2017-01-25 16:54:04 +0000
180@@ -40,11 +40,15 @@
181 public:
182 NetworkListWorker(LocationItemDirIterator * dirIterator,
183 DirItemInfo * mainItemInfo,
184- const DirItemInfo * parent = 0);
185+ DirItemInfo * parent = 0);
186 ~NetworkListWorker();
187+public slots:
188+ void onParentDestroyed();
189+
190 protected:
191 virtual DirItemInfoList getNetworkContent();
192 void setSmbItemAttributes();
193+
194 protected:
195 LocationItemDirIterator * m_dirIterator;
196 DirItemInfo * m_mainItemInfo;

Subscribers

People subscribed via source and target branches