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

Avoid WPMediaPickerViewController reloadItemsAtIndexPaths crash by wrapping the call in try-catch block #414

Merged
merged 4 commits into from
Aug 15, 2023

Conversation

staskus
Copy link
Contributor

@staskus staskus commented Aug 14, 2023

Fixes #21102

This is an extension of #412 fix.

There're still some crashes happening in WPMediaPickerViewController. Although updateDataWithRemoved is alredy called from within try-catch block, the logic inside the completion block happens asynchronously and happens outside the scope of try-catch block`

To test:

I couldn't reproduce the original crash, however, adding a fake insertItem.. calls in the completion block I was able to reproduce similar crashes and confirm that try-catch block helps to avoid them temporarily.

The best way to avoid these crashes long term is to adopt UICollectionViewDiffableDataSource which requires refactoring of the current solution.


  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

Although updateDataWithRemoved is called from within try-catch block, the logic inside the completion block happens asynchronously and happens outside the scope of try-catch block
@staskus staskus requested review from kean and guarani August 14, 2023 12:48
@staskus staskus marked this pull request as ready for review August 14, 2023 12:48
Copy link

@guarani guarani left a comment

Choose a reason for hiding this comment

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

Hey @staskus, I agree with this approach since we can't reproduce this and it's an extension of the same temporary fix we've applied recently.

Looking at the three crashes, the logs indicate the data source's first section is emptied but no removals in the updates. They also all happen on the call to reloadItemsAtIndexPaths. Since the call to this method doesn't involve updates (it's the prior call to performBatchUpdates that does), I'm not sure I understand the error message.

After reading https://stackoverflow.com/questions/38597804/how-to-order-moves-inserts-deletes-and-updates-in-a-uicollectionview-performb, it could be the case that we're reloading "old" index paths which don't correspond to a data source item after the batch updates.

I tried simulating a few move scenarios using the example library but couldn't make it crash. I think this is fine to merge as-is.

@guarani
Copy link

guarani commented Aug 14, 2023

The best way to avoid these crashes long term is to adopt UICollectionViewDiffableDataSource which requires refactoring of the current solution.

That's true. What also came to find is wordpress-mobile/WordPress-iOS#21190 😄

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

Successfully merging this pull request may close these issues.

2 participants