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 :).
Details
Diff Detail
- Repository
- R167 Krusader
- Branch
- arcpatch-D5292
- Lint
No Linters Available - Unit
No Unit Test Coverage
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.
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.
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; }
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.
- Fix for crash when closing currently refreshing panel
- Safer version of "Fix for crash when closing currently refreshing panel" (f01640db)
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.
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());
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.