Fix crash by making Current reentrant during setup (#1940)

Sources/Shared/API/ServerManager.swift

@@ -150,8 +150,8 @@ internal final class ServerManagerImpl: ServerManager {
         self.decoder = decoder
     }
 
-    func setup(environment: AppEnvironment) {
-        cache.restrictCaching = environment.isAppExtension
+    func setup() {
+        cache.restrictCaching = Current.isAppExtension
 
         // load to cache immediately
         _ = all
@@ -159,7 +159,7 @@ internal final class ServerManagerImpl: ServerManager {
         do {
             try migrateIfNeeded()
         } catch {
-            environment.Log.error("failed to load historic server: \(error)")
+            Current.Log.error("failed to load historic server: \(error)")
         }
     }
 
@@ -273,9 +273,11 @@ internal final class ServerManagerImpl: ServerManager {
     private func migrateIfNeeded() throws {
         guard all.isEmpty else { return }
 
-        let userDefaults = UserDefaults(suiteName: Constants.AppGroupID)!
+        let userDefaults = Current.settingsStore.prefs
         if let tokenInfoData = try historicKeychain.getData("tokenInfo"),
            let connectionInfoData = try historicKeychain.getData("connectionInfo") {
+            Current.Log.info("migrating historic server")
+
             // UserDefaults may be missing due to delete/reinstall, so fill in values for those if needed
             let versionString = userDefaults.string(forKey: "version") ?? "2021.1"
             let name = userDefaults.string(forKey: "location_name") ?? ServerInfo.defaultName
@@ -293,6 +295,8 @@ internal final class ServerManagerImpl: ServerManager {
 
             add(identifier: Server.historicId, serverInfo: serverInfo)
             try historicKeychain.removeAll()
+        } else {
+            Current.Log.info("no historic server found to import")
         }
     }
 
@@ -348,18 +352,31 @@ private extension ServerManagerKeychain {
     }
 
     func getServerInfo(key: String, decoder: JSONDecoder) -> ServerInfo? {
-        guard let data = try? getData(key) else {
+        do {
+            guard let data = try getData(key) else {
+                return nil
+            }
+
+            return try decoder.decode(ServerInfo.self, from: data)
+        } catch {
+            Current.Log.error("failed to get server info for \(key): \(error)")
             return nil
         }
-
-        return try? decoder.decode(ServerInfo.self, from: data)
     }
 
     func set(serverInfo: ServerInfo, key: String, encoder: JSONEncoder) {
-        try? set(encoder.encode(serverInfo), key: key)
+        do {
+            try set(encoder.encode(serverInfo), key: key)
+        } catch {
+            Current.Log.error("failed to set server info for \(key): \(error)")
+        }
     }
 
     func deleteServerInfo(key: String) {
-        try? remove(key)
+        do {
+            try remove(key)
+        } catch {
+            Current.Log.error("failed to get delete \(key): \(error)")
+        }
     }
 }

Sources/Shared/Environment/CrashReporter.swift

@@ -8,8 +8,8 @@ public protocol CrashReporter {
 }
 
 public class CrashReporterImpl: CrashReporter {
-    internal func setup(environment: AppEnvironment) {
-        guard environment.settingsStore.privacy.crashes else {
+    internal func setup() {
+        guard Current.settingsStore.privacy.crashes else {
             return
         }
 
@@ -17,14 +17,14 @@ public class CrashReporterImpl: CrashReporter {
             return
         }
 
-        environment.Log.add(destination: with(SentryLogDestination()) {
+        Current.Log.add(destination: with(SentryLogDestination()) {
             $0.outputLevel = .warning
         })
 
         SentrySDK.start { options in
             options.dsn = "https://762c198b86594fa2b6bedf87028db34d@o427061.ingest.sentry.io/5372775"
-            options.debug = environment.appConfiguration == .Debug
-            options.enableAutoSessionTracking = environment.settingsStore.privacy.analytics
+            options.debug = Current.appConfiguration == .Debug
+            options.enableAutoSessionTracking = Current.settingsStore.privacy.analytics
             options.maxBreadcrumbs = 1000
 
             var integrations = type(of: options).defaultIntegrations()
@@ -38,11 +38,11 @@ public class CrashReporterImpl: CrashReporter {
                 "SentryCrashIntegration",
             ])
 
-            if !environment.settingsStore.privacy.crashes {
+            if !Current.settingsStore.privacy.crashes {
                 integrations.removeAll(where: { crashesIntegrations.contains($0) })
             }
 
-            if !environment.settingsStore.privacy.analytics {
+            if !Current.settingsStore.privacy.analytics {
                 integrations.removeAll(where: { analyticsIntegrations.contains($0) })
             }
 

Sources/Shared/Environment/Environment.swift

@@ -27,7 +27,25 @@ public enum AppConfiguration: Int, CaseIterable, CustomStringConvertible {
     }
 }
 
-public var Current = AppEnvironment()
+private var underlyingWasSetUp: UInt32 = 0
+private var underlyingCurrent = AppEnvironment()
+
+public var Current: AppEnvironment {
+    get {
+        let result = underlyingCurrent
+        if OSAtomicTestAndSetBarrier(0, &underlyingWasSetUp) == false {
+            // we only want to run setup once, but we _must_ have 'Current' work during it to allow 'Current' to be
+            // reentrant, which is a requirement for touching things like Log but also touching more unexpected
+            // things like accessing any L10n helper value, which funnels through Current as well.
+            result.setup()
+        }
+        return result
+    }
+    set {
+        underlyingCurrent = newValue
+    }
+}
+
 /// The current "operating envrionment" the app. Implementations can be swapped out to facilitate better
 /// unit tests.
 public class AppEnvironment {
@@ -54,19 +72,17 @@ public class AppEnvironment {
             case .error: Current.Log.error(string)
             }
         }
+    }
 
-        let crashReporter = CrashReporterImpl()
-        self.crashReporter = crashReporter
+    internal func setup() {
+        _ = Current // just to make sure we don't crash for this case
 
-        let servers = ServerManagerImpl()
-        self.servers = servers
-
-        crashReporter.setup(environment: self)
-        servers.setup(environment: self)
+        (crashReporter as? CrashReporterImpl)?.setup()
+        (servers as? ServerManagerImpl)?.setup()
     }
 
     /// Crash reporting and related metadata gathering
-    public var crashReporter: CrashReporter
+    public var crashReporter: CrashReporter = CrashReporterImpl()
 
     /// Provides URLs usable for storing data.
     public var date: () -> Date = Date.init
@@ -84,7 +100,7 @@ public class AppEnvironment {
 
     public var style: Style = .init()
 
-    public var servers: ServerManager
+    public var servers: ServerManager = ServerManagerImpl()
 
     public var cachedApis = [Identifier<Server>: HomeAssistantAPI]()
 

Tests/Shared/ServerManager.test.swift

@@ -24,7 +24,7 @@ class ServerManagerTests: XCTestCase {
         }
 
         servers = ServerManagerImpl(keychain: keychain, historicKeychain: historicKeychain)
-        servers.setup(environment: Current)
+        servers.setup()
     }
 
     func testInitiallyEmptyAndGainingServersWithCaching() throws {
@@ -394,7 +394,7 @@ class ServerManagerTests: XCTestCase {
         Current.settingsStore.prefs.set(locationName, forKey: "location_name")
 
         servers = ServerManagerImpl(keychain: keychain, historicKeychain: historicKeychain)
-        servers.setup(environment: Current)
+        servers.setup()
 
         return .init(connectionInfo: connectionInfo, tokenInfo: tokenInfo)
     }