Fix UnfairLock deadlock

[paulb777]

Fix #15394

The os_unfair_lock is causing a deadlock due to its lack of recursion support. Fixed by replacing it with NSRecursiveLock in UnfairLock.swift. This involves updating the import, properties, and lock/unlock methods to use the new recursive lock, while keeping the public API unchanged.

The core of the problem, as indicated by the stack trace, is a deadlock occurring within FirebaseCoreInternal.UnfairLock. This UnfairLock is a low-level locking primitive used by various Firebase modules, including Firebase Sessions.

The stack trace clearly shows:

1. SessionInitiator.appForegrounded() (from Firebase Sessions) is involved.
2. It calls into RemoteSettings.sessionTimeout.getter (also from Firebase Sessions, as per the file structure
   FirebaseSessions/Sources/Settings/RemoteSettings.swift).
3. This getter then attempts to acquire the FirebaseCoreInternal.UnfairLock.

The issue arises because os_unfair_lock (which UnfairLock was wrapping) is not recursive. If the main thread attempts to acquire this lock while it already holds it (due to nested calls or multiple observers being triggered by the same event, like app foregrounding), it will deadlock.

Are there any usages of UnfairLock across the repository that require it to not be recursive?

✦ That's a very important question. A recursive lock behaves differently from a non-recursive one, and it's crucial to ensure this change doesn't introduce unintended side effects.

I'll investigate all usages of UnfairLock in the codebase to determine if any of them rely on the non-recursive behavior. My plan is as follows:

1. Find all usages: I'll perform a repository-wide search for where UnfairLock is instantiated and used.
2. Analyze each usage: I will examine the code within each lock to check for any logic that might be sensitive to the lock's recursivity.

I'll start by finding all occurrences of "UnfairLock". │
│ Found 42 matches │

✦ The search results show UnfairLock is used across several Firebase modules (Functions, AI, Sessions, Auth, Core) primarily for safeguarding mutable properties.

Here's my analysis of the usage patterns:

• Simple State Guarding: In almost all cases, the lock is used to protect simple properties (Strings, Bools, Dictionaries, Sets, etc.) from concurrent access. The code inside the withLock blocks is very simple: it's usually a direct read, a value assignment, or a simple collection mutation (e.g., Set.insert).
• No Re-entrancy: I have not found any instances where the code within a withLock closure calls another function that would then try to acquire the same lock again. The operations are atomic and self-contained.

The original crash was caused by a deadlock when different parts of the Sessions module, both responding to foregrounding notifications, tried to acquire the same non-recursive lock.

Given these observations, changing to a recursive lock is a safe and correct fix. It solves the deadlock problem without creating new issues because the existing code doesn't rely on the non-recursive behavior. The recursive lock will simply continue to work for simple cases and prevent deadlocks in the more complex re-entrant scenarios that were causing your crash.

[gemini-code-assist]

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

[paulb777]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses a critical deadlock issue by replacing the non-recursive os_unfair_lock with NSRecursiveLock in the UnfairLock utility. The change is straightforward and effectively resolves the re-entrancy problem that was causing deadlocks. The investigation into all usages of UnfairLock to ensure this change is safe is commendable. The code is clean and the change is well-contained. I have one minor stylistic suggestion.

[ncooke3]

I don't think this PR will solve the problem. I revisited this issue last week before holidays and I think the proper solution needs to be more involved.

Based on the stack trace in the issue.

• Thread 0 (main queue) wants access to a lock
• Thread 3 (background queue) has access to the lock that thread 0 wants.
  • Part of the work in the locked context is to synchronously post a notification to the main queue because it's blocked.

I had a few ideas to experiment with like trying to see if the notification could be posted off the main thread. The proper solution seemed like moving from theses locks to an actor based approach which seems possible now that Sessions is iOS 13+.

[paulb777]

Closing based on Nick's comment. This will require more thought.