Crash in unregisterSuperProperty and currentSuperProperties

[daleswift]

Repeated calls to currentSuperProperties and unregisterSuperProperty can cause a crash.

We are seeing this occur frequently in our production app.

Thread 22 Crashed:
0   libsystem_kernel.dylib               0x00000001e231dfbc __pthread_kill + 8
1   libsystem_c.dylib                    0x00000001a2c65b90 abort + 176
2   libswiftCore.dylib                   0x0000000194347064 swift::fatalErrorv(unsigned int, char const*, char*) + 124
3   libswiftCore.dylib                   0x0000000194347084 swift::fatalError(unsigned int, char const*, ...) + 28
4   libswiftCore.dylib                   0x000000019434ba84 swift_deallocClassInstance + 304
5   libswiftCore.dylib                   0x000000019434b914 _swift_release_dealloc + 52
6   libswiftCore.dylib                   0x000000019434cfb0 bool swift::RefCounts<swift::RefCountBitsT<(swift::RefCountInlinedness)1> >::doDecrementSlow<(swift::PerformDeinit)1>(swift::RefCountBitsT<(swift::RefCountInlinedness)1>, unsigned int) + 132
7   nucleus-smart                        0x00000001009909d8 closure #1 () -> () in Mixpanel.MixpanelInstance.unregisterSuperProperty(Swift.String) -> () (<compiler-generated>:0)
8   nucleus-smart                        0x00000001009858cc reabstraction thunk helper from @escaping @callee_guaranteed () -> () to @escaping @callee_unowned @convention(block) () -> () (<compiler-generated>:0)
9   libdispatch.dylib                    0x00000001a2bab6a8 _dispatch_call_block_and_release + 28
10  libdispatch.dylib                    0x00000001a2bad300 _dispatch_client_callout + 16
11  libdispatch.dylib                    0x00000001a2bb4894 _dispatch_lane_serial_drain + 744
12  libdispatch.dylib                    0x00000001a2bb53c4 _dispatch_lane_invoke + 376
13  libdispatch.dylib                    0x00000001a2bc0004 _dispatch_root_queue_drain_deferred_wlh + 284
14  libdispatch.dylib                    0x00000001a2bbf878 _dispatch_workloop_worker_thread + 400
15  libsystem_pthread.dylib              0x0000000204bd7964 _pthread_wqthread + 284
16  libsystem_pthread.dylib              0x0000000204bd7a04 start_wqthread + 4

Diagnosis

In most cases MixPanelInstance references superProperties from a block executed from the trackingQueue and under readWriteLock and so the reference is thread safe. As in registerSuperProperties:

    public func registerSuperProperties(_ properties: Properties) {
        trackingQueue.async { [weak self] in
            guard let self = self else { return }
            let updatedSuperProperties = self.trackInstance.registerSuperProperties(properties,
                                                                                    superProperties: self.superProperties)
            self.readWriteLock.write {
                self.superProperties = updatedSuperProperties
            }
            self.readWriteLock.read {
                MixpanelPersistence.saveSuperProperties(superProperties: self.superProperties, instanceName: self.name)
            }
        }
    }

However, currentSuperProperties only protects superProperties with readWriteLock:

    public func currentSuperProperties() -> [String: Any] {
        var properties = InternalProperties()
        self.readWriteLock.read {
            properties = superProperties
        }
        return properties
    }

and unregisterSuperProperty only protects superProperties with trackingQueue:

    public func unregisterSuperProperty(_ propertyName: String) {
        trackingQueue.async { [weak self] in
            guard let self = self else { return }
            self.superProperties = self.trackInstance.unregisterSuperProperty(propertyName,
                                                                              superProperties: self.superProperties)
            MixpanelPersistence.saveSuperProperties(superProperties: self.superProperties, instanceName: self.name)
        }
    }

this causes a crash when both unregisterSuperProperty and currentSuperProperties are called from different threads at the same time.

The following unit test simulates the problem:

    func testCrash() {
        let token = randomId()
        let mixPanel = Mixpanel.initialize(token: token, trackAutomaticEvents: false)
        let property = "MySuperProperty"
        let queue1 = DispatchQueue(label: "queue1", qos: .default)

        for _ in 0...10000 {
            mixPanel.unregisterSuperProperty(property)
            mixPanel.registerSuperProperties([property: "1"])
        }

        queue1.async {
            while true {
                _ = mixPanel.currentSuperProperties()
            }
        }

        let expect = expectation(description: "tracking finished")
        mixPanel.trackingQueue.async {
            expect.fulfill()
        }
        wait(for: [expect])
    }

    func randomId() -> String {
        return String(format: "%08x%08x", arc4random(), arc4random())
    }

Solution

The solution should be straightforward. Reference superProperties under readWriteLock in unregisterSuperProperty and similarly in updateSuperProperty

[daleswift]

Submitted PR #623