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.

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

IOS-699: fix crash on deletion of access method #6287

Merged

Conversation

acb-mv
Copy link
Contributor

@acb-mv acb-mv commented May 27, 2024

ListAccessMethodViewController's data source was getting out of sync with fetchedItems, which was caused by reloading items (useful when existing items have changed, catastrophic for when items have been removed from the middle of the list). Only reloading if the number of items is the same fixes the issue.


This change is Reviewable

@acb-mv acb-mv added the iOS Issues related to iOS label May 27, 2024
@acb-mv acb-mv requested a review from rablador May 27, 2024 16:16
@acb-mv acb-mv self-assigned this May 27, 2024
Copy link

linear bot commented May 27, 2024

@acb-mv acb-mv requested a review from mojganii May 28, 2024 08:25
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crash happens when the snapshot tries to reload items during removal. In updateDataSource we create a new snapshot and simultaneously populate and reload items from the repository. It seems the snapshot then crashes when it tries to compare the new items (3) to the old (4), making the index go out of range. A slight guess, but in a "pure" removal there is no comparison made and therefore no crash. Also, adding and reloading at the same time seems to be fine, since nothing is removed and range remains "safe".

The solution here - although more cumbersome - would be to avoid reloading when removing items. It was put there to make sure that non-destructive changes to existing api methods (eg. name change) would be propagated back to the list.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@acb-mv acb-mv requested a review from rablador May 28, 2024 12:13
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@buggmagnet buggmagnet force-pushed the ios-699-app-crashes-when-deleting-a-custom-api-access-method branch from 22bfece to c4d789a Compare May 29, 2024 11:49
@buggmagnet buggmagnet merged commit c58361e into main May 29, 2024
7 checks passed
@buggmagnet buggmagnet deleted the ios-699-app-crashes-when-deleting-a-custom-api-access-method branch May 29, 2024 11:50
Copy link

🚨 End to end tests failed. Please check the failed workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants