Wordpress-ios: WPTableViewHandler controllerDidChangeContent Crash

[jkmassel]

From Fabric:

Crashed: com.apple.main-thread
EXC_BAD_ACCESS KERN_INVALID_ADDRESS 0x0000000000000000

Crashed: com.apple.main-thread
0  UIKitCore                      0x23befcd44 __46-[UITableView _updateWithItems:updateSupport:]_block_invoke + 80
1  UIKitCore                      0x23befce5c __46-[UITableView _updateWithItems:updateSupport:]_block_invoke.1100 + 228
2  UIKitCore                      0x23befc5b4 -[UITableView _updateWithItems:updateSupport:] + 1976
3  UIKitCore                      0x23bef5248 -[UITableView _endCellAnimationsWithContext:] + 14944
4  UIKitCore                      0x23bf0b4e8 -[UITableView endUpdates] + 88
5  WordPress                      0x10084a54c -[WPTableViewHandler controllerDidChangeContent:] (WPTableViewHandler.m:618)
6  CoreData                       0x212366f74 __82-[NSFetchedResultsController(PrivateMethods) _core_managedObjectContextDidChange:]_block_invoke + 5016
7  CoreData                       0x212329b28 developerSubmittedBlockToNSManagedObjectContextPerform + 156
8  CoreData                       0x2121f42dc -[NSManagedObjectContext performBlockAndWait:] + 252
9  CoreData                       0x212200308 -[NSFetchedResultsController(PrivateMethods) _core_managedObjectContextDidChange:] + 116
10 CoreFoundation                 0x20f7095bc __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 20
11 CoreFoundation                 0x20f709588 ___CFXRegistrationPost_block_invoke + 64
12 CoreFoundation                 0x20f708a7c _CFXRegistrationPost + 392
13 CoreFoundation                 0x20f708728 ___CFXNotificationPost_block_invoke + 96
14 CoreFoundation                 0x20f682524 -[_CFXNotificationRegistrar find:object:observer:enumerator:] + 1496
15 CoreFoundation                 0x20f7081d8 _CFXNotificationPost + 696
16 Foundation                     0x2100f0814 -[NSNotificationCenter postNotificationName:object:userInfo:] + 68
17 CoreData                       0x212203e68 -[NSManagedObjectContext(_NSInternalNotificationHandling) _postObjectsDidChangeNotificationWithUserInfo:] + 724
18 CoreData                       0x2122000f4 -[NSManagedObjectContext(_NSInternalChangeProcessing) _createAndPostChangeNotification:deletions:updates:refreshes:deferrals:wasMerge:] + 1268
19 CoreData                       0x2121ff238 -[NSManagedObjectContext(_NSInternalChangeProcessing) _processRecentChanges:] + 1240
20 CoreData                       0x2122027dc -[NSManagedObjectContext(_NestedContextSupport) _parentProcessSaveRequest:inContext:error:] + 1704
21 CoreData                       0x21232feec __82-[NSManagedObjectContext(_NestedContextSupport) executeRequest:withContext:error:]_block_invoke + 576
22 CoreData                       0x2123319a0 internalBlockToNSManagedObjectContextPerform + 96
23 libdispatch.dylib              0x20f1d97d4 _dispatch_client_callout + 16
24 libdispatch.dylib              0x20f187ce8 _dispatch_async_and_wait_invoke + 92
25 libdispatch.dylib              0x20f1d97d4 _dispatch_client_callout + 16
26 libdispatch.dylib              0x20f187004 _dispatch_main_queue_callback_4CF$VARIANT$mp + 1068
27 CoreFoundation                 0x20f729ec0 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 12
28 CoreFoundation                 0x20f724df8 __CFRunLoopRun + 1924
29 CoreFoundation                 0x20f724354 CFRunLoopRunSpecific + 436
30 GraphicsServices               0x21192479c GSEventRunModal + 104
31 UIKitCore                      0x23bd0fb68 UIApplicationMain + 212
32 WordPress                      0x1007dfa80 main (main.m:11)
33 libdyld.dylib                  0x20f1ea8e0 start + 4

[designsimply]

First seen in: 7.5.0.2
Last seen in: 12.0.0.3 (27 times in the last 90 days)
Users Affected: 526 in the last 90 days
Frequency: ~10 per day in the last 90 days
_{(58ff5913be077a4dcce58a69-fabric-ios)}

[designsimply]

controllerDidChangeContent is an issue with NSFetchedResultsController, which has objectively terrible integration with UITableView. IMHO we shouldn’t rely on it because we get all sorts of these kinds of bugs.

h/t @jkmassel

[jkmassel]

lol so of course in this case where I gripe about it, it's not related to NSFetchedResultsController + UITableView section and row indexes.

That said:

https://github.com/wordpress-mobile/WordPress-iOS/blob/d3eb6acdaffc6df49d241b27249f04127ffc8800/WordPress/Classes/Utility/WPTableViewHandler.m#L640-L649

[designsimply]

30-day impact: ~11 per day
Users affected: 153
First seen in: 7.5.0.2
Last seen in: 12.1.3

^{(58ff5913be077a4dcce58a69-fabric-ios)}

[designsimply]

Numbers from Fabric:

30-day impact: ~10 per day
Users affected: 120
Last seen in: 12.2.0.3

^{(58ff5913be077a4dcce58a69-fabric-ios)}

I _think_ https://sentry.io/share/issue/969b0bc88e2c4ec9a42bc908b672bda3/ might be the same report in Sentry. @jkmassel can you help me confirm?

[yaelirub]

@designsimply , I think https://sentry.io/share/issue/969b0bc88e2c4ec9a42bc908b672bda3/ is a different issue -> https://github.com/wordpress-mobile/WordPress-iOS/issues/11399.

[yaelirub]

The closest issue I found in Sentry to this issue is: https://sentry.io/share/issue/331ad2ef7f894793b3f5b830b113d503/

Researching this I found this issue: https://github.com/martydill/ios-queryable/issues/7
which states:

The method in discussion executes a fetch request (via its toArray) each time the runtime asks for a batch of objects to enumerate through. And if the managed object contents change between fetch requests, it generates a crash as the toArray contents are not the same between invocations.

I'm guessing something similar happens here in refreshTableViewPreservingOffset where the managedObjectContext changes between enumerations.

Not sure if this is the same issue or a different one.
Any thoughts, @jkmassel ?

[jkmassel]

It's super interesting to me that the Sentry error is Attempted to dereference garbage pointer.

Because WPTableViewHandler.m is Obj-C, it makes me wonder if we have a use-after-free bug?

You could try running the app with NSZombies enabled, and see if any show up?

To avoid the out-of-sync issue, we could ignore changes coming in from the managedObjectContext while we're refreshing the table view. Then once we're done, merge any new changes?

Ideal scenario IMHO is just grab all of the objects in the fetchedResultsController, manually diff them ourselves, then update the table view manually, and only use controllerDidChangeContent as a trigger to do all of that, but that's a bigger architectural question that's probably outside the scope of this issue.

[yaelirub]

Another thing that I noticed is that the Sentry is that all the random affected users I looked at are on version 12.3 and using iPad pro 2nd generation.

[jkmassel]

I'm seeing a few events from iPad mini and iPhone 6 in there as well, so _probably_ not a device-specific issue? I'm looking at this ticket: https://sentry.io/share/issue/331ad2ef7f894793b3f5b830b113d503/

[yaelirub]

@jkmassel , not sure. It seems to happen more on iPad. Though the issue your referenced seems to have the same crashing reason while it's displaying the NoResultsViewController which is a bit odd imo.
Unfortunately, I didn't have time to investigate this issue more during GK round.

[designsimply]

90-day impact: ~133 per day
Users affected in the last 90 days: 3.3k

^{https://sentry.io/share/issue/331ad2ef7f894793b3f5b830b113d503/}

[designsimply]

Moving to high priority.

^{(Please see internal reference: p77Llu-cdK-p2).}

[designsimply]

90-day impact: ~211 per day
Users affected in the last 90 days: 5.5k

^{https://sentry.io/share/issue/331ad2ef7f894793b3f5b830b113d503/}

[aerych]

The reports in Sentry seem to cover a variety of different issues. I took a look specifically at the reports related to WPTableViewHandler controllerDidChangeContent as outlined in the description.

I found a handful off reports that included WPTableViewHandler in the stack trace: 1, 2, 3, 4. Each one has another thread with a call to [ContextManager saveDerivedContext:withCompletionBlock]. Based on this it seemed reasonable to suspect a feature in the app using a WPTableViewHandler and a derived context.

I found three candidates: ReaderStreamViewController, ReaderCommentsViewController, and CommentsViewController. All three controllers use WPTableViewHandler to wrangle their table view, and pass a derived core data context to their respective backing services.

Theory: Something about the usage of the derived context in one or more of these controllers is not thread safe, modifying data in an unexpected way, and yielding the crash.

ReaderStreamViewController is backed by ReaderPostService, and the call to sync posts is done on a background thread. When the call to sync is completed the service calls its success and failure blocks on the thread its running in. This is fine as the controller ensures work is moved out to the main thread. When the service saves changes, it calls ContextManager.saveContext or ContextManager.saveContext:withCompletionBlock, both of which will properly handle a derived context.

Similarly, CommentViewController calls its service inside the derived context's perform block, but ensures work in callback blocks are moved to the main thread. When the CommentService saves changes it calls ContextManager.saveContext--again, a method that properly handles a derived context.

ReaderCommentViewController is a bit different. In its case, it creates the service with a derived context but calls its methods from the main thread. In the service, the call to sync runs on the main thread but handles results processing and errors from within its context's perform block. Note that it doesn't know if its context is main or derived. When its work is finished, it executes its call back blocks on the main thread--except for one failure which is not wrapped in a dispatch_async call. Differing from the first two, saves are handled by ContextManager.saveContextAndWait--a method that _does not_ properly handle saving a derived context. This method calls the internalSaveContext method, saving the derived context as if it were the main context, and does not subsequently update the main context.

Amended theory: Syncing reader comments--hierarchical comments work most of the time. However, there is some scenario where comments are synced, the derived context is saved, but the main is not updated to reflect changes. A subsequent save to the main context triggers WPTableViewHandler.contentDidChange and items are added or removed unexpectedly yielding a 💥.

Since we don't know the exact scenario that will reproduce the crash, let's see if improving the CommentService's saving behavior might have a positive impact. https://github.com/wordpress-mobile/WordPress-iOS/pull/12440

[koke]

I found three candidates: ReaderStreamViewController, ReaderCommentsViewController, and CommentsViewController. All three controllers use WPTableViewHandler to wrangle their table view, and pass a derived core data context to their respective backing services.

I tested these while enabling Core Data's concurrency debug mode (-com.apple.CoreData.ConcurrencyDebug 1) and we were indeed violating multithreading rules on those three. I'll prepare a PR for this.

[aerych]

Tentatively closing as we think @koke's work in #12539 will sort this one. We can reopen if it reappears.