Skip to content

Fix retain cycles in AnnouncementsDataStoreTests and GutenbergVideoUploadProcessor#25548

Open
amanjeetsingh150 wants to merge 2 commits intowordpress-mobile:trunkfrom
amanjeetsingh150:fix/retain-cycles-leak-audit
Open

Fix retain cycles in AnnouncementsDataStoreTests and GutenbergVideoUploadProcessor#25548
amanjeetsingh150 wants to merge 2 commits intowordpress-mobile:trunkfrom
amanjeetsingh150:fix/retain-cycles-leak-audit

Conversation

@amanjeetsingh150
Copy link
Copy Markdown
Contributor

@amanjeetsingh150 amanjeetsingh150 commented May 8, 2026

Closes #25547.

Report:

wp-local-audit-Xcode26.0.zip

Description

Fixes two distinct retain cycles surfaced by XCTestLeaks against the WordPress scheme.

1. AnnouncementsDataStoreTests (test code)

store → changeDispatcher → observers[token] → closure → store

The four tests stash subscription = store.onChange { … store.x … } as an instance property; the closure captures the local store strongly and is retained by Dispatcher.observers. Without a tearDown the Receipt only deinits when XCTest tears the test instance down, which is deferred when the class has multiple queued tests — long enough for the post-test leak snapshot to observe the cycle.

Fix: add tearDown that nils subscription. The Receipt's deinit then calls unsubscribe immediately, removing the closure from observers and breaking the cycle.

2. GutenbergVideoUploadProcessor (production code)

GutenbergVideoUploadProcessor
  → videoHtmlProcessor (HTMLProcessor) [lazy strong]
    → replacer.context → captures self strongly
      → GutenbergVideoUploadProcessor (cycle)

Three lazy var properties construct child processors whose replacer: closures reference self.remoteURLString / self.mediaUploadID without a capture list. Each lazy property is strongly owned by self and retains a closure that captures self strongly — three independent two-node cycles.

Fix: [weak self] + guard let self in all three closures. By ownership the closure can only execute while self is alive, so the guard is never tripped in practice.

Testing

# Cycle 1
xctestleaks agent run \\
  --workspace WordPress.xcworkspace --scheme WordPress \\
  --only-testing "WordPressTest/AnnouncementsDataStoreTests"

# Cycle 2
xctestleaks agent run \\
  --workspace WordPress.xcworkspace --scheme WordPress \\
  --only-testing "WordPressTest/GutenbergVideoUploadProcessorTests/testMediaTextBlockProcessor"

Both reported 0 leaks on this branch and the original leak counts on trunk.

Notes for reviewers

The first leak is in test file, I'm open to revert let me know how'd you feel about it. The second one is production leak and we can get that in.

amanjeetsingh150 and others added 2 commits May 9, 2026 04:05
Each of the four tests stores `subscription = store.onChange { … store.x … }`
in an instance property. The closure captures the local `store` strongly,
which is then retained by `CachedAnnouncementsStore.changeDispatcher`'s
observers dictionary, forming the cycle:

  store → changeDispatcher → observers[token] → closure → store

Without a `tearDown`, the `Receipt` only deinits when XCTest tears down the
test instance, which it defers when more tests are queued in the class —
so the cycle is observable by the end-of-test leak snapshot.

Adding `tearDown` to nil out `subscription` triggers the `Receipt.deinit`
unsubscribe path immediately and breaks the cycle.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The three `lazy var` properties (`videoHtmlProcessor`, `videoBlockProcessor`,
`mediaTextVideoBlockProcessor`) each construct a child processor whose
`replacer:` closure references `self` without a capture list. The child
processor is owned strongly by `self`, and the closure retained inside it
captures `self` strongly — producing three independent 2-node cycles:

  GutenbergVideoUploadProcessor
    → videoHtmlProcessor (HTMLProcessor)
      → replacer.context → captures self
        → GutenbergVideoUploadProcessor (cycle)

Switching each closure to `[weak self]` (with a `guard let self` early-out)
breaks the back-edge. By ownership, the closure is only invoked while
`self` is still alive, so the guard never trips in practice.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Retain cycles in AnnouncementsDataStoreTests and GutenbergVideoUploadProcessor (flagged by leak audit)

1 participant