Refactor refreshConnectedDevices to tear down old device handles and re-register current devices to prevent orphaned callbacks

Author: NullPointerDepressiveDisorder

MiddleDrag/Managers/DeviceMonitor.swift

@@ -352,7 +352,13 @@ class DeviceMonitor: TouchDeviceProviding {
         timer.cancel()
     }
 
-    /// Finds newly connected devices and registers callbacks for them.
+    /// Tears down old device handles and re-registers the current device set.
+    ///
+    /// `MTDeviceCreateList()` may return different pointer addresses for the same
+    /// physical device on every call, so we cannot correlate old handles with new
+    /// ones. A full unregister-stop-then-register-start cycle on every refresh
+    /// prevents orphaned callbacks that would otherwise accumulate and deliver
+    /// duplicate touch events.
     func refreshConnectedDevices() {
         unsafe DeviceMonitor.lifecycleLock.lock()
         defer { unsafe DeviceMonitor.lifecycleLock.unlock() }
@@ -362,21 +368,41 @@ class DeviceMonitor: TouchDeviceProviding {
 
         guard unsafe isRunning else { return }
 
-        // Do not skip refreshes based only on the number of devices.
-        // A disconnect/connect swap can keep the count unchanged while the
-        // actual device set changes, which would otherwise leave the newly
-        // connected device unregistered.
         let devices = unsafe enumerator.enumerateDevices()
+        let previousCount = lastKnownDeviceCount
+
+        // --- tear down every old handle ---
+        let oldDevices: Set<UnsafeMutableRawPointer> =
+            unsafe Set(registeredDevicesByID.values.flatMap { unsafe $0 })
+        if !oldDevices.isEmpty {
+            // Unregister callbacks first (same ordering as stop()).
+            for unsafe deviceRef in unsafe oldDevices {
+                unsafe enumerator.unregisterContactCallback(deviceRef, deviceContactCallback)
+            }
+            // Stop each device under gCallbackLock so the framework's internal
+            // thread cannot access the handle mid-teardown.
+            for unsafe deviceRef in unsafe oldDevices {
+                unsafe os_unfair_lock_lock(&gCallbackLock)
+                unsafe enumerator.stopDevice(deviceRef)
+                unsafe os_unfair_lock_unlock(&gCallbackLock)
+            }
+            unsafe registeredDevicesByID.removeAll()
+            unsafe device = nil
+        }
 
+        // --- register fresh handles ---
         let registrationResult = unsafe registerAvailableDevices(
             logDeviceCount: false, prefetchedDevices: devices)
-        let addedCount = registrationResult.addedCount
 
-        if addedCount > 0 {
+        // Only log when the set of devices actually changed, not on every
+        // routine re-registration cycle.
+        if devices.count != previousCount {
             Log.info(
-                "Registered \(addedCount) newly connected multitouch device(s)",
+                "Multitouch device count changed: \(previousCount) → \(devices.count)",
                 category: .device)
         }
+
+        _ = registrationResult  // suppress unused-result warning
     }
 
     /// Enumerates currently available devices and registers any that are not already active.
@@ -433,8 +459,10 @@ class DeviceMonitor: TouchDeviceProviding {
 
     /// Registers callbacks and starts a device if we are not already monitoring it.
     ///
-    /// Note: Device identity is pointer-based and not stable across MTDeviceCreateList() calls.
-    /// Cross-refresh deduplication is handled by the count-based check in refreshConnectedDevices().
+    /// Device identity is pointer-based and not stable across `MTDeviceCreateList()` calls.
+    /// `refreshConnectedDevices()` handles this by tearing down all old handles before
+    /// calling `registerAvailableDevices`, so within a single registration pass the
+    /// pointer-based IDs are consistent.
     private func registerDeviceIfNeeded(
         _ deviceRef: MTDeviceRef?,
         deviceID: Int64?

MiddleDrag/MiddleDragTests/DeviceMonitorTests.swift

@@ -556,9 +556,9 @@ import Synchronization
         unsafe XCTAssertFalse(monitor.hasActiveRefreshTimer)
     }
 
-    // MARK: - Refresh Skip / Register
+    // MARK: - Refresh Teardown / Re-register
 
-    func testRefreshSkipsWhenDeviceCountUnchanged() {
+    func testRefreshTearsDownOldHandlesBeforeRegistering() {
         let enumerator = unsafe MockDeviceEnumerator()
         let device = unsafe makeFakeDevice()
         defer { unsafe device.deallocate() }
@@ -569,12 +569,62 @@ import Synchronization
         defer { unsafe monitor.stop() }
         unsafe monitor.start()
 
-        let callsBefore = unsafe enumerator.registeredCallbackDevices.count
+        // After start: 1 register, 1 start
+        unsafe XCTAssertEqual(enumerator.registeredCallbackDevices.count, 1)
+
+        // Refresh should unregister the old handle, then re-register it
         unsafe monitor.refreshConnectedDevices()
-        let callsAfter = unsafe enumerator.registeredCallbackDevices.count
 
-        XCTAssertEqual(callsBefore, callsAfter,
-                       "Refresh should skip registration when device count is unchanged")
+        unsafe XCTAssertTrue(enumerator.unregisteredCallbackDevices.contains(device),
+                             "Old handle should be unregistered during refresh")
+        // Total registrations: 1 (start) + 1 (refresh re-register)
+        unsafe XCTAssertEqual(enumerator.registeredCallbackDevices.count, 2)
+    }
+
+    func testRefreshWithUnstablePointersDoesNotLeakCallbacks() {
+        // Simulates the core pointer-instability bug:
+        // MTDeviceCreateList() returns a different pointer for the same
+        // physical device on each call. Without the teardown step, this
+        // would orphan the old callback and register a duplicate.
+        let enumerator = unsafe MockDeviceEnumerator()
+        let ptr1 = unsafe makeFakeDevice()
+        let ptr2 = unsafe makeFakeDevice()
+        let ptr3 = unsafe makeFakeDevice()
+        defer {
+            unsafe ptr1.deallocate()
+            unsafe ptr2.deallocate()
+            unsafe ptr3.deallocate()
+        }
+        unsafe enumerator.devices = [ptr1]
+        unsafe enumerator.defaultDevice = ptr1
+
+        let monitor = unsafe makeMonitor(enumerator: enumerator)
+        defer { unsafe monitor.stop() }
+        unsafe monitor.start()
+
+        // Simulate framework returning different pointer for same device
+        unsafe enumerator.devices = [ptr2]
+        unsafe enumerator.defaultDevice = ptr2
+        unsafe monitor.refreshConnectedDevices()
+
+        // ptr1 must have been torn down
+        unsafe XCTAssertTrue(enumerator.unregisteredCallbackDevices.contains(ptr1),
+                             "Old pointer should have its callback unregistered")
+        unsafe XCTAssertTrue(enumerator.stoppedDevices.contains(ptr1),
+                             "Old pointer should be stopped")
+
+        // Do it again with a third pointer
+        unsafe enumerator.devices = [ptr3]
+        unsafe enumerator.defaultDevice = ptr3
+        unsafe monitor.refreshConnectedDevices()
+
+        unsafe XCTAssertTrue(enumerator.unregisteredCallbackDevices.contains(ptr2))
+        unsafe XCTAssertTrue(enumerator.stoppedDevices.contains(ptr2))
+
+        // Total unregisters should match: ptr1, ptr2 (not ptr3 — still active)
+        let unregCount = unsafe enumerator.unregisteredCallbackDevices.count
+        XCTAssertEqual(unregCount, 2,
+                       "Exactly the old handles should be unregistered, no leaks")
     }
 
     func testRefreshRegistersWhenDeviceCountIncreases() {
@@ -591,12 +641,12 @@ import Synchronization
 
         // Simulate connecting a second device
         unsafe enumerator.devices = [device1, device2]
-        let callsBefore = unsafe enumerator.registeredCallbackDevices.count
         unsafe monitor.refreshConnectedDevices()
-        let callsAfter = unsafe enumerator.registeredCallbackDevices.count
 
-        XCTAssertGreaterThan(callsAfter, callsBefore,
-                             "Refresh should register devices when count increases")
+        // Both devices should have been registered in the refresh pass
+        unsafe XCTAssertEqual(monitor.lastKnownDeviceCount, 2)
+        unsafe XCTAssertTrue(enumerator.registeredCallbackDevices.contains(device2),
+                             "Newly connected device should be registered")
     }
 
     func testRefreshHandlesDeviceDisconnect() {
@@ -618,6 +668,9 @@ import Synchronization
 
         unsafe XCTAssertEqual(monitor.lastKnownDeviceCount, 1,
                               "Count should update after device disconnect")
+        // device2 should have been torn down
+        unsafe XCTAssertTrue(enumerator.unregisteredCallbackDevices.contains(device2))
+        unsafe XCTAssertTrue(enumerator.stoppedDevices.contains(device2))
     }
 
     func testRefreshUpdatesLastKnownDeviceCount() {