Prevent crash in dialog that warns user about vfs and e2ee #3369

FlexW · 2021-05-27T08:46:16Z

Signed-off-by: Felix Weilbach felix.weilbach@nextcloud.com

I know the root cause of the problem lies in the fact that we use a pointer from a model too long, but I wanted to make this fix as non-invasive as possible.

allexzander · 2021-05-27T08:52:00Z

src/gui/accountsettings.cpp

@@ -96,6 +101,10 @@ bool showEnableE2eeWithVirtualFilesWarningDialog()
    encryptButton->setText(AccountSettings::tr("Encrypt folder"));
    e2eeWithVirtualFilesWarningMsgBox.exec();

+    SyncJournalFileRecord record;
+    if (!folder->journalDb()->getFileRecord(path, &record) || !record.isValid()) {


@FlexW I think this check must be made after the dialog was closed.

exec() returns if the dialog was closed.

@FlexW Ok. But still, is it the correct place to check for folder existence? I mean, it's the code for the warning dialog, and, it will even be removed once we get the E2EE with VFS PR in, so this check will then be gone. Is it possible that the folder gets removed during us clicking the "Encrypt" even without having a warning dialog displayed? If you know what I mean.

No, I think the check is only necessary (and I think even then it's very rare that a user might encounter that case) when the dialog is displayed. I wanted to have it close to the dialog because then it's easier to remove it once the dialog is removed.

mgallien · 2021-05-27T10:42:24Z

/rebase

mgallien · 2021-05-28T12:15:28Z

Signed-off-by: Felix Weilbach felix.weilbach@nextcloud.com

I know the root cause of the problem lies in the fact that we use a pointer from a model too long, but I wanted to make this fix as non-invasive as possible.

what would you do if you were doing the "invasive" fix ?

FlexW · 2021-05-28T14:29:35Z

Signed-off-by: Felix Weilbach felix.weilbach@nextcloud.com
I know the root cause of the problem lies in the fact that we use a pointer from a model too long, but I wanted to make this fix as non-invasive as possible.

what would you do if you were doing the "invasive" fix ?

Use open() instead of exec() and move the part from AccountSettings::slotMarkSubfolderEncrypted() that encrypts the file into the signal handler of QDialog::finished(). Also I then would probably change the signature from AccountSettings:slotMarkSubfolderEncrypted() to make sure that the pointer of the model gets not used for too long.

allexzander · 2021-05-28T14:36:55Z

Signed-off-by: Felix Weilbach felix.weilbach@nextcloud.com
I know the root cause of the problem lies in the fact that we use a pointer from a model too long, but I wanted to make this fix as non-invasive as possible.

what would you do if you were doing the "invasive" fix ?

Use open() instead of exec() and move the part from AccountSettings::slotMarkSubfolderEncrypted() that encrypts the file into the signal handler of QDialog::finished(). Also I then would probably change the signature from AccountSettings:slotMarkSubfolderEncrypted() to make sure that the pointer of the model gets not used for too long.

I am not sure we need to put so much effort into something that will later be removed. I mean, this dialog, it's a temporary solution, right?
In my opinion, we are good as long as we don't have a crash.

mgallien · 2021-05-28T16:09:56Z

Signed-off-by: Felix Weilbach felix.weilbach@nextcloud.com
I know the root cause of the problem lies in the fact that we use a pointer from a model too long, but I wanted to make this fix as non-invasive as possible.

what would you do if you were doing the "invasive" fix ?

Use open() instead of exec() and move the part from AccountSettings::slotMarkSubfolderEncrypted() that encrypts the file into the signal handler of QDialog::finished(). Also I then would probably change the signature from AccountSettings:slotMarkSubfolderEncrypted() to make sure that the pointer of the model gets not used for too long.

I am not sure we need to put so much effort into something that will later be removed. I mean, this dialog, it's a temporary solution, right?
In my opinion, we are good as long as we don't have a crash.

We should not let code we think is not good enough land because we can never know that we will for sure clean it before the next release. Once code is in, we have to live with it. The clean up is never guaranteed.

We can have priorities shift that would prevent doing the clean up.
We can have work to do on other parts and we never do the clean up.
...

mgallien · 2021-06-01T12:59:33Z

@FlexW can you please take care of using open() and having a fully safe implementation ?
I would prefer to fix the current issues in master

src/gui/accountsettings.cpp

mgallien · 2021-06-15T21:35:24Z

src/gui/accountsettings.cpp

@@ -339,21 +342,34 @@ void AccountSettings::slotMarkSubfolderEncrypted(FolderStatusModel::SubFolderInf

    const auto folder = folderInfo->_folder;
    Q_ASSERT(folder);
+    const auto path = folderInfo->_path;
+    const auto fileId = folderInfo->_fileId;
+    const auto encryptFolder = [this, fileId, folder, path] {


are you sure folder pointer cannot be made invalid before the lambda is called ?
QMessageBox::open is only window modal but maybe the account and folder can be removed via the tray main dialog ?

Good catch. It could make sense to pass a folder alias/name and then, request it from the FolderManager within the lambda :)

mgallien

thanks
please clean history before merging

FlexW · 2021-06-17T12:17:57Z

/rebase

Signed-off-by: Felix Weilbach <felix.weilbach@nextcloud.com>

nextcloud-desktop-bot · 2021-06-21T10:08:53Z

AppImage file: Nextcloud-PR-3369-cb5e8398c4cb04778eaa2d0cfd84377eecc8906c-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

FlexW · 2021-06-21T10:25:48Z

/backport to stable-3.2

FlexW requested review from mgallien, camilasan, er-vin and allexzander May 27, 2021 08:46

allexzander reviewed May 27, 2021

View reviewed changes

allexzander approved these changes May 27, 2021

View reviewed changes

github-actions bot force-pushed the bugfix/dont-crash-e2ee-vfs-dialog-on-folder-delete branch from 7ccdb5c to df04668 Compare May 27, 2021 13:58

mgallien added the 1. to develop label Jun 7, 2021

FlexW force-pushed the bugfix/dont-crash-e2ee-vfs-dialog-on-folder-delete branch from df04668 to e74bc99 Compare June 8, 2021 14:00

allexzander reviewed Jun 15, 2021

View reviewed changes

src/gui/accountsettings.cpp Outdated Show resolved Hide resolved

allexzander reviewed Jun 15, 2021

View reviewed changes

src/gui/accountsettings.cpp Show resolved Hide resolved

src/gui/accountsettings.cpp Outdated Show resolved Hide resolved

src/gui/accountsettings.cpp Outdated Show resolved Hide resolved

src/gui/accountsettings.cpp Show resolved Hide resolved

mgallien reviewed Jun 15, 2021

View reviewed changes

FlexW force-pushed the bugfix/dont-crash-e2ee-vfs-dialog-on-folder-delete branch from ad2053e to d8542dd Compare June 16, 2021 14:23

mgallien approved these changes Jun 16, 2021

View reviewed changes

github-actions bot force-pushed the bugfix/dont-crash-e2ee-vfs-dialog-on-folder-delete branch from d8542dd to ee73786 Compare June 17, 2021 12:18

Felix Weilbach added 3 commits June 21, 2021 12:04

Prevent crash in dialog that warns user about vfs and e2ee

52342e5

Signed-off-by: Felix Weilbach <felix.weilbach@nextcloud.com>

Fix review comments

ab97843

Signed-off-by: Felix Weilbach <felix.weilbach@nextcloud.com>

Fix review comment

cb5e839

Signed-off-by: Felix Weilbach <felix.weilbach@nextcloud.com>

FlexW force-pushed the bugfix/dont-crash-e2ee-vfs-dialog-on-folder-delete branch from ee73786 to cb5e839 Compare June 21, 2021 10:05

FlexW merged commit 616725f into master Jun 21, 2021

FlexW deleted the bugfix/dont-crash-e2ee-vfs-dialog-on-folder-delete branch June 21, 2021 10:24

backportbot-nextcloud bot added the backport-request label Jun 21, 2021

backportbot-nextcloud bot mentioned this pull request Jun 21, 2021

[stable-3.2] Prevent crash in dialog that warns user about vfs and e2ee #3467

Closed

backportbot-nextcloud bot removed the backport-request label Jun 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent crash in dialog that warns user about vfs and e2ee #3369

Prevent crash in dialog that warns user about vfs and e2ee #3369

FlexW commented May 27, 2021

allexzander May 27, 2021

FlexW May 27, 2021

allexzander May 27, 2021

FlexW May 27, 2021

mgallien commented May 27, 2021

mgallien commented May 28, 2021

FlexW commented May 28, 2021

allexzander commented May 28, 2021 •

edited

Loading

mgallien commented May 28, 2021

mgallien commented Jun 1, 2021

mgallien Jun 15, 2021

allexzander Jun 16, 2021

mgallien left a comment

FlexW commented Jun 17, 2021

nextcloud-desktop-bot commented Jun 21, 2021

FlexW commented Jun 21, 2021

Prevent crash in dialog that warns user about vfs and e2ee #3369

Prevent crash in dialog that warns user about vfs and e2ee #3369

Conversation

FlexW commented May 27, 2021

allexzander May 27, 2021

Choose a reason for hiding this comment

FlexW May 27, 2021

Choose a reason for hiding this comment

allexzander May 27, 2021

Choose a reason for hiding this comment

FlexW May 27, 2021

Choose a reason for hiding this comment

mgallien commented May 27, 2021

mgallien commented May 28, 2021

FlexW commented May 28, 2021

allexzander commented May 28, 2021 • edited Loading

mgallien commented May 28, 2021

mgallien commented Jun 1, 2021

mgallien Jun 15, 2021

Choose a reason for hiding this comment

allexzander Jun 16, 2021

Choose a reason for hiding this comment

mgallien left a comment

Choose a reason for hiding this comment

FlexW commented Jun 17, 2021

nextcloud-desktop-bot commented Jun 21, 2021

FlexW commented Jun 21, 2021

allexzander commented May 28, 2021 •

edited

Loading