woocommerce / woocommerce-ios

WooCommerce iOS app
https://www.woocommerce.com/mobile
GNU General Public License v2.0
299 stars 110 forks source link

Invalid Number Of Rows/Sections Crash #1543

Closed astralbodies closed 3 years ago

astralbodies commented 4 years ago

Describe the bug Text from one of the crashes:

Invalid update: invalid number of rows in section 0. The number of rows contained in an existing section after the update (8) must be equal to the number of rows contained in that section before the update (7), plus or minus the number of rows inserted or deleted from that section (1 inserted, 7 deleted) and plus or minus the number of rows moved into or out of that section (0 moved in, 0 moved out).

Crash report: https://sentry.io/share/issue/63aa8ffcc1634d5da1c7c2c0f8155ac8/

To Reproduce Unsure of how to reproduce.

Expected behavior No crashes when orders list is updated.

astralbodies commented 4 years ago

FWIW the ghost feature caused a crash in WPiOS that might be similar in nature. https://github.com/wordpress-mobile/WordPress-iOS/pull/12974

pmusolino commented 4 years ago

I'm reopening this issue since it occurs also on release 3.5

shiki commented 4 years ago

Moved to Waiting for now. We'll observe how #2361 and #2362 will affect this.

sentry-io[bot] commented 4 years ago

Sentry issue: WOOCOMMERCE-IOS-81

shiki commented 4 years ago

Looks like this is still happening in 4.5 so #2447 may not actually be enough. 🙁

pmusolino commented 4 years ago

@shiki I'm thinking if it's possible that there is a case which we haven't considered, where the number of sections returned for a table view is 0. This is a value not allowed (there must always be a section, and the minimum number of sections allowed is 1)! For example, it seems that the number of sections returned by a ResultsController permit to return an empty array, and when we call the method .count on it, this will return 0. 💥

shiki commented 4 years ago

That's a good idea to look into. We might be able to easily simulate that scenario and see if it crashes by making a new test case like this one in ResultsControllerUIKitTests.

The other thing I totally forgot about is that some of our ViewControllers are still using the Ghost table dataSource switching which we've proven before that it can cause "invalid number of rows" crashes. (https://github.com/woocommerce/woocommerce-ios/issues/2448)

shiki commented 4 years ago

Suggestion by Apple was implemented in Simplenote https://github.com/Automattic/simplenote-ios/pull/798. Ref p91TBi-2Yr-p2#comment-1239

sentry-io[bot] commented 4 years ago

Sentry issue: WOOCOMMERCE-IOS-A5

sentry-io[bot] commented 4 years ago

Sentry issue: WOOCOMMERCE-IOS-RS

shiki commented 4 years ago

As of August 17, 2020:

shiki commented 3 years ago

The PR #2820 changes the Order list to use DiffableDataSource. This is an experiment to try and fix this crash. If this change is worse than we currently have, we can revert #2820. If it is successful, we should migrate all other ResultsController usages to use DiffableDataSource in order to fix the non-Order list crashes.

shiki commented 3 years ago

This was accidentally closed. I didn't mean to do that!

Reasons above: https://github.com/woocommerce/woocommerce-ios/issues/1543#issuecomment-692985153

sentry-io[bot] commented 3 years ago

Sentry issue: WOOCOMMERCE-IOS-1C4S

nbradbury commented 3 years ago

Just a note that this is still happening. Given the number of events/users, I've re-added the "high priority" label.

Screen Shot 2021-04-06 at 8 47 09 AM
koke commented 3 years ago

I think this might be Sentry misleading us, unless I'm messing up the filtering there. I looked at the last 90 days of data for this crash, and it all seems to be coming from older releases. I spotted a handful of crashes coming from 6.2/6.3 but the bulk of it is coming from older versions:

wcios-tableview-crashes

I checked just to be sure, and most of the users seem to be using the latest 2-3 releases, so this seems to be over representing those stuck in older versions.

wcios-release-launches

designsimply commented 3 years ago

Closing based on @koke's review noting whatever was causing this crash is mainly affecting users stuck in older releases and I also noticed that potentially only 3 users overall were affected in the last 90 days (according to the numbers at the top right of the Sentry report in Details view).

Events in the last 90d: 199 Users affected in the last 90d: 3 WOOCOMMERCE-IOS-1C4S: https://sentry.io/share/issue/d8c49c82786a48919bd363e1ef366935/