Discovering iOS memory leaks: A case study with Firefox app II

Context

After reading part 1 of this series, you might wonder why I didn't raise PR for firefox-ios, if I already found a fix to the leak. This is exactly what we are going to discuss in this part of my blog:

1. Raising the PR for firefox-ios.
2. Did the contribution get accepted? Short answer, yes 🎉. My fix is going in the v124 release.
3. Explaining the memory leak and its fix.
4. Sharing the experience on contributing to firefox-ios.

Contributing to firefox-ios

If you've read Part 1 you might anticipate that the fix is related to Deferred.swift aggregation function all as discussed here in the blog. Well, that was planned but things didn't quite go as I had hoped:

1. My initial approach was to submit a PR addressing two leaks associated with Deferred.swift. The solution involved refactoring the method to eliminate recursion that could potentially create a retain cycle.
2. Unfortunately, the PR wasn't merged. The Firefox team had plans to phase out Deferred.swift usage and do that with completion handlers instead, which is fair. You can see the discussion here.
3. Following their suggestion on the GitHub issue, I reached out to the team via ElementChat and got to know that we could just replace the implementation with DispatchGroup.
4. Before I start working on it, another contributor had already submitted a PR for it.

Reflecting on this experience, I've gathered some key takeaways if you are someone looking forward to contributing to such open-source projects:

1. Initiate by Reporting the Issue: Raise the Github issue first with all your findings.
2. Engage Before Contributing: Before investing time in a PR, communicate with the project team—typically found in the README—to understand their perspective and plans for the issue. Sometimes, they may prioritize different solutions or not consider the issue a priority.
3. Align and Seek Approval: Once there's a mutual understanding and you've received the go-ahead, feel free to proceed with your contribution. Communities like Mozilla genuinely appreciate and welcome efforts from external contributors.

This is exactly what I did for my "actual" fix, which was not related to Deferred implementation 😅

Interestingly, this is exactly what we practice while maintaining maestro but I didn't realize this until I was on the other side.

I would like to give a shoutout to lmarceau 🎉, who helped me in all the discussions and narrowing down on what I can fix.

Resolving UI kit retain cycle

The leak I finally ended up on working was a retain cycle between UI Kit references, I have also attached the full leak trace here in case you want to have a look.

11  com.apple.UIKitCore      -[UIView(AdditionalLayoutSupport) _updateConstraintsIfNeededCollectingViews:forSecondPass:] + 848
10  com.apple.UIKitCore      -[UIView(AdditionalLayoutSupport) _updateConstraintsIfNeededCollectingViews:forSecondPass:] + 1232
9   com.apple.UIKitCore      -[UIView(AdditionalLayoutSupport) _sendUpdateConstraintsIfNecessaryForSecondPass:] + 81
8   com.apple.UIKitCore      -[UIButton _needsDoubleUpdateConstraintsPass] + 138
7   com.apple.UIKitCore      -[UIButtonConfigurationVisualProvider hasMultilineText] + 27
6   com.apple.UIKitCore      -[UIButtonConfigurationVisualProvider updateConfigurationIfNecessary] + 229
5   org.mozilla.ios.Fennec      @objc SecondaryRoundedButton.updateConfiguration() + 24
4   org.mozilla.ios.Fennec   SecondaryRoundedButton.updateConfiguration() + 148
3   libswiftUIKit.dylib      UIButton.configuration.getter + 71
2   libswiftCore.dylib       swift_allocObject + 39
1   libswiftCore.dylib       swift_slowAlloc + 40
0   libsystem_malloc.dylib   _malloc_zone_malloc + 241 

Let's dive in on the code here and understand why this was a retain cycle:

// 1
guard var updatedConfiguration = configuration else {
   return
}

// 2
updatedConfiguration.titleTextAttributesTransformer = UIConfigurationTextAttributesTransformer { incoming in
     var container = incoming

     // 3
     container.foregroundColor = updatedConfiguration.baseForegroundColor
     container.font = DefaultDynamicFontHelper.preferredBoldFont(
          withTextStyle: .callout,
          size: UX.buttonFontSize
     )
     return container
 }

This code is part of updateConfiguration method of a custom UIButton implementation, called SecondaryRoundedButton. Let's break down the code here:

1. We have a variable updateConfiguration that holds the current configuration of UIButton
2. titleTextAttributesTransformer takes a closure that applies some properties on AttributedText when the button state changes. You can look at the documentation for details.
3. This is exactly where we have a retain cycle, this line assigns foreground color.

titleTextAttributesTransformer | Apple Developer Documentation

A block to update the attributed title when the button state changes.

Apple Developer Documentation

Why this is a retain cycle?

The UIButton class strongly holds its configuration and the closure block of titleTextAttributesTransformer. In line 3, when we update the color in the closure, we capture the updatedConfiguration making cycle, which is going to prevent UIButton from deinitializing altogether. Something like this:

Fix?

Just weakly capturing the foreground color did the trick here because we already have it declared. So changing the code to:

let transformer = UIConfigurationTextAttributesTransformer { [weak foregroundColor] incoming in
    var container = incoming

    // weakly capturing foreground color in closure                        
    container.foregroundColor = foregroundColor
    container.font = DefaultDynamicFontHelper.preferredBoldFont(
         withTextStyle: .callout,
         size: UX.buttonFontSize
    )
    return container
}
updatedConfiguration.titleTextAttributesTransformer = transformer

Conclusion

Finally, this fix got merged 🎉. You can check it out here:

Bugfix FXIOS-8456 [v124] Fix for retain cycle between UI kit references in SecondaryButton by amanjeetsingh150 · Pull Request #18751 · mozilla-mobile/firefox-ios

📜 Tickets Jira ticket Github issue 💡 Description The closure inside updateConfiguration implementation strongly captures updatedConfiguration which is the current UI configuration of the UIButton a…

GitHubmozilla-mobile

This will be released in the v124 version of firefox iOS soon. Shoutout to thatswinnie as well for reviewing the PR here 🙌

Did you find this useful and are interested in seeing more examples of fixing leaks? If yes, do share this and let me know, would love to write another part of this showing more examples of leaks found and their fixes. Reach out to me at @droid_singh and let me know.

Photo by Joe Zlomek on Unsplash