Fix crash in Folder::isSyncRunning() #3586
Conversation
| @@ -252,7 +252,7 @@ bool Folder::isBusy() const | |||
|
|
|||
| bool Folder::isSyncRunning() const | |||
| { | |||
| return _engine->isSyncRunning() || _vfs->isHydrating(); | |||
| return _engine->isSyncRunning() || (_vfs && _vfs->isHydrating()); | |||
There was a problem hiding this comment.
Is it okay to have no _vfs object or is that a state that should not be encountered?
There was a problem hiding this comment.
Is it okay to have no _vfs object or is that a state that should not be encountered?
@FlexW I see that in void Folder::wipeForRemoval() the _vfs is set to nullptr. Also, in some other methods, there is a check for not null _vfs.
There was a problem hiding this comment.
@FlexW Just confirmed under the debugger. void Folder::wipeForRemoval() is called before bool Folder::isSyncRunning() const, hence, _vfs is already set to nullptr. So, that check is needed.
_vfs.reset(nullptr); // warning: folder now in an invalid state
That comment, though, is weird. Not sure if it's ok to have _vfs set to nullptr here 😕
There was a problem hiding this comment.
when is Folder::wipeForRemoval() called before calling into Folder::isSyncRunning ?
There was a problem hiding this comment.
@FlexW Just confirmed under the debugger.
void Folder::wipeForRemoval()is called beforebool Folder::isSyncRunning() const, hence,_vfsis already set tonullptr. So, that check is needed.
_vfs.reset(nullptr); // warning: folder now in an invalid state
That comment, though, is weird. Not sure if it's ok to have_vfsset tonullptrhere confused
looks like other code expect that so I would say that even if weird seems the original author idea
There was a problem hiding this comment.
when is
Folder::wipeForRemoval()called before calling intoFolder::isSyncRunning?
@mgallien This happens when requesting a remote wipe. That's how I was able to discover this crash. I am sure we need to add this check now. Otherwise, I have no other idea how to fix it quickly.
There was a problem hiding this comment.
can you quickly try to extend the tests in TestFolderMan class to call into Folder::isSyncRunning() ?
that would be nice
There was a problem hiding this comment.
@mgallien Done.
|
/rebase |
github-actions
bot
force-pushed
the
bugfix/fix-crash-folder-isSyncRunning
branch
from
00eadef to
e69511c
Compare
|
/rebase |
Signed-off-by: allexzander <blackslayer4@gmail.com>
Signed-off-by: allexzander <blackslayer4@gmail.com>
github-actions
bot
force-pushed
the
bugfix/fix-crash-folder-isSyncRunning
branch
from
e69511c to
88d18fd
Compare
|
AppImage file: Nextcloud-PR-3586-88d18fd5f31380387e30792d86607cfa7b4fd792-x86_64.AppImage |
Signed-off-by: allexzander blackslayer4@gmail.com