Skip to content

fix: eliminate the kitsRegistry thread-safety crash #916

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.

Sign up for GitHub

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

Merged
thomson-t merged 5 commits into from
Sep 4, 2026
Merged

fix: eliminate the kitsRegistry thread-safety crash #916

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b89060d
fix: scope the kit registry lock to the state it guards
nickolas-dimitrakas Sep 1, 2026
9c5968b
fix: synchronize the wrapperInstance handoff in flushSerializedKits
nickolas-dimitrakas Sep 2, 2026
61e406f
fix: silence analyzer warning on the NSNull sentinel ternary
nickolas-dimitrakas Sep 2, 2026
3a3ee9a
fix: identify sideloaded kits by code, not by a soon-to-be-nil wrappe…
nickolas-dimitrakas Sep 2, 2026
ab721dc
fix: defer freeKitRegister's kit teardown off kitsSemaphore
nickolas-dimitrakas Sep 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 61 additions & 39 deletions mParticle-Apple-SDK/Kits/MPKitContainer.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@

NSString *const kitFileExtension = @"eks";
static NSMutableSet <id<MPExtensionKitProtocol>> *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
Expand Down Expand Up @@ -79,12 +83,12 @@ - (nullable instancetype)initWithInstance:(nonnull NSObject<MPKitProtocol> *)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<NSNumber *, MPBracket *> *brackets;
NSInteger sideloadedKitCodeNextValue;
Expand All @@ -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();
}
}
Expand All @@ -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;
Expand Down Expand Up @@ -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<id<MPExtensionKitProtocol>> *kitsSnapshot = [kitsRegistry allObjects];
NSMutableArray<id> *detachedWrapperInstances = [[NSMutableArray alloc] initWithCapacity:kitsSnapshot.count];
for (id<MPExtensionKitProtocol> 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.
Expand All @@ -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 (id<MPExtensionKitProtocol>kitRegister in kitsSnapshot) {
id<MPKitProtocol> wrapperInstance = kitRegister.wrapperInstance;
if (wrapperInstance) {
kitRegister.wrapperInstance = nil;
[kitsSnapshot enumerateObjectsUsingBlock:^(id<MPExtensionKitProtocol> kitRegister, NSUInteger idx, BOOL *stop) {
id wrapperInstance = detachedWrapperInstances[idx];
if (wrapperInstance != (id)[NSNull null]) {
[self teardownDetachedWrapperInstance:wrapperInstance forIntegrationId:kitRegister.code];
}
}
}];
dispatch_group_leave(kitTeardownGroup);
});
}
Expand All @@ -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<MPExtensionKitProtocol>)kitRegister integrationId:(NSNumber *)integrationId {
NSAssert(integrationId != nil, @"Required parameter. It cannot be nil.");

Expand All @@ -317,12 +325,12 @@ - (void)freeKitRegister:(id<MPExtensionKitProtocol>)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<MPKitProtocol>)wrapperInstance forIntegrationId:(NSNumber *)integrationId {
if ([wrapperInstance respondsToSelector:@selector(stop)]) {
[wrapperInstance stop];
Expand Down Expand Up @@ -1243,7 +1251,7 @@ - (void)project:(id<MPExtensionKitProtocol>)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'
Expand Down Expand Up @@ -1718,7 +1726,7 @@ - (void)project:(id<MPExtensionKitProtocol>)kitRegister
}

if (strongSelf) {
dispatch_semaphore_signal(strongSelf->kitsSemaphore);
dispatch_semaphore_signal(kitsSemaphore);
}

completionHandler(projectedCommerceEvents, projectedEvents, appliedProjections);
Expand Down Expand Up @@ -1750,7 +1758,7 @@ - (void)project:(id<MPExtensionKitProtocol>)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
Expand Down Expand Up @@ -2033,7 +2041,7 @@ - (void)project:(id<MPExtensionKitProtocol>)kitRegister
}

if (strongSelf) {
dispatch_semaphore_signal(strongSelf->kitsSemaphore);
dispatch_semaphore_signal(kitsSemaphore);
}

completionHandler(projectedEvents, appliedProjections);
Expand All @@ -2058,24 +2066,38 @@ - (void)project:(id<MPExtensionKitProtocol>)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 (id<MPExtensionKitProtocol>kitRegister 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 (id<MPExtensionKitProtocol>kitRegister in kits) {
if (![kitRegister.wrapperInstance respondsToSelector:@selector(stop)] &&
[self.kitConfigurations.allKeys containsObject:[kitRegister.wrapperInstance.class kitCode]]) {
[kitsRegistry removeObject:kitRegister];
}
}
dispatch_semaphore_signal(kitsSemaphore);
}

- (nullable NSArray<id<MPExtensionKitProtocol>> *)activeKitsRegistry {
Expand Down
Loading