fix(breadcrumbs): guard read access to breadcrumbs count

[tomlongridge]

Goal

Fixes #616

Design

Refactored access to breadcrumb list to avoid the need for the readWriteQueue guard in most cases.

Changeset

• BugsnagBreadcrumbs - removed capacity, count, and objectAtIndexedSubscript as no longer needed
• BugsnagBreadcrumbs:arrayValue - removed readWriteQueue guard and used a copy of the list instead; also made signature nonnull to return empty array if there are no elements.
• BugsnagConfiguration / BugsnagClient - moved the breadcrumbs field from configuration to client and added fields to configuration for maxBreadcrumbs and enabledBreadcrumbTypes. These are then used in the BugsnagBreadcrumbs constructor. This prevents any need for resizing the array of breadcrumbs outside the class.

Tests

• Enabled Thread Sanitizer and added breadcrumbs in a tight loop to check for issues.
• Amended unit tests accordingly

[tomlongridge]

I've removed internal access to the count property. Now we have a clearer distinction between internal and external headers, can we remove it?

[fractalwrench]

I think there's scope to remove quite a bit of dead code here so have commented inline.

This touches more on the overall class design, but it's worth noting that using dispatch_barrier_sync for a method that can be called from the main thread is not great. This is because it will block until IO completes.

Fortunately I think it should be possible to replace this with dispatch_barrier_async because the only remaining method after implementing these suggestions would be cachedBreadcrumbs. This is always called before breadcrumb collection happens, so could be extracted out to BugsnagClient/BSGOutOfMemoryWatchdog. This is potentially a bit out of the scope of this changeset so it'd be acceptable to address this when the breadcrumb disk serialization is revisited in general.

[tomlongridge]

This touches more on the overall class design, but it's worth noting that using dispatch_barrier_sync for a method that can be called from the main thread is not great. This is because it will block until IO completes.

Fortunately I think it should be possible to replace this with dispatch_barrier_async because the only remaining method after implementing these suggestions would be cachedBreadcrumbs. This is always called before breadcrumb collection happens, so could be extracted out to BugsnagClient/BSGOutOfMemoryWatchdog. This is potentially a bit out of the scope of this changeset so it'd be acceptable to address this when the breadcrumb disk serialization is revisited in general.

I haven't addressed this as there are wider issues trying to run this asynchronously. I'll raise this separately for a future change as this should be a good efficiency gain.

[fractalwrench]

LGTM pending resolution of two minor comments

[tomlongridge]

This PR also fixes #737

[raylillywhite]

I'm using commit 9044a26 and still getting crashes

EXC_BAD_ACCESS · Attempted to dereference garbage pointer 0xd590dd120.
/usr/lib/libobjc.A.dylib_objc_msgSend	
Bugsnag/Bugsnag/Breadcrumbs/BugsnagBreadcrumbs.m:122:37-[BugsnagBreadcrumbs arrayValue]	
Bugsnag/Bugsnag/Client/BugsnagClient.m:1096:29-[BugsnagClient serializeBreadcrumbs]	
Bugsnag/Bugsnag/Client/BugsnagClient.m:781:5-[BugsnagClient leaveBreadcrumbWithMessage:metadata:andType:]	
Bugsnag/Bugsnag/Bugsnag.m:187:9+[Bugsnag leaveBreadcrumbWithMessage:metadata:andType:]

Link here in case you have access to all bugsnag data

[tomlongridge]

Thanks @raylillywhite - I'm investigating now.

[tomlongridge]

@raylillywhite - I haven't been able to reproduce this, but it does seem that we're somehow holding on to a freed reference, even though the mutable copy should have preserved the reference.

Are you able to readily reproduce this in a non-live environment? I have pushed a change (808b8bc) that reinstates the barrier guard for arrayValue so that the operation is protected as it was before #743. I was wondering if you'd be able to see if this resolves the issue for you?

[raylillywhite]

I can't reproduce it on demand, but we saw it quite a few times (we log a lot of breadcrumbs). And we're not shipping for a few weeks so I'll use that commit in our next test build today and see if it still happens.