Merge release/23.5 into trunk (#16266)

Author: iamgabrielma

Modules/Sources/NetworkingCore/Network/AlamofireNetworkErrorHandler.swift

@@ -4,6 +4,8 @@ import Alamofire
 /// Thread-safe handler for network error tracking and retry logic
 final class AlamofireNetworkErrorHandler {
     private let queue = DispatchQueue(label: "com.networkingcore.errorhandler", attributes: .concurrent)
+    /// Serial queue for UserDefaults operations to prevent race conditions while avoiding deadlocks
+    private let userDefaultsQueue = DispatchQueue(label: "com.networkingcore.errorhandler.userdefaults")
     private let userDefaults: UserDefaults
     private let credentials: Credentials?
     private let notificationCenter: NotificationCenter
@@ -162,7 +164,11 @@ final class AlamofireNetworkErrorHandler {
     }
 
     func flagSiteAsUnsupported(for siteID: Int64, flow: RequestFlow, cause: AppPasswordFlagCause, error: Error) {
-        queue.sync(flags: .barrier) {
+        // Use dedicated serial queue for UserDefaults operations to:
+        // 1. Prevent race conditions where concurrent writes overwrite each other
+        // 2. Avoid deadlock by not using the main queue that KVO observers may need
+        userDefaultsQueue.sync { [weak self] in
+            guard let self else { return }
             var currentList = userDefaults.applicationPasswordUnsupportedList
             currentList[String(siteID)] = Date()
             userDefaults.applicationPasswordUnsupportedList = currentList
@@ -233,11 +239,16 @@ private extension AlamofireNetworkErrorHandler {
     }
 
     func clearUnsupportedFlag(for siteID: Int64) {
-        queue.sync(flags: .barrier) {
+        // Use dedicated serial queue for UserDefaults operations to:
+        // 1. Prevent race conditions where concurrent writes overwrite each other
+        // 2. Avoid deadlock by not using the main queue that KVO observers may need
+        userDefaultsQueue.sync { [weak self] in
+            guard let self else { return }
             let currentList = userDefaults.applicationPasswordUnsupportedList
-            userDefaults.applicationPasswordUnsupportedList = currentList.filter { flag in
+            let filteredList = currentList.filter { flag in
                 flag.key != String(siteID)
             }
+            userDefaults.applicationPasswordUnsupportedList = filteredList
         }
     }
 

Modules/Tests/NetworkingTests/Network/AlamofireNetworkErrorHandlerTests.swift

@@ -606,6 +606,109 @@ final class AlamofireNetworkErrorHandlerTests: XCTestCase {
         // Then - no crashes should occur (especially no EXC_BREAKPOINT from array index out of bounds)
         wait(for: [expectation], timeout: 5.0)
     }
+
+    // MARK: - Deadlock Regression Test
+
+    func test_no_deadlock_when_kvo_observer_triggers_during_flagSiteAsUnsupported() {
+        // This test reproduces the exact deadlock scenario from the production crash:
+        // 1. flagSiteAsUnsupported writes to UserDefaults
+        // 2. UserDefaults triggers KVO notification synchronously
+        // 3. KVO observer calls prepareAppPasswordSupport
+        // 4. prepareAppPasswordSupport accesses appPasswordFailures
+        //
+        // BEFORE FIX: This would deadlock because:
+        // - flagSiteAsUnsupported used queue.sync(flags: .barrier) around UserDefaults write
+        // - KVO fired synchronously during the barrier
+        // - prepareAppPasswordSupport tried queue.sync while barrier was still active
+        //
+        // AFTER FIX: No deadlock because:
+        // - flagSiteAsUnsupported uses userDefaultsQueue.async for UserDefaults write
+        // - KVO fires on userDefaultsQueue, not the main queue
+        // - prepareAppPasswordSupport can safely use queue.sync on the main queue
+
+        // Given - Set up KVO observer to simulate the production scenario
+        let siteID: Int64 = 12345
+        let kvoTriggered = XCTestExpectation(description: "KVO observer triggered")
+        let preparePasswordSupportCalled = XCTestExpectation(description: "prepareAppPasswordSupport called from KVO")
+        let operationCompleted = XCTestExpectation(description: "Operation completed without deadlock")
+
+        var kvoObservation: NSKeyValueObservation?
+        kvoObservation = userDefaults.observe(\.applicationPasswordUnsupportedList, options: [.new]) { [weak self] _, _ in
+            kvoTriggered.fulfill()
+
+            // Simulate what happens in production:
+            // AlamofireNetwork.observeSelectedSite gets triggered by KVO
+            // and calls prepareAppPasswordSupport
+            self?.errorHandler.prepareAppPasswordSupport(for: siteID)
+            preparePasswordSupportCalled.fulfill()
+        }
+
+        // When - Trigger the scenario that caused the deadlock
+        DispatchQueue.global().async {
+            // This will write to UserDefaults, triggering KVO
+            self.errorHandler.flagSiteAsUnsupported(
+                for: siteID,
+                flow: .apiRequest,
+                cause: .majorError,
+                error: NetworkError.notFound(response: nil)
+            )
+            operationCompleted.fulfill()
+        }
+
+        // Then - All expectations should complete without timing out (no deadlock)
+        // The timeout of 2 seconds is generous - if there's a deadlock, this will timeout
+        let result = XCTWaiter.wait(
+            for: [kvoTriggered, preparePasswordSupportCalled, operationCompleted],
+            timeout: 2.0,
+            enforceOrder: false
+        )
+
+        XCTAssertEqual(result, .completed, "Test should complete without deadlock. If this times out, the deadlock bug has returned!")
+
+        // Cleanup
+        kvoObservation?.invalidate()
+    }
+
+    func test_no_deadlock_with_concurrent_kvo_observers_and_flag_operations() {
+        // This test creates even more stress by having multiple KVO observers
+        // and concurrent flag operations to ensure the fix is robust
+
+        let completionExpectation = XCTestExpectation(description: "All operations complete")
+        completionExpectation.expectedFulfillmentCount = 10 // 10 flag operations
+
+        var observations: [NSKeyValueObservation] = []
+
+        // Set up multiple KVO observers (simulating multiple parts of the app observing)
+        for i in 1...3 {
+            let observation = userDefaults.observe(\.applicationPasswordUnsupportedList, options: [.new]) { [weak self] _, _ in
+                let siteID = Int64(1000 + i)
+                // Each observer tries to access the error handler
+                self?.errorHandler.prepareAppPasswordSupport(for: siteID)
+            }
+            observations.append(observation)
+        }
+
+        // When - Perform multiple concurrent operations that trigger KVO
+        for i in 0..<10 {
+            DispatchQueue.global().async {
+                let siteID = Int64(i)
+                self.errorHandler.flagSiteAsUnsupported(
+                    for: siteID,
+                    flow: .apiRequest,
+                    cause: .majorError,
+                    error: NetworkError.notFound(response: nil)
+                )
+                completionExpectation.fulfill()
+            }
+        }
+
+        // Then - Should complete without deadlock
+        let result = XCTWaiter.wait(for: [completionExpectation], timeout: 3.0)
+        XCTAssertEqual(result, .completed, "Concurrent operations with multiple KVO observers should not deadlock")
+
+        // Cleanup
+        observations.forEach { $0.invalidate() }
+    }
 }
 
 // MARK: - Helper Methods

WooCommerce/Classes/Authentication/SiteCredentialLoginUseCase.swift

@@ -100,15 +100,16 @@ enum SiteCredentialLoginError: LocalizedError {
 /// This use case handles site credential login without the need to use XMLRPC API.
 /// Steps for login:
 /// - Make a request to the site wp-login.php with a redirect to the nonce retrieval URL.
-/// - Upon redirect, cancel the request and verify if the redirect URL is the nonce retrieval URL.
-/// - If it is, make a request to retrieve nonce at that URL, the login succeeds if this is successful.
+/// - If the redirect succeeds with a nonce in the response, login is successful.
+/// - If the request does not redirect or the redirect fails, login fails.
+/// Ref: pe5sF9-1iQ-p2
 ///
 final class SiteCredentialLoginUseCase: NSObject, SiteCredentialLoginProtocol {
     private let siteURL: String
     private let cookieJar: HTTPCookieStorage
     private var successHandler: (() -> Void)?
     private var errorHandler: ((SiteCredentialLoginError) -> Void)?
-    private lazy var session = URLSession(configuration: .default, delegate: self, delegateQueue: nil)
+    private lazy var session = URLSession(configuration: .default)
 
     init(siteURL: String,
          cookieJar: HTTPCookieStorage = HTTPCookieStorage.shared) {
@@ -134,10 +135,9 @@ final class SiteCredentialLoginUseCase: NSObject, SiteCredentialLoginProtocol {
         Task { @MainActor in
             do {
                 try await startLogin(with: loginRequest)
+                successHandler?()
             } catch let error as SiteCredentialLoginError {
                 errorHandler?(error)
-            } catch let nsError as NSError where nsError.domain == NSURLErrorDomain && nsError.code == -999 {
-                /// login request is cancelled upon redirect, ignore this error
             } catch {
                 errorHandler?(.genericFailure(underlyingError: error as NSError))
             }
@@ -160,10 +160,26 @@ private extension SiteCredentialLoginUseCase {
             throw SiteCredentialLoginError.invalidLoginResponse
         }
 
-        switch response.statusCode {
-        case 404:
+        /// The login request comes with a redirect header to nonce retrieval URL.
+        /// If we get a response from this URL, that means the redirect is successful.
+        /// We need to check the result of this redirect first to determine if login is successful.
+        let isNonceUrl = response.url?.absoluteString.hasSuffix(Constants.wporgNoncePath) == true
+
+        switch (isNonceUrl, response.statusCode) {
+        case (true, 200):
+            if let nonceString = String(data: data, encoding: .utf8),
+               nonceString.isValidNonce() {
+                // success!
+                return
+            } else {
+                throw SiteCredentialLoginError.invalidLoginResponse
+            }
+        case (true, 404):
+            throw SiteCredentialLoginError.inaccessibleAdminPage
+        case (false, 404):
             throw SiteCredentialLoginError.inaccessibleLoginPage
-        case 200:
+        case (false, 200):
+            // 200 for the login URL, which means a failure
             guard let html = String(data: data, encoding: .utf8) else {
                 throw SiteCredentialLoginError.invalidLoginResponse
             }
@@ -180,39 +196,6 @@ private extension SiteCredentialLoginUseCase {
         }
     }
 
-    @MainActor
-    func checkRedirect(url: URL?) async {
-        guard let url, url.absoluteString.hasSuffix(Constants.wporgNoncePath),
-              let nonceRetrievalURL = URL(string: siteURL + Constants.adminPath + Constants.wporgNoncePath) else {
-            errorHandler?(.invalidLoginResponse)
-            return
-        }
-        do {
-            let nonceRequest = try URLRequest(url: nonceRetrievalURL, method: .get)
-            try await checkAdminPageAccess(with: nonceRequest)
-            successHandler?()
-        } catch let error as SiteCredentialLoginError {
-            errorHandler?(error)
-        } catch {
-            errorHandler?(.genericFailure(underlyingError: error as NSError))
-        }
-    }
-
-    func checkAdminPageAccess(with nonceRequest: URLRequest) async throws {
-        let (_, response) = try await session.data(for: nonceRequest)
-        guard let response = response as? HTTPURLResponse else {
-            throw SiteCredentialLoginError.invalidLoginResponse
-        }
-        switch response.statusCode {
-        case 200:
-            return // success 🎉
-        case 404:
-            throw SiteCredentialLoginError.inaccessibleAdminPage
-        default:
-            throw SiteCredentialLoginError.unacceptableStatusCode(code: response.statusCode)
-        }
-    }
-
     func buildLoginRequest(username: String, password: String) -> URLRequest? {
         guard let loginURL = URL(string: siteURL + Constants.loginPath) else {
             return nil
@@ -239,18 +222,6 @@ private extension SiteCredentialLoginUseCase {
     }
 }
 
-extension SiteCredentialLoginUseCase: URLSessionDataDelegate {
-    func urlSession(_ session: URLSession,
-                    task: URLSessionTask,
-                    willPerformHTTPRedirection response: HTTPURLResponse,
-                    newRequest request: URLRequest) async -> URLRequest? {
-        // Disables redirect and check if the redirect is correct
-        task.cancel()
-        await checkRedirect(url: request.url)
-        return nil
-    }
-}
-
 extension SiteCredentialLoginUseCase {
     enum Constants {
         static let loginPath = "/wp-login.php"
@@ -310,4 +281,15 @@ private extension String {
     func hasInvalidCredentialsPattern() -> Bool {
         contains("document.querySelector('form').classList.add('shake')")
     }
+
+    /// Validates if the string matches the expected nonce format.
+    /// A valid nonce should contain at least 2 alphanumeric characters.
+    ///
+    func isValidNonce() -> Bool {
+        guard let regex = try? Regex("^[0-9a-zA-Z]{2,}$") else {
+            DDLogError("⚠️ Invalid regex pattern")
+            return false
+        }
+        return wholeMatch(of: regex) != nil
+    }
 }

WooCommerce/Classes/System/SessionManager.swift

@@ -54,6 +54,10 @@ final class SessionManager: SessionManagerProtocol {
     ///
     private let imageCache: ImageCache
 
+    /// Serial queue for thread-safe credentials access
+    ///
+    private let credentialsQueue = DispatchQueue(label: "com.woocommerce.session.credentials", qos: .userInitiated)
+
     /// Makes sure the credentials are in sync with the watch session.
     ///
     private lazy var watchDependenciesSynchronizer = {
@@ -76,20 +80,31 @@ final class SessionManager: SessionManagerProtocol {
     ///
     var defaultCredentials: Credentials? {
         get {
-            return loadCredentials()
+            credentialsQueue.sync {
+                loadCredentials()
+            }
         }
         set {
-            guard newValue != defaultCredentials else {
-                return
-            }
+            let shouldUpdateWatchSync = credentialsQueue.sync { () -> Bool in
+                let currentCredentials = loadCredentials()
+                guard newValue != currentCredentials else {
+                    return false
+                }
 
-            removeCredentials()
+                removeCredentials()
 
-            if let credentials = newValue {
-                saveCredentials(credentials)
+                if let credentials = newValue {
+                    saveCredentials(credentials)
+                }
+
+                return true
             }
 
-            watchDependenciesSynchronizer.credentials = newValue
+            // Update watch synchronizer outside the sync block to avoid potential deadlocks
+            // from @Published property triggering Combine subscribers that might read credentials
+            if shouldUpdateWatchSync {
+                watchDependenciesSynchronizer.credentials = newValue
+            }
         }
     }
 
@@ -235,7 +250,7 @@ final class SessionManager: SessionManagerProtocol {
     /// Deletes application password
     ///
     func deleteApplicationPassword(using creds: Credentials?, locally: Bool) {
-        let useCase: ApplicationPasswordUseCase? = {
+        let useCase: ApplicationPasswordUseCase? = credentialsQueue.sync {
             let credentials = creds ?? loadCredentials()
             switch credentials {
             case let .wporg(username, password, siteAddress):
@@ -253,7 +268,7 @@ final class SessionManager: SessionManagerProtocol {
             case .none:
                 return nil
             }
-        }()
+        }
         guard let useCase else {
             return
         }