Fix for crash when closing currently refreshing panel
ClosedPublic

Authored by martinkostolny on Apr 3 2017, 10:32 PM.

Details

Reviewers
abika
janlepper
Group Reviewers
Krusader
Summary

Here is a little patch to fix the crash. I believe it was introduced in commit ec93890d69a93360db97fb5d14aaf36719752dce. I hope I didn't break something else by this :).

Test Plan

Open a new tab and go to remote url so it takes some time to connect. Then close the tab and observe Krusader does not crash.

Diff Detail

Repository
R167 Krusader
Branch
arcpatch-D5292
Lint
No Linters Available
Unit
No Unit Test Coverage
martinkostolny created this revision.Apr 3 2017, 10:32 PM
martinkostolny edited the summary of this revision. (Show Details)
janlepper added a subscriber: janlepper.EditedApr 4 2017, 10:08 AM

I'm sorry for the regression.
I assumed this situation (a panel was destroyed while still refreshing) wouldn't occur.
There has been code in place to prevent this, but looking at PanelManager::deletePanel(), the panel is now unconditionally deleted.
So a proper fix would be to re-introduce the previous behaviour.
Asynchronous refresh of course would also fix that problem.

abika accepted this revision.Apr 4 2017, 2:33 PM
abika added a reviewer: janlepper.
abika added a subscriber: abika.

There has been code in place to prevent this, but looking at PanelManager::deletePanel(), the panel is now unconditionally deleted.
So a proper fix would be to re-introduce the previous behaviour.

I removed the whole code together with the previous check

if (p && p->func && p->func->files() && !p->func->files()->vfs_canDelete()) {
    connect(p->func->files(), SIGNAL(deleteAllowed()), p, SLOT(deleteLater()));
    p->func->files()->vfs_requestDelete();
    return;

mainly because it doesn't follow the principle of encapsulation in OOP. The panel manager should not have to check the inner state of the filesystem refresh invoked by the panel function object.
This fix is totally fine imo.

Asynchronous refresh of course would also fix that problem.

Yes. That would be the "correct" way instead of a workaround.

This revision is now accepted and ready to land.Apr 4 2017, 2:33 PM
janlepper edited edge metadata.EditedApr 4 2017, 10:12 PM
In D5292#99747, @abika wrote:

There has been code in place to prevent this, but looking at PanelManager::deletePanel(), the panel is now unconditionally deleted.
So a proper fix would be to re-introduce the previous behaviour.

I removed the whole code together with the previous check

if (p && p->func && p->func->files() && !p->func->files()->vfs_canDelete()) {
    connect(p->func->files(), SIGNAL(deleteAllowed()), p, SLOT(deleteLater()));
    p->func->files()->vfs_requestDelete();
    return;

mainly because it doesn't follow the principle of encapsulation in OOP. The panel manager should not have to check the inner state of the filesystem refresh invoked by the panel function object.
This fix is totally fine imo.

The code was ugly, but accessing a deleted object results in undefinefd behaviour.

Asynchronous refresh of course would also fix that problem.

Yes. That would be the "correct" way instead of a workaround.

I'm still committed to implement this - for now I suggest something like

void ListPanel::destroy() {
  if (func->files()->isRefreshing())
    _deleteRequested = true:
  else
    deleteLater();
}

and in ListPanelFunc::refresh()

const bool refreshed = fileSystemP->refresh(url); 
if (panel->_deleteRequested) {
  panel->deleteLater();
  return;
}
abika added a comment.Apr 5 2017, 9:01 PM

The code was ugly, but accessing a deleted object results in undefinefd behaviour.

True, the !panel check is incorrect. How about just wrapping panel in a QPointer? I still don't like that the panel cannot be safely deleted from another class.

abika updated this revision to Diff 13235.Apr 8 2017, 3:40 PM
  • Fix for crash when closing currently refreshing panel
  • Safer version of "Fix for crash when closing currently refreshing panel" (f01640db)
abika added a comment.Apr 8 2017, 3:42 PM

Ok now?

(Tested: still working)

Sorry for my late reaction. Yes, this is nicer and working. Thanks, Alex! To avoid waiting of each other: You do the commit :).

Sorry for the even later reaction ;)
Well, the QPointer doesn't hurt, but it doesn't fix the problem in this case.
Actually the ListPanelFunc object is destroyed before the ListPanel, so when you do the check for this->panel, this points to already freed memory.

janlepper added a comment.EditedApr 9 2017, 3:12 PM

Hmm, I can't update the diff (permissions?)

This is safer:

diff --git a/krusader/Panel/panelfunc.cpp b/krusader/Panel/panelfunc.cpp
index 3f2b184f7..c3dcaff41 100644
--- a/krusader/Panel/panelfunc.cpp
+++ b/krusader/Panel/panelfunc.cpp
@@ -310,11 +310,15 @@ void ListPanelFunc::doRefresh()
 
     // NOTE: this is blocking. Returns false on error or interruption (cancel requested or panel
     // was deleted)
+    QPointer<ListPanel> panelSave = panel;
     const bool refreshed = fileSystemP->refresh(url);
     if (refreshed) {
         // update the history and address bar, as the actual url might differ from the one requested
         history->setCurrentUrl(fileSystemP->currentDirectory());
         panel->setNavigatorUrl(fileSystemP->currentDirectory());
+    } else if (!panelSave) {
+        // panel was deleted while refreshing
+        return;
     }
 
     panel->view->setNameToMakeCurrent(QString());

Hmm, I can't update the diff (permissions?)

This is safer:

diff --git a/krusader/Panel/panelfunc.cpp b/krusader/Panel/panelfunc.cpp
index 3f2b184f7..c3dcaff41 100644
--- a/krusader/Panel/panelfunc.cpp
+++ b/krusader/Panel/panelfunc.cpp
@@ -310,11 +310,15 @@ void ListPanelFunc::doRefresh()
 
     // NOTE: this is blocking. Returns false on error or interruption (cancel requested or panel
     // was deleted)
+    QPointer<ListPanel> panelSave = panel;
     const bool refreshed = fileSystemP->refresh(url);
     if (refreshed) {
         // update the history and address bar, as the actual url might differ from the one requested
         history->setCurrentUrl(fileSystemP->currentDirectory());
         panel->setNavigatorUrl(fileSystemP->currentDirectory());
+    } else if (!panelSave) {
+        // panel was deleted while refreshing
+        return;
     }
 
     panel->view->setNameToMakeCurrent(QString());

Or even better:

diff --git a/krusader/Panel/panelfunc.cpp b/krusader/Panel/panelfunc.cpp
index 3f2b184f7..c3dcaff41 100644
--- a/krusader/Panel/panelfunc.cpp
+++ b/krusader/Panel/panelfunc.cpp
@@ -310,11 +310,15 @@ void ListPanelFunc::doRefresh()
 
     // NOTE: this is blocking. Returns false on error or interruption (cancel requested or panel
     // was deleted)
+    QPointer<ListPanelFunc> thisSave = this;
     const bool refreshed = fileSystemP->refresh(url);
     if (refreshed) {
         // update the history and address bar, as the actual url might differ from the one requested
         history->setCurrentUrl(fileSystemP->currentDirectory());
         panel->setNavigatorUrl(fileSystemP->currentDirectory());
+    } else if (!thisSave) {
+        // panel was deleted while refreshing
+        return;
     }
 
     panel->view->setNameToMakeCurrent(QString());
abika added a comment.Apr 9 2017, 4:37 PM

Actually the ListPanelFunc object is destroyed before the ListPanel, so when you do the check for this->panel, this points to already freed memory.

Oh heck. I didn't know that and assumed this is still valid in the method. But it is "undefined behaviour". Well, I'm too spoiled by Java.

Just commit your patch and we can throw this diff away.

abika closed this revision.Apr 11 2017, 5:41 PM

I took the liberty and commited your patch.