Crash when moving email to trash #1336
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -452,10 +452,22 @@ extension InboxViewController: MsgListViewController { | |
| tableNode.reloadData() | ||
| } else { | ||
| state = .fetched(.byNumber(total: newTotalNumber)) | ||
| do { | ||
| try ObjcException.catch { | ||
| self.tableNode.deleteRows(at: [IndexPath(row: index, section: 0)], with: .left) | ||
|
Contributor
There was a problem hiding this comment. Data source for table node should be updated with correct amount of
Contributor
Author
There was a problem hiding this comment. Data source is updated above: We could reload table node with But I guess that this PR is more about handling Objc exceptions in Swift.
Collaborator
There was a problem hiding this comment. While originally it was indeed primaryli about handling of ObjC exceptions, that was in part because we didn't know any better. If the actual root cause of this exception can be fixed, that is even better and more appropriate. Both should be done. We didn't know that, so we just focused on handling, but Anton has experience with Texture specifically to notice that this is something we should be able to fix directly as well.
Collaborator
There was a problem hiding this comment. I think it's something more specific than just updating data source, as this crash happens very rarely. May be switching from index to message id (as @ivan-ushakov proposed) will fix this crash, but we can't be 100% sure as currently we're not able to reproduce this issue.
Collaborator
There was a problem hiding this comment. If the goal is to collect more information about the crash, I suppose we should have it show a modal similar to other crash scenarios? Then some user may take a screenshot and send it to us for debugging. As currently, it will just be logged then forgotten. |
||
| } | ||
| } catch { | ||
| showAlert(message: "Failed to remove message at \(index) in fetched state: \(error)") | ||
| } | ||
| } | ||
| default: | ||
| do { | ||
| try ObjcException.catch { | ||
| self.tableNode.deleteRows(at: [IndexPath(row: index, section: 0)], with: .left) | ||
| } | ||
| } catch { | ||
| showAlert(message: "Failed to remove message at \(index) in \(state): \(error)") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| // | ||
| // Use this file to import your target's public headers that you would like to expose to Swift. | ||
| // | ||
|
|
||
| #import "ObjcException.h" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| // | ||
| // ObjcException.h | ||
| // FlowCrypt | ||
| // | ||
| // Created by Ivan Ushakov on 25.01.2022 | ||
| // Copyright © 2017-present FlowCrypt a. s. All rights reserved. | ||
| // | ||
|
|
||
| #import <Foundation/Foundation.h> | ||
|
|
||
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| @interface ObjcException : NSObject | ||
|
|
||
| + (BOOL)catchException:(void(^)(void))tryBlock error:(__autoreleasing NSError **)error; | ||
|
|
||
| @end | ||
|
|
||
| NS_ASSUME_NONNULL_END |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| // | ||
| // ObjcException.m | ||
| // FlowCrypt | ||
| // | ||
| // Created by Ivan Ushakov on 25.01.2022 | ||
| // Copyright © 2017-present FlowCrypt a. s. All rights reserved. | ||
| // | ||
|
|
||
| #import "ObjcException.h" | ||
|
|
||
| @implementation ObjcException | ||
|
|
||
| + (BOOL)catchException:(void(^)(void))tryBlock error:(__autoreleasing NSError **)error { | ||
| @try | ||
| { | ||
| tryBlock(); | ||
| return YES; | ||
| } | ||
| @catch (NSException *exception) | ||
| { | ||
| NSMutableDictionary *userInfo = [NSMutableDictionary new]; | ||
| if (exception.userInfo != NULL) | ||
| { | ||
| userInfo = [[NSMutableDictionary alloc] initWithDictionary:exception.userInfo]; | ||
| } | ||
|
|
||
| if (exception.reason != nil) | ||
| { | ||
| if (![userInfo.allKeys containsObject:NSLocalizedFailureReasonErrorKey]) | ||
| { | ||
| [userInfo setObject:exception.reason forKey:NSLocalizedFailureReasonErrorKey]; | ||
| } | ||
| } | ||
|
|
||
| *error = [[NSError alloc] initWithDomain:exception.name code:0 userInfo:userInfo]; | ||
| return NO; | ||
| } | ||
| } | ||
|
|
||
| @end |
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.
Can you please explain how is this fix related to the crash fix?
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.
It looks like there is an issue related to the index of the row that we try to update
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.
As @tomholub said, we want to avoid having crash here. Even if issue is still here, we should not crash.
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.
Can we have an issue to solve the root cause of the crash?
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.
absolutely. Let's solve it in this PR as well.
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.
Since all UI is async with Texture, I guess that using message index is unsafe. We should move to message ID, as this is the only way to know that message we want to delete is the same message.
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.
Would suggest to investigate root cause of the issue.
Also if there was only one message before moving to trash we need to update whole state of the controller.