POS: Fix retain cycles related to payment onboarding view #15083

staskus · 2025-02-06T14:47:27Z

Part of: #14593

Note: One review is enough!

Description

#15055 PR review @jaclync noticed a memory leak after payment onboarding was presented.

It all pointed out to showSupport closure in PointOfSaleDashboardView:

woocommerce-ios/WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

Line 130 in 7e6c816

posModel.cancelCardPaymentsOnboarding()

This closure captures POSAggregateModel. It creates a strong reference cycle of:

POSAggregateModel -> CardPresentPaymentService -> CardPresentPaymentsOnboardingPresenterAdaptor -> CardPresentPaymentsOnboardingViewModel -> showSupport -> POSAggregateModel.

Unfortunately, simple solutions of applying weak to posModel didn't work. Other parts of the stack keep holding the onboarding model which holds the support closure that holds the posModel. Even when I:

Stopped holding onboarding model within CardPresentPaymentsOnboardingPresenterAdaptor
Set it to weak within PointOfSaleAggregateModel

Still, there remained a Combine publisher that kept holding the reference:

When I looked closely, it looked like the onboarding model was caught in a reference cycle:

I realized I spent hours looking at the wrong places. I looked at this observation logic multiple times, but my eyes haven't caught that we call handleEvents twice, and once strongly capturing self. 🤦

At this point, we must leave one strong reference. I think it makes sense for aggregate model to hold onto onboarding view model for the duration of the presentation.

Another Issue

Even after doing all this, POSAggregateModel was still held after closing POS. This fact made me discard a lot of the changes I made, thinking they didn't work.

It turns out that when we try to connect to the reader and onboarding is shown, the card reader connection task never finishes:

    func connectCardReader() {
        analytics.track(.pointOfSaleCardReaderConnectionTapped)
        Task { @MainActor in
            _ = try await cardPresentPaymentService.connectReader(using: .bluetooth)
        }
}

CardPresentPaymentPreflightAdaptor never returns the result from the attemptConnection call. Thankfully, putting [weak self] within a Task was enough for it not to hold onto the POSAggregateModel after POS was closed.

Steps to reproduce

Prerequisites:

Return stripeAccountOverdueRequirement(plugin: .wcPay) in CardPresentPaymentsOnboardingUseCase.checkOnboardingState

Open POS
Tap to connect to the card reader
Confirm onboarding view opens
Close it
Open it again
Confirm "Contact us" open support
Close POS
Repeat one or two times
Close POS
Open Memory Debugger
Confirm POSAggregateModel is released (or at least not held by support closure).

If POSAggregateModel is not released for you, could you:

Enable Edit Scheme -> WooCommerce -> Diagnostics -> Malloc Stack Logging
Reproduce memory leak again
In the memory debugger select File -> Export Memory Graph and share with me
Thanks!

Testing information

I tested on an iPad Pro 11-inch M4 18.2 simulator. A couple of times after closing POS, the aggregate model was still held by some SwiftUI related properties. However, it's worth waiting a bit or putting the POS in the background and returning it before opening the memory debugger. It worked for me and POSAggregateModel was released. I asked Apple engineer about it yesterday:

Depending on the lifecycle of underlying views and a specific situation, SwiftUI might release the objects not immediately. Among other factors, it may depend on UIKit, AppKit or other frameworks involved. You may notice that the objects owned by recently destroyed views get deallocated after an app goes to the background or when another similar view is constructed.

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

…closure captures POSAggregateModel

…if connection hasn't finished connectReader calls may not finish, for example, when onboarding is shown. If POS is closed, POSAggregateModel is not released. To solve this issue, capturing self weakly so POSAggregateModel would be released even if the Task hasn't finished

…ak-2

…entsOnboardingViewModel

…aymentOnboardingAdaptor A new instance is created when it's passed through a publisher and destroyed when no longer needed

wpmobilebot · 2025-02-06T15:28:00Z

📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS
	Build Number	pr15083-43c2942
	Version	21.6
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`43c2942`
	App Center Build	WooCommerce - Prototype Builds #12848

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

joshheald · 2025-02-06T15:56:14Z

Ooft, well done.

Have you tested this in the main app as well? The change in CardPresentPaymentsOnboardingViewModel will affect the IPP code (hopefully for the better!

staskus · 2025-02-06T16:52:09Z

Have you tested this in the main app as well? The change in CardPresentPaymentsOnboardingViewModel will affect the IPP code (hopefully for the better!

@joshheald thanks for reminding me. I will test IPP as well.

…ak-2

joshheald

Looks good, and a really good spot, though I know this will have been frustrating.

I had POSAggregateModel in memory 3 times after my repro, but none included show support. Backgrounding the app for a few minutes dropped that to 2 copies.

I think this is an improvement, well done.

joshheald · 2025-02-06T19:53:51Z

WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift

+        Task { @MainActor [weak self] in
+            _ = try await self?.cardPresentPaymentService.connectReader(using: .bluetooth)


Does this really form part of a retain cycle? We don't retain the Task, so I'm not sure why this is needed...

Tasks are not released until the asynchronous work inside it finishes.

As I tested, attemptConnection never finishes when onboarding is opened and closed. Task is not released until the connectReader finishes and it doesn't finish. Since Task retains self through accessing cardPresentPaymentService, self (PointOfSaleAggregateModel) remains retained.

Task.weak.self.720p.mov

Tasks are not released until the asynchronous work inside it finishes.

That's tricky to "visually" catch, as there are no explicit references to self. Thanks for the work on this and the explanation!

joshheald · 2025-02-06T19:55:08Z

...ewRelated/Dashboard/Settings/In-Person Payments/CardPresentPaymentsOnboardingViewModel.swift

+            .handleEvents(receiveOutput: { [weak self] state in
+                self?.trackState(state)
+            })


This is a great catch, it's not obvious at all that this captures self. I know it does, but it doesn't stand out.

staskus · 2025-02-06T20:36:24Z

@joshheald, thanks for testing!

I had POSAggregateModel in memory 3 times after my repro

It drives me crazy that I cannot reproduce it... Could you add me to the site you're using? Maybe a specific configuration hits some places in the code that mine doesn't.

joshheald · 2025-02-07T07:32:24Z

@staskus invited you

staskus added 6 commits February 6, 2025 15:35

Break a strong reference cycle where onboardingViewModel.showSupport …

5d4e4eb

…closure captures POSAggregateModel

Merge branch 'trunk' into fix/14593-fix-pos-aggregate-modal-memory-le…

42643d3

…ak-2

Remove a retain cycle within CardPresentPaymentsOnboardingViewModel

434f8fe

Break a reference cycle between POSAggregateModel and CardPresentPaym…

4b16658

…entsOnboardingViewModel

Don't hold CardPresentPaymentsOnboardingViewModel within CardPresentP…

5c39bca

…aymentOnboardingAdaptor A new instance is created when it's passed through a publisher and destroyed when no longer needed

staskus added type: bug A confirmed bug. feature: POS labels Feb 6, 2025

staskus added this to the 21.7 milestone Feb 6, 2025

staskus requested review from jaclync, joshheald and iamgabrielma February 6, 2025 14:47

joshheald self-assigned this Feb 6, 2025

Merge branch 'trunk' into fix/14593-fix-pos-aggregate-modal-memory-le…

43c2942

…ak-2

joshheald approved these changes Feb 6, 2025

View reviewed changes

staskus merged commit 68abe4b into trunk Feb 7, 2025
13 of 15 checks passed

staskus deleted the fix/14593-fix-pos-aggregate-modal-memory-leak-2 branch February 7, 2025 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POS: Fix retain cycles related to payment onboarding view #15083

POS: Fix retain cycles related to payment onboarding view #15083

staskus commented Feb 6, 2025 •

edited

Loading

wpmobilebot commented Feb 6, 2025 •

edited

Loading

joshheald commented Feb 6, 2025

staskus commented Feb 6, 2025

joshheald left a comment

joshheald Feb 6, 2025

staskus Feb 6, 2025

iamgabrielma Feb 7, 2025

joshheald Feb 6, 2025

staskus commented Feb 6, 2025

joshheald commented Feb 7, 2025

		Task { @MainActor [weak self] in
		_ = try await self?.cardPresentPaymentService.connectReader(using: .bluetooth)

POS: Fix retain cycles related to payment onboarding view #15083

POS: Fix retain cycles related to payment onboarding view #15083

Conversation

staskus commented Feb 6, 2025 • edited Loading

Description

Another Issue

Steps to reproduce

Testing information

Reviewer (or Author, in the case of optional code reviews):

wpmobilebot commented Feb 6, 2025 • edited Loading

joshheald commented Feb 6, 2025

staskus commented Feb 6, 2025

joshheald left a comment

Choose a reason for hiding this comment

joshheald Feb 6, 2025

Choose a reason for hiding this comment

staskus Feb 6, 2025

Choose a reason for hiding this comment

iamgabrielma Feb 7, 2025

Choose a reason for hiding this comment

joshheald Feb 6, 2025

Choose a reason for hiding this comment

staskus commented Feb 6, 2025

joshheald commented Feb 7, 2025

staskus commented Feb 6, 2025 •

edited

Loading

wpmobilebot commented Feb 6, 2025 •

edited

Loading