diff --git a/mParticle-Apple-SDK/Kits/MPKitContainer.m b/mParticle-Apple-SDK/Kits/MPKitContainer.m index cb74963ca..3a35cbdf5 100644 --- a/mParticle-Apple-SDK/Kits/MPKitContainer.m +++ b/mParticle-Apple-SDK/Kits/MPKitContainer.m @@ -40,6 +40,10 @@ NSString *const kitFileExtension = @"eks"; static NSMutableSet > *kitsRegistry; +// kitsRegistry is class-level state, so its lock has to be class-level too. This was a +// per-instance ivar, which meant two containers could mutate the same set while each held +// its own semaphore - the lock could not do the job it was there for. +static dispatch_semaphore_t kitsSemaphore; // Tracks kit teardown work deferred to the main queue by flushSerializedKits and // freeKitRegister:integrationId: (stop(), disk cleanup, notification). Workspace-switch // callers enter this group before dispatching that work and leave it after, so @@ -79,12 +83,12 @@ - (nullable instancetype)initWithInstance:(nonnull NSObject *)ins static const NSInteger sideloadedKitCodeStartValue = 1000000000; @interface MPKitContainer_PRIVATE () { - dispatch_semaphore_t kitsSemaphore; - // brackets is per-instance state (unlike kitsRegistry, which is static/shared), so its - // lock is scoped to the instance too. It is a separate semaphore from kitsSemaphore, - // not reused, because updateBracketsWithConfiguration:integrationId: is called from - // inside configureKits:'s kitsSemaphore-locked region - dispatch_semaphore is not - // reentrant, so guarding brackets with kitsSemaphore itself would deadlock there. + // brackets is per-instance state (unlike kitsRegistry/kitsSemaphore, which are + // static/shared - see +initialize), so its lock is scoped to the instance too. It is a + // separate semaphore from kitsSemaphore, not reused, because + // updateBracketsWithConfiguration:integrationId: is called from inside configureKits:'s + // kitsSemaphore-locked region - dispatch_semaphore is not reentrant, so guarding + // brackets with kitsSemaphore itself would deadlock there. dispatch_semaphore_t bracketsSemaphore; NSMutableDictionary *brackets; NSInteger sideloadedKitCodeNextValue; @@ -105,6 +109,7 @@ @implementation MPKitContainer_PRIVATE + (void)initialize { if (self == [MPKitContainer_PRIVATE class]) { kitsRegistry = [[NSMutableSet alloc] initWithCapacity:DEFAULT_ALLOCATION_FOR_KITS]; + kitsSemaphore = dispatch_semaphore_create(1); kitTeardownGroup = dispatch_group_create(); } } @@ -124,7 +129,6 @@ - (instancetype)init { _attributionInfo = [NSMutableDictionary dictionary]; NSMutableDictionary *linkInfo = _attributionInfo; _initializedTime = [NSDate date]; - kitsSemaphore = dispatch_semaphore_create(1); bracketsSemaphore = dispatch_semaphore_create(1); brackets = [[NSMutableDictionary alloc] init]; sideloadedKitCodeNextValue = sideloadedKitCodeStartValue; @@ -249,14 +253,20 @@ - (void)flushSerializedKits { // under the lock rather than enumerating the live one, which raced with the locked // mutation in configureKits: and could crash mid-enumeration. // - // The snapshot is taken here and iterated on the main queue without holding the lock: - // waiting on kitsSemaphore from the main queue would let a long background critical - // section stall the main thread. Iterating the snapshot lets this path call - // freeKitRegister: directly, so it never reads kitsRegistry unguarded - freeKit: - // itself has to stay lock-free because configureKits: calls it while holding the - // semaphore, and dispatch_semaphore is not recursive. + // wrapperInstance is declared nonatomic, so detaching it here is what actually + // synchronizes with isActiveAndNotDisabled:, which reads it under this same lock on + // other threads - a plain unguarded nil-out on the main queue raced with those reads + // and crashed inside objc_retain/objc_release on the freed wrapper. The rest of the + // teardown (stop, file cleanup, notification) does not touch container state, so it + // runs unlocked on the main queue below - holding kitsSemaphore across that would let + // a slow stop() or a slow disk stall every other thread waiting on the lock. dispatch_semaphore_wait(kitsSemaphore, DISPATCH_TIME_FOREVER); NSArray> *kitsSnapshot = [kitsRegistry allObjects]; + NSMutableArray *detachedWrapperInstances = [[NSMutableArray alloc] initWithCapacity:kitsSnapshot.count]; + for (id kitRegister in kitsSnapshot) { + [detachedWrapperInstances addObject:(id)kitRegister.wrapperInstance ?: [NSNull null]]; + kitRegister.wrapperInstance = nil; + } dispatch_semaphore_signal(kitsSemaphore); // Entered synchronously, here, before returning - not inside the dispatched block below. @@ -267,13 +277,12 @@ - (void)flushSerializedKits { // workspace's kits start before this teardown is even scheduled, let alone done. dispatch_group_enter(kitTeardownGroup); dispatch_async(dispatch_get_main_queue(), ^{ - for (idkitRegister in kitsSnapshot) { - id wrapperInstance = kitRegister.wrapperInstance; - if (wrapperInstance) { - kitRegister.wrapperInstance = nil; + [kitsSnapshot enumerateObjectsUsingBlock:^(id kitRegister, NSUInteger idx, BOOL *stop) { + id wrapperInstance = detachedWrapperInstances[idx]; + if (wrapperInstance != (id)[NSNull null]) { [self teardownDetachedWrapperInstance:wrapperInstance forIntegrationId:kitRegister.code]; } - } + }]; dispatch_group_leave(kitTeardownGroup); }); } @@ -290,16 +299,15 @@ - (void)freeKit:(NSNumber *)integrationId { [self freeKitRegister:kitRegister integrationId:integrationId]; } -// configureKits: calls freeKit:/freeKitRegister: while holding kitsSemaphore (see the -// deactivateKits cleanup there), and dispatch_semaphore is not recursive, so this method -// cannot take the lock itself. The wrapperInstance = nil detach below has to run inline -// regardless, since that is what synchronizes with isActiveAndNotDisabled:'s reads of -// wrapperInstance on other threads under the same lock. Everything after the detach - -// stop(), disk cleanup, posting mParticleKitDidBecomeInactiveNotification - runs arbitrary -// kit and observer code, so it is deferred to the main queue instead of running inline: -// doing that work while still holding kitsSemaphore (as configureKits: does) would stall -// every other thread waiting on the lock for as long as it takes, or deadlock outright if -// an observer calls back into a kitsSemaphore-guarded method. +// Callers of freeKit:/freeKitRegister: (via configureKits:) already hold kitsSemaphore for +// the whole call, so detaching wrapperInstance here needs no separate lock - it is already +// serialized against every other access. The detach must stay inline (it is what +// synchronizes with isActiveAndNotDisabled: on other threads), but the teardown below is +// deferred past the lock for the same reason flushSerializedKits defers it: stop(), disk +// I/O and posting mParticleKitDidBecomeInactiveNotification all run arbitrary/observer code +// on the calling thread, and doing that while still holding kitsSemaphore would stall every +// other thread waiting on the lock for as long as that takes, or deadlock outright if an +// observer calls back into any kitsSemaphore-guarded method. - (void)freeKitRegister:(id)kitRegister integrationId:(NSNumber *)integrationId { NSAssert(integrationId != nil, @"Required parameter. It cannot be nil."); @@ -317,12 +325,12 @@ - (void)freeKitRegister:(id)kitRegister integrationId:(N } } -// wrapperInstance must already be detached from its kitRegister (kitRegister.wrapperInstance -// set to nil) before calling this, by a caller synchronized with kitsSemaphore. This method -// itself touches no container state, so it does not take kitsSemaphore, and every caller -// runs it off of that lock regardless (see flushSerializedKits and freeKitRegister: above), -// since stop(), disk cleanup and the notification post here run arbitrary kit/observer code -// that must not run while the lock is held. +// Stops wrapperInstance and cleans up its on-disk state and notification. wrapperInstance +// must already be detached from its kitRegister (kitRegister.wrapperInstance set to nil) +// before calling this. This method itself touches no container state, so it does not take +// kitsSemaphore - but every caller defers it off of that lock regardless (see +// flushSerializedKits and freeKitRegister:integrationId:), since the work here runs +// arbitrary kit and observer code that must not run while the lock is held. - (void)teardownDetachedWrapperInstance:(id)wrapperInstance forIntegrationId:(NSNumber *)integrationId { if ([wrapperInstance respondsToSelector:@selector(stop)]) { [wrapperInstance stop]; @@ -1243,7 +1251,7 @@ - (void)project:(id)kitRegister __strong MPKitContainer_PRIVATE *strongSelf = weakSelf; if (strongSelf) { - dispatch_semaphore_wait(strongSelf->kitsSemaphore, DISPATCH_TIME_FOREVER); + dispatch_semaphore_wait(kitsSemaphore, DISPATCH_TIME_FOREVER); } // Filter projections only to those of 'messageType' @@ -1718,7 +1726,7 @@ - (void)project:(id)kitRegister } if (strongSelf) { - dispatch_semaphore_signal(strongSelf->kitsSemaphore); + dispatch_semaphore_signal(kitsSemaphore); } completionHandler(projectedCommerceEvents, projectedEvents, appliedProjections); @@ -1750,7 +1758,7 @@ - (void)project:(id)kitRegister __strong MPKitContainer_PRIVATE *strongSelf = weakSelf; if (strongSelf) { - dispatch_semaphore_wait(strongSelf->kitsSemaphore, DISPATCH_TIME_FOREVER); + dispatch_semaphore_wait(kitsSemaphore, DISPATCH_TIME_FOREVER); } // Attribute projection lambda function @@ -2033,7 +2041,7 @@ - (void)project:(id)kitRegister } if (strongSelf) { - dispatch_semaphore_signal(strongSelf->kitsSemaphore); + dispatch_semaphore_signal(kitsSemaphore); } completionHandler(projectedEvents, appliedProjections); @@ -2058,17 +2066,30 @@ - (void)project:(id)kitRegister - (void)removeAllSideloadedKits { // Remove all sideloaded kits as new instances will be provided in the new MParticleOptions + // The copy protects the enumeration but not the removal, so both run under the lock. + // + // Identify sideloaded registers by their code, not by asking wrapperInstance whether it + // responds to sideloadedKitCode. Both call sites (resetForSwitchingWorkspaces:, reset:) + // call flushSerializedKits immediately before this, and flushSerializedKits now detaches + // wrapperInstance to nil synchronously (to close a race with activeKitsRegistry - see that + // method) before this ever runs. [nil respondsToSelector:] is NO, so that check stopped + // matching anything once wrapperInstance was already nil, and sideloaded kits from the + // previous workspace were never removed. initWithInstance:kitCode: assigns every sideloaded + // kit a code >= sideloadedKitCodeStartValue, and that assignment survives the detach. + dispatch_semaphore_wait(kitsSemaphore, DISPATCH_TIME_FOREVER); NSSet *kits = [kitsRegistry copy]; for (idkitRegister in kits) { - if ([kitRegister.wrapperInstance respondsToSelector:@selector(sideloadedKitCode)]) { + if (kitRegister.code.integerValue >= sideloadedKitCodeStartValue) { [kitsRegistry removeObject:kitRegister]; } } + dispatch_semaphore_signal(kitsSemaphore); } - (void)removeKitsFromRegistryInvalidForWorkspaceSwitch { // Remove kits from registry that can't be freed so they won't receive new events // Leave any kit that was never used yet (i.e. was not used in the previous workspace) + dispatch_semaphore_wait(kitsSemaphore, DISPATCH_TIME_FOREVER); NSSet *kits = [kitsRegistry copy]; for (idkitRegister in kits) { if (![kitRegister.wrapperInstance respondsToSelector:@selector(stop)] && @@ -2076,6 +2097,7 @@ - (void)removeKitsFromRegistryInvalidForWorkspaceSwitch { [kitsRegistry removeObject:kitRegister]; } } + dispatch_semaphore_signal(kitsSemaphore); } - (nullable NSArray> *)activeKitsRegistry {