bitwarden / bitwarden/ios

[PM-38023] Memory leak: DefaultTOTPExpirationManager Timer retain cycle prevents deinit

Open Beginner friendly
#2,698 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Swift
Stars
684
Forks
154
Avg merge
3d 22h
Merged PRs (30d)
24

Description

## Summary

`DefaultTOTPExpirationManager.init` schedules a repeating `Timer` whose block captures `self` strongly. This forms a retain cycle (`Manager → updateTimer → Timer → block → self`) that prevents `deinit` from running, which means `cleanup()` (the method that invalidates the Timer) is never called. Every instance ever constructed stays alive for the process lifetime.

## How this was discovered

I'm running an ongoing iOS memory-leak hunt, write-up of the methodology and first episode here: [Hunting iOS Memory Leaks — S1E1](https://www.amanjeet.me/hunting-ios-memory-leaks-s1e1/).

The initial audit surfaced only leaks in unit tests (uses macOS `leaks(1)`).

A subsequent agent-based audit of production patterns then revealed this `DefaultTOTPExpirationManager` issue, which is "abandoned memory" rather than a strict leak, reachable from a process root (the main RunLoop, which owns the Timer), that apparently is invisible to the leaks tool.

I learned leaks tool only considers a leak when objects are not reachable from the root of the graph, so this is a different type of leak where its abandoned sitting in heap, still connected to root run loop though.

## The bug

`BitwardenShared/UI/Vault/Utilities/TOTPExpirationManager.swift` (lines 56–74):

```swift
init(
timeProvider: any TimeProvider,
onExpiration: (([VaultListItem]) -> Void)?,
) {
self.timeProvider = timeProvider
self.onExpiration = onExpiration
updateTimer = Timer.scheduledTimer(
withTimeInterval: 0.25,
repeats: true,
block: { _ in
self.checkForExpirations() // ← strong self capture
},
)
}

deinit {
cleanup() // ← never called, because of the cycle
}
```

The cycle:
- `Manager` holds `updateTimer` (strong, stored property)
- `Timer` holds its block (strong, Foundation behavior)
- block captures `self` strongly (default Swift closure capture)
- back to `Manager`

ARC requires every edge in a cycle to be strong for the cycle to leak — all three are. `deinit` never runs; `cleanup()` is never invoked.

## Impact

Per leaked instance:
- ~96-byte `DefaultTOTPExpirationManager`
- 64-byte `Timer` + its block
- `itemsByInterval` dictionary (sized by number of TOTP items the screen was tracking)
- `onExpiration` closure (captures whatever the caller passed — current call sites use `[weak self]`, so doesn't propagate further)
- `timeProvider`

Each leaked manager also continues running its scheduled timer for the rest of the process lifetime on stale data — wasted work that compounds with each accumulated instance.

Scope: every `VaultGroup` screen view, every Vault search interaction with TOTP-bearing items, every TOTP autofill flow creates at least one instance.

## Proving the leak with a unit test

Because the cycle is reachable from a process root, `leaks(1)`-based audits report no leak.

The verification has to ask a different question, "is this object alive when it shouldn't be?", which a weak-reference XCTest can encode directly:

```swift
import BitwardenKit
import BitwardenKitMocks
import XCTest
@testable import BitwardenShared

@MainActor
func test_DefaultTOTPExpirationManager_deallocatesAfterRelease() async throws {
weak var weakManager: DefaultTOTPExpirationManager?

do {
let manager = DefaultTOTPExpirationManager(
timeProvider: MockTimeProvider(.currentTime),
onExpiration: { _ in },
)
weakManager = manager
}

// Let any in-flight work settle so deinit (if it could fire) would have.
try await Task.sleep(nanoseconds: 200_000_000)

XCTAssertNil(
weakManager,
"Manager should deallocate after release; retain cycle prevents deinit.",
)
}
```

Run on current `main`, this test fails with `XCTAssertNil failed: "BitwardenShared.DefaultTOTPExpirationManager"`, the manager is still alive 200 ms after the only strong reference was dropped. Run after either fix below, it passes.

I used a red green verification to verify if its fixed.

## Fix options

### Option A — Minimal: `[weak self]` in the Timer block

```diff
- block: { _ in
- self.checkForExpirations()
- }
+ block: { [weak self] _ in
+ self?.checkForExpirations()
+ }
```

- One-line change, non-breaking, no callers affected.
- Breaks the cycle; `deinit` runs normally; `cleanup()` runs automatically.
- The existing `VaultGroupProcessor` workaround becomes defensive instead of load-bearing.

### Option B: Structural: explicit `start()`/`stop()` lifecycle

Move Timer scheduling out of `init` into a new `start()` method, paired with `stop()`. `init` becomes pure — no side effects.

- This is a design hygiene that makes caller aware about the lifecycle of timer
- More robust: `deinit`-based cleanup no longer load-bearing.
- This would also need updating the callers, and would have more surface area of change.
- I can go with this option if you like, but I wanted to keep the behavioural change minimum.

## Recommendation

**Option A** as the immediate fix, one-line, non-breaking, ships the bugfix without any caller migration. `[weak self]` is the actual change that resolves the cycle; the structural refactor doesn't fix the bug by itself (verified by isolating the changes in the unit test).

I'll open a PR for Option A linked to this issue, including the weak-reference regression test above.

Option B is worth doing as a follow-up to make the design less fragile against future regressions, but is a separate architectural conversation. Happy to open a separate Discussion for it if there's interest.

Contributor guide

Open the contributing guide

Research direction

Start in BitwardenShared/UI/Vault/Utilities/TOTPExpirationManager.swift, especially the Timer setup in init and its cleanup path. Run the weak-reference regression test described in the issue, then verify that the manager deallocates after its last strong reference is released and that the existing behavior tests still pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
swift
Domain
mobile
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.