appcues / appcues-ios-sdk

The Appcues iOS SDK
https://www.appcues.com/mobile
MIT License
8 stars 2 forks source link

Fix memory leak when presenting an experience #527

Closed mmaatttt closed 5 months ago

mmaatttt commented 5 months ago

I was debugging the memory graph for some push stuff and noticed that our experience-related objects were sticking around after an experience completes. I verified this wasn't an sdk4 issue, so here we are.

This one was a bit confusing, and I honestly don't fully understand why it was an issue, but the ExperiencePackage owns the AppcuesExperiencePageMonitor, but then the page monitor observer list was holding a reference to the package/state machine side effect. We have weak package already which I'd have expected to take care of the problem, but it turns out that the call to SideEffect.handlePresentationError() was the culprit.

If I inlined the contents of handlePresentationError(), the retain cycle disappeared. And the standard weak self to try and do self?.handlePresentationError() doesn't work in this case because 'weak' may only be applied to class and class-bound protocol types, not 'ExperienceStateMachine.SideEffect' (an enum). So somehow the SideEffect is being captured, while simultaneously not being capture-able 🤷‍♂️. I also tried adding [weak handlePresentationError] but you get the same error since it tries to capture the function as a closure block.

Anyways, refactoring to move the helper to be a method on ExperienceStateMachine meant I didn't need to inline the function in two places, but now there's no retain cycle. Pretty simple fix.

I also noticed we don't have test cases for this error behaviour and I wanted to be sure I didn't break anything, so I added test cases first and verified they still work after the update.