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

Prevent attempt to access index out of bounds on an array. #902

Merged
merged 1 commit into from
Jan 24, 2018

Conversation

gioneill
Copy link
Member

fixes #899

Copy link
Contributor

@winniequinn winniequinn left a comment

Choose a reason for hiding this comment

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

@gioneill I'm not sure this is the right fix here.

If the user is deleting a bookmark, indexPath.row should always be < self.bookmarks.count. If it is not, I think that means the bookmarks have changed since the table data was loaded and the deletion was requested. While your fix will guard against a crash, it seems to me that the wrong bookmarks will be deleted at times if self.bookmarks is changing without immediately updating the table view appropriately. I can't see any evidence of that happening though so I am a little confused here as to why indexPath.row would ever be out of bounds…

Were you actually seeing crashes due to out-of-bounds array accesses? If so, why do you think they are happening in the first place?

@gioneill
Copy link
Member Author

gioneill commented Jan 23, 2018

@winniequinn Like everything you've mentioned, I've spent a reasonable amount of time trying to understand how this would happen, and have not been able to find it. So I'm simply looking to prevent the crash for now.

The stack trace shows a crash due to out of bounds array access on self.bookmarks in the TOC view controller in the didCommitEdit method.

The property is set by the class that creates the object, copied from the readium view, and also updated at the end of a syncing network request. When the user toggles between the two TOC types with the segment bar, the same tableview reloads with new data (the delegate methods pull different data depending on which segment is selected).

There are three total events logged for it, so I'll leave it to your judgment whether or not we leave this as an open issue for now.

@gioneill
Copy link
Member Author

I'm going to merge.

I'm creating issue #904 to highlight that there is still a bug to solve.

@gioneill gioneill merged commit 70dda28 into master Jan 24, 2018
@gioneill gioneill deleted the toc_vc_nsarray_fix_#899 branch January 24, 2018 15:35
@gioneill gioneill added this to the 2.2.3 milestone Jan 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants