Desktop: Fixes #11457: Fix crash on startup if old read-only items are in the trash

[personalizedrefrigerator]

Summary

This pull request should fix #11457.

Previously, the trash auto-deletion logic attempted to delete items in the trash even if part of a read-only share. This caused a crash on startup when deleting old items.

Important

Although this issue doesn't seem to be a recent regression, this pull request targets release-3.1 — this issue is preventing at least one user from starting Joplin.

Testing plan

This pull request includes an automated test, but has not been tested manually.

I have verified that, if the changes to permanentlyDeleteItems.ts are reverted, then the test fails with:

    Cannot change or delete a read-only item: a85b609b6eef4914833c381e0d393a89

[laurent22]

Thanks for looking into this but I wonder if this is the right fix. First, a readonly item probably shouldn't have been deleted? Or if it can under certain condition, shouldn't it be made non-readonly and removed from the share?

[personalizedrefrigerator]

First, a readonly item probably shouldn't have been deleted? Or if it can under certain condition, shouldn't it be made non-readonly and removed from the share?

Good point — this error seems to indicate an issue exists somewhere else.

However, because at least one user's copy of Joplin crashes on startup due to this invalid state, it may make sense to either:

1. Add a task to locally remove deleted readonly items before the trash auto-deletion logic runs.
   • If the sync target also has deleted readonly items, additional logic would be needed to prevent these items from being downloaded.
2. Update the trash folder logic to not crash if there are deleted readonly items.
   • This option is currently what is done by this pull request.
   • An alternative might be to wrap the trash auto-deletion logic in a try/catch. This would allow Joplin to start even if the trash auto-deletion task fails. However, I expect that this will be more difficult to test.

Note

Note that a response to the above comment was posted on the forum:

laurent wrote

First, a readonly item probably shouldn't have been deleted?

I own several shares that I made available to other JoplinCloud basic accounts when the ‘read only’ option was not yet available. Later, when I discovered that the ‘read only’ option was offered, I changed these shared to ‘read only’.

[laurent22]

Based on the above comment, I feel this is what should be done?

• When the app starts, check the items on the bin and unshare any item in there (it seems the problem is that when we delete something, we don't unshare it, but we should because it's been effectively removed from the share).

I believe just this change should fix this particular crash? And this change should be removed after a few months because it's only meant to fix invalid data

Then we should also review what happens when a shared item is deleted, but that would be a different PR

[personalizedrefrigerator]

• When the app starts, check the items on the bin and unshare any item in there (it seems the problem is that when we delete something, we don't unshare it, but we should because it's been effectively removed from the share).

Thank you for the feedback!

I don't think unsharing would fix the issue — the share recipients would still have invalid data (and may not have permission to unshare the items). To fix the crash, one of the following would likely be necessary:

• Logic that:
  1. Locally deletes shared items that are in the trash on startup.
  2. Prevents shared trashed items from being downloaded during sync.
• Logic that disables trash autodeletion on startup if there are deleted shared items.
• A fix similar to this pull request, that prevents permanentlyDeleteOldItems from crashing when handling invalid data.

Of the above, I think that allowing permanentlyDeleteOldItems to handle invalid state without crashing the app is the simplest to implement and test. This is the solution currently implemented by this pull request.

Explanation: When the crash happens, 1) the current user lacks permission to permanently delete items and 2) the crash occurs on startup within permanentlyDeleteOldItems. As a result,

• The current user won't have permission to unshare the deleted items (since the share is read-only).
• There isn't necessarily enough time for a full sync to occur.
  • One possibility might be to move permanentlyDeleteOldItems after the first full sync. However, this has a few potential problems:
    • The user that created the share needs to update Joplin and sync to fix the crash that's happening for the user that received the share.
    • The user that received the share needs to have a network connection.

Edited: Reworded and reorganized. (If it still isn't clear, please let me know and I can try to clarify!)

[laurent22]

Ok I think that makes sense to fix this particular crash then. Also to address his comment:

I own several shares that I made available to other JoplinCloud basic accounts when the ‘read only’ option was not yet available. Later, when I discovered that the ‘read only’ option was offered, I changed these shares to ‘read only’.

Could you please check what happens when a note in a shared folder is moved to the trash? Do we clear the share_id and is_shared property?

As for read-only items, I believe there are checks in place to prevent them from being deleted in general. I think it was only possible in this case maybe due to a bug with share_id not being cleared.

[laurent22]

@personalizedrefrigerator, I've created this issue: #11482