Fix crash when opening conflict dialog #2948

FlexW · 2021-02-23T15:32:35Z

Note: This PR is a draft. It fixes the problem but probably needs some polishing. I first wanted to hear your thoughts about this problem, my findings and my proposed solution before I investigate further.

Now to the actual problem:

When the client runs and a conflict gets detected, the sync engine runs
two times.

On the first run, the sync engine detects the conflict, marks the
file as a conflict and propagates that to the GUI. This leads to an
error notification with the original filename in the main dialog.

The sync engine runs then a second time. On this second run, the file
that originally caused the conflict is not anymore a conflict
file. Instead, the sync engine detects the conflicted copy and
propagates that file as a conflict to the GUI.

When opening the conflict dialog with the original file name (not the
conflicted copy) a crash happens. Usually, the two sync runs are really
fast, so the user does not notice the first notification. However, a
problem can occur if a conflict gets created while the client is not
running. Since then, the client does not do two sync runs. It does only
run once.

Steps to reproduce the original Issue on master:

Stop the client
Modify a synced file on the server
Modify the same file on the local machine
Start the client
Wait that the conflict gets detected
Press on the conflict notification
Observe the crash

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

FlexW · 2021-02-24T10:31:39Z

Another solution to his problem as proposed by @allexzander would be to just run a second sync after a file conflict has been detected and don't propagate the first conflict error to the GUI.

@er-vin I ping you because your input on this could be valuable :)

src/common/utility.h

er-vin · 2021-02-25T17:25:28Z

Another solution to his problem as proposed by @allexzander would be to just run a second sync after a file conflict has been detected and don't propagate the first conflict error to the GUI.

This maybe a good idea as well. Maybe less invasive? I have a hard time to gauge this from the top of my head.

Comes to mind that maybe it's the call to the dialog which should be fixed instead? I wonder if the call from socket api (so doesn't go at all through activity model) suffers from the same or a similar issue.

FlexW · 2021-03-04T14:37:42Z

I updated the pr to block the unnecessary activity and schedule a new sync after a conflicted copy gets created.
@er-vin @allexzander

src/gui/tray/UserModel.cpp

src/gui/tray/UserModel.h

src/gui/tray/UserModel.cpp

allexzander · 2021-03-05T09:47:59Z

src/gui/tray/UserModel.cpp

+    const auto isDifferentAccount = folder->accountState() != _account.data();
+    const auto isConflictFromOriginalFile = item->_status == SyncFileItem::Conflict && !Utility::isConflictFile(item->_file);
+
+    if (isDifferentAccount) {


@FlexW

We still have too many lines of code IMHO.

bool User::isValueableActivity(const Folder *folder, const SyncFileItemPtr &item) const { // activity for a different account - ignore it if (!folder->accountState() != _account.data()) { return false; } // first time conflict activity should be ignored until the next sync (fix conflict dialog crash issue) if (item->_status == SyncFileItem::Conflict && !Utility::isConflictFile(item->_file) { return false; } return true; }

@allexzander I find it more descriptive with the variable names. Then it's even readable without the comments:) With you approach it's just one line less. However, I like your comments about the conditionals. I updated it and changed the wording a bit.

FlexW · 2021-03-05T11:31:48Z

/rebase

When the client runs and a conflict gets detected, the sync engine runs two times. On the first run, the sync engine detects the conflict, marks the file as a conflict and propagates that to the GUI. This leads to an error notification with the original filename in the main dialog. The sync engine runs then a second time. On this second run, the file that originally caused the conflict is not anymore a conflict file. Instead, the sync engine detects the conflicted copy and propagates that file as a conflict to the GUI. When opening the conflict dialog with the original file name (not the conflicted copy) a crash happens. Usually, the two sync runs are really fast, so the user does not notice the first notification. However, a problem can occur if a conflict gets created while the client is not running. Since then, the client does not do two sync runs. It does only run once. Signed-off-by: Felix Weilbach <felix.weilbach@nextcloud.com>

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

nextcloud-desktop-bot · 2021-03-05T13:18:20Z

AppImage file: Nextcloud-PR-2948-63dbb90bbd96d3169b6186fb7a37edddf3f00d3e-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.

er-vin · 2021-03-05T17:36:24Z

src/gui/tray/UserModel.cpp

 void User::slotItemCompleted(const QString &folder, const SyncFileItemPtr &item)
 {
    auto folderInstance = FolderMan::instance()->folder(folder);

    if (!folderInstance)
        return;

-    // check if we are adding it to the right account and if it is useful information (protocol errors)
-    if (folderInstance->accountState() == _account.data()) {
+    if (isValueableActivity(folderInstance, item)) {


Ouch, this is subpar API naming, what is "valueable"? It will tell me nothing months from now. I think it was better not factoring this out in a method but instead store the result in two nicely named booleans and combine then in the if. Typically something like:

const auto isForUserAccount = folderInstance->accountState() == _account.data(); const auto isUnresolvableConflict = item->_status == SyncFileItem::Conflict && !Utility::isConflictFile(item->_file); if (isForUserAccount && !isUnresolvableConflict) {

@FlexW I guess we still have time to create a PR with just these changes :) @er-vin Sorry, we had to make it move forward. I must confess, I am not that nitpicky reviewer :)

@er-vin @allexzander pr for that #2976

@er-vin Sorry, we had to make it move forward.

No worries, I'm less available that's how it is. :-)

I must confess, I am not that nitpicky reviewer :)

Well, I know I put quite some nitpicks too, but there it was really more of a code smell. If something is not clearly named for some reason and one can't find a proper name for it... well something is fishy. ;-)

FlexW requested review from er-vin, allexzander and camilasan February 23, 2021 15:32

FlexW force-pushed the bugfix/conflict-dialog-issue branch from 1313512 to d226f64 Compare February 23, 2021 18:29

er-vin reviewed Feb 25, 2021

View reviewed changes

src/common/utility.h Outdated Show resolved Hide resolved

src/common/utility.h Outdated Show resolved Hide resolved

FlexW force-pushed the bugfix/conflict-dialog-issue branch from d226f64 to a5c4bae Compare March 4, 2021 14:35

allexzander reviewed Mar 5, 2021

View reviewed changes

src/gui/tray/UserModel.cpp Outdated Show resolved Hide resolved

src/gui/tray/UserModel.cpp Outdated Show resolved Hide resolved

src/gui/tray/UserModel.cpp Outdated Show resolved Hide resolved

src/gui/tray/UserModel.h Outdated Show resolved Hide resolved

allexzander suggested changes Mar 5, 2021

View reviewed changes

src/gui/tray/UserModel.cpp Outdated Show resolved Hide resolved

FlexW force-pushed the bugfix/conflict-dialog-issue branch from a5c4bae to b00eb28 Compare March 5, 2021 09:05

allexzander reviewed Mar 5, 2021

View reviewed changes

allexzander approved these changes Mar 5, 2021

View reviewed changes

Felix Weilbach added 2 commits March 5, 2021 11:32

Iteration

63dbb90

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

github-actions bot force-pushed the bugfix/conflict-dialog-issue branch from a6ca9d4 to 63dbb90 Compare March 5, 2021 11:32

FlexW merged commit f17c52d into master Mar 5, 2021

FlexW deleted the bugfix/conflict-dialog-issue branch March 5, 2021 13:32

er-vin reviewed Mar 5, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash when opening conflict dialog #2948

Fix crash when opening conflict dialog #2948

FlexW commented Feb 23, 2021 •

edited

Loading

FlexW commented Feb 24, 2021

er-vin commented Feb 25, 2021

FlexW commented Mar 4, 2021

allexzander Mar 5, 2021 •

edited

Loading

FlexW Mar 5, 2021

FlexW commented Mar 5, 2021

nextcloud-desktop-bot commented Mar 5, 2021

er-vin Mar 5, 2021

allexzander Mar 5, 2021

FlexW Mar 9, 2021

er-vin Mar 9, 2021

Fix crash when opening conflict dialog #2948

Fix crash when opening conflict dialog #2948

Conversation

FlexW commented Feb 23, 2021 • edited Loading

FlexW commented Feb 24, 2021

er-vin commented Feb 25, 2021

FlexW commented Mar 4, 2021

allexzander Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

FlexW Mar 5, 2021

Choose a reason for hiding this comment

FlexW commented Mar 5, 2021

nextcloud-desktop-bot commented Mar 5, 2021

er-vin Mar 5, 2021

Choose a reason for hiding this comment

allexzander Mar 5, 2021

Choose a reason for hiding this comment

FlexW Mar 9, 2021

Choose a reason for hiding this comment

er-vin Mar 9, 2021

Choose a reason for hiding this comment

FlexW commented Feb 23, 2021 •

edited

Loading

allexzander Mar 5, 2021 •

edited

Loading