-
Notifications
You must be signed in to change notification settings - Fork 15
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
fix: crash on conversation content - WPB-15979 #2554
base: release/cycle-3.118
Are you sure you want to change the base?
Conversation
Test Results 1 files 2 suites 2m 2s ⏱️ Results for commit 582ee7f. ♻️ This comment has been updated with latest results. |
Datadog ReportBranch report: ✅ 0 Failed, 1687 Passed, 26 Skipped, 2m 1.84s Total Time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: If the conversations are updated while in the background can things become out of sync or is the data reloaded when we enter the foreground again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good but I want to see the answer to @samwyndham 's question.
...os/Wire-iOS/Sources/UserInterface/Conversation/Content/ConversationTableViewDataSource.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix looks good for the background. I think I saw (but I'm not certain) in the attached crash files that sometimes it also occurred in the foreground, there might be a different fix to do if that's the case.
@samwyndham I changed the implementation to refetch when resuming to foreground, so no updates are missed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I just added a non-blocking comment.
do { | ||
try fetchController?.performFetch() | ||
tableView.reloadData() | ||
tableView.setContentOffset(contentOffset, animated: false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: I'm surprised that this is needed - I assume you saw an issue with the content offset changing when calling reload data?
Issue
In the crash report associated to the ticket, we can see that the app is killed by the system while fetching messages of the conversation in CoreData while in background.
It's not directly clear if the exhaustion of resources we get is caused by just fetching a lot of messages or processing in another thread messages.
Solution
In case of the first one, we could remove the observation of changes in the current conversation when moving to background.
Testing