Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deleting one message causes another arbitrary message to be deleted #148

Open
7 tasks done
tom93 opened this issue Mar 23, 2024 · 4 comments · May be fixed by #149
Open
7 tasks done

Deleting one message causes another arbitrary message to be deleted #148

tom93 opened this issue Mar 23, 2024 · 4 comments · May be fixed by #149
Labels
bug Something is not working

Comments

@tom93
Copy link
Contributor

tom93 commented Mar 23, 2024 •
edited
Loading

Proposed fix: #149.

Checklist

  • I can reproduce the bug with the latest version given here.
  • I made sure that there are no existing issues - open or closed - to which I could contribute my information.
  • I made sure that there are no existing discussions - open or closed - to which I could contribute my information.
  • I have read the FAQs inside the app (Menu -> About -> FAQs) and my problem isn't listed.
  • I have taken the time to fill in all the required details. I understand that the bug report will be dismissed otherwise.
  • This issue contains only one bug.
  • I have read and understood the contribution guidelines.

Affected app version

1.0.1

Affected Android/Custom ROM version

Android 14

Affected device model

Emulator

How did you install the app?

GitHub releases

Steps to reproduce the bug

(See screen recording for demo)

  1. Use a clean phone with no messages to avoid deleting important messages and to be able to reproduce this reliably.
  2. Create a conversation with 555-0101 and send two SMS's: "Message 1" and "Message 2".
  3. Create a conversation with 555-0102 and send an SMS: "Message 3".
  4. Delete "Message 3".
  5. Switch to the conversation with 555-0101.

Expected behavior

There should still be two SMS's there ("Message 1" and "Message 2").

Actual behavior

"Message 2" is deleted (!)

Screenshots/Screen recordings

screenrecord.mp4

(the end got clipped a little, you may need to pause to see that Message 2 disappears)

Additional information

Workaround: DO NOT DELETE MESSAGES

Cause: Commit 44c540b introduced a hack to update the conversation snippet, however there is a bug in that code: it tries to delete by thread ID, but the condition is applied to the SMS table, not to the threads table (Android source).
So instead of deleting the thread with that ID, it deletes whichever SMS happens to have that ID.

Proposed solution: I haven't tested this yet, but I believe changing the condition to "1=0" will fix the issue (it will still cause the threads to be updated but won't delete anything). Update: I've tested it using the steps above and it seems to work fine, but additional testing would be welcome.

CC: @Aga-C
@tom93 tom93 added bug Something is not working needs triage Issue is not yet ready for PR authors to take up labels Mar 23, 2024
tom93 added a commit to tom93/FossifyOrg-Messages that referenced this issue Mar 23, 2024
@tom93
Fix deletion of random SMS when trying to update threads
8096869 
Fixes FossifyOrg#148.
tom93 added a commit to tom93/FossifyOrg-Messages that referenced this issue Mar 23, 2024
@tom93
Fix deletion of random SMS when trying to update threads
98fdeac 
Fixes FossifyOrg#148.
@tom93 tom93 linked a pull request Mar 23, 2024 that will close this issue
Open
4 tasks
@Aga-C Aga-C removed the needs triage Issue is not yet ready for PR authors to take up label Mar 23, 2024
tom93 added a commit to tom93/FossifyOrg-Messages that referenced this issue Mar 23, 2024
@tom93
Fix deletion of random SMS when trying to update threads
fe71e4f 
Commit 44c540b (Fixed updating last message after deleting (FossifyOrg#167),
2021-09-04) introduced a hack to update the conversation snippet,
however there is a bug in that code: it tries to delete by thread ID,
but the condition is applied to the SMS table, not to the threads
table.[1] So instead of deleting the thread with that ID, it deletes
whichever SMS happens to have that ID.

The fix is to change the condition to an always-false condition, so
that no messages will be deleted. The threads will still get
updated.[2]

Fixes FossifyOrg#148.

[1] https://android.googlesource.com/platform/packages/providers/TelephonyProvider/+/android14-release/src/com/android/providers/telephony/MmsSmsProvider.java#1405
[2] https://android.googlesource.com/platform/packages/providers/TelephonyProvider/+/android14-release/src/com/android/providers/telephony/MmsSmsProvider.java#1409
tom93 added a commit to tom93/FossifyOrg-Messages that referenced this issue Mar 23, 2024
@tom93
Fix deletion of random SMS when trying to update threads
68e396e 
Commit 44c540b (Fixed updating last message after deleting (FossifyOrg#167),
2021-09-04) introduced a hack to update the conversation snippet,
however there is a bug in that code: it tries to delete by thread ID,
but the condition is applied to the SMS table, not to the threads
table.[1] So instead of deleting the thread with that ID, it deletes
whichever SMS happens to have that ID.

The fix is to change the condition to an always-false condition, so
that no messages will be deleted. The threads will still get
updated.[2]

Fixes FossifyOrg#148.

[1] https://android.googlesource.com/platform/packages/providers/TelephonyProvider/+/android14-release/src/com/android/providers/telephony/MmsSmsProvider.java#1405
[2] https://android.googlesource.com/platform/packages/providers/TelephonyProvider/+/android14-release/src/com/android/providers/telephony/MmsSmsProvider.java#1409
tom93 added a commit to tom93/FossifyOrg-Messages that referenced this issue Mar 23, 2024
@tom93
Fix deletion of random SMS when trying to update threads
91b6c73 
Commit 44c540b (Fixed updating last message after deleting (FossifyOrg#167),
2021-09-04) introduced a hack to update the conversation snippet,
however there is a bug in that code: it tries to delete by thread ID,
but the condition is applied to the SMS table, not to the threads
table.[1] So instead of deleting the thread with that ID, it deletes
whichever SMS happens to have that ID.

The fix is to change the condition to an always-false condition, so
that no messages will be deleted. The threads will still get
updated.[2]

Fixes FossifyOrg#148.

[1] https://android.googlesource.com/platform/packages/providers/TelephonyProvider/+/android14-release/src/com/android/providers/telephony/MmsSmsProvider.java#1405
[2] https://android.googlesource.com/platform/packages/providers/TelephonyProvider/+/android14-release/src/com/android/providers/telephony/MmsSmsProvider.java#1409
@tom93 tom93 mentioned this issue Mar 24, 2024
Open
7 tasks
@tom93 tom93 mentioned this issue Apr 4, 2024
Closed
6 tasks
@tom93
Copy link
Contributor Author

tom93 commented Apr 4, 2024

It might be possible to recover unintentionally-deleted messages using the Room DAO cache, I'm not sure how easy/reliable that would be.

Either way, I think my proposed fix (#149) should be merged and released ASAP (I don't think it will interfere with potential future recovery efforts).

@naveensingh
Copy link
Member

@Aga-C have you tried reproducing this bug?

@Aga-C
Copy link
Member

Aga-C commented Apr 4, 2024

@naveensingh I was able to reproduce it after the fresh install of the app with no SMS messages on the phone yet. With more messages, maybe either I didn't notice or the IDs were already so high that nothing got removed.

I've also checked, no one reported it in Simple SMS Messenger, but the code has been there since 2021.

@tom93
Copy link
Contributor Author

tom93 commented Apr 4, 2024 •
edited
Loading

With more messages, maybe either I didn't notice or the IDs were already so high that nothing got removed.

For reference, when there are many messages, the behaviour I'm predicting is that if you delete a message from a conversation whose ID is i (meaning, the i-th oldest conversation by date of first message), then it will also delete the message with ID i (meaning the i-th oldest message across all conversations). So it will be the oldest messages that get unintentionally deleted, and the unintentional deletion will only occur for the first deletion in each conversation.

To maximise the chances of discovery, the process would be:

  1. Make a note of the oldest messages (by going over the conversations and keeping track of the oldest messages you find).
  2. Delete one message from each conversation (e.g. the most recent message).
  3. Check if the oldest messages still exist. If you have n conversations and you've never deleted any messages before this test, then I predict that the oldest n messages will be unintentionally deleted. If you deleted messages in the past, then there will be fewer unintentional deletions from step 2, because some of those unintentional deletions would have already occurred in the past! Also, note that ID's might not correspond to age for imported messages.

Obviously back up the messages first. Personally I wouldn't attempt this on a device with important messages, even with a backup.

(Edited to clarify)

tom93 added a commit to tom93/FossifyOrg-Messages that referenced this issue May 20, 2024
@tom93
Fix deletion of random SMS when trying to update threads
99cea60 
Commit 44c540b (Fixed updating last message after deleting (FossifyOrg#167),
2021-09-04) introduced a hack to update the conversation snippet,
however there is a bug in that code: it tries to delete by thread ID,
but the condition is applied to the SMS table, not to the threads
table.[1] So instead of deleting the thread with that ID, it deletes
whichever SMS happens to have that ID.

The fix is to change the condition to an always-false condition, so
that no messages will be deleted. The threads will still get
updated.[2]

Fixes FossifyOrg#148.

[1] https://android.googlesource.com/platform/packages/providers/TelephonyProvider/+/android14-release/src/com/android/providers/telephony/MmsSmsProvider.java#1405
[2] https://android.googlesource.com/platform/packages/providers/TelephonyProvider/+/android14-release/src/com/android/providers/telephony/MmsSmsProvider.java#1409
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something is not working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix deletion of random SMS when trying to update threads tom93/FossifyOrg-Messages
3 participants
@tom93 @naveensingh @Aga-C