Bugfix FXIOS-9528 [TabManagerTests] Fix selectTab causing memory leak errors #21439
Conversation
…to avoid unit tests failing with memory leaks for the TabManagerTests.
Client.app: Coverage: 30.95
Generated by 🚫 Danger Swift against 532f12f |
| ensureMainThread { [weak self] in | ||
| self?.selectedTab?.createWebview(with: sessionData) | ||
| self?.selectedTab?.lastExecutedTime = Date.now() | ||
| } |
There was a problem hiding this comment.
This function is called within an async context and from my limited understanding on the topic you aren't supposed to do any manual thread management when in an async context. I'm not 100% certain but I think this is problematic.
@mattreaganmozilla mentioned he was going to do a deep dive on this topic so pinging him here for an interesting use case.
There was a problem hiding this comment.
Ahh good catch! This is definitely a unique case and I wonder if there's a better way to solve the memory leak.
More context for @mattreaganmozilla (writeup I made to discuss this further at mozweek) https://docs.google.com/document/d/1PTkMUMnRCpkC-TemmoQM_-ONPhmdwM_4n6wgeCBm9n0/edit
There was a problem hiding this comment.
I'm a bit confused on the practical impact this change (removing @MainActor and instead using ensureMainThread). @ih-codes is that something you can clarify further -- were you able to see what subtle differences there were between the two that makes one problematic and the other not?
I'm not 100% certain but I think this is problematic.
I'll dig into it a bit more but strictly speaking I don't know of any contracts in Swift concurrency that would disallow checking isMainThread and/or dispatching to the main thread from an async task. As an aside, I think my broader concern is just that I feel like our async code is becoming increasingly difficult to reason about due to how many different places we're operating on an arbitrary background thread and then executing something critical on the main thread, but sometimes without clear insight into how the timing of those individual operations will actually unfold. (That's a bigger topic though, not a criticism of this PR.)
There was a problem hiding this comment.
I agree with you @mattreaganmozilla. A couple of my test improvement PRs have been getting held up because there's still discussion to be had on how we want to write and test async code, and some of the bugs I fixed in my first month were related to race conditions from threading. If we're not happy with smaller "fixes" like this (strictly to improve unit tests), then the only alternatives seem to be A) adding sleeps to tests, B) a larger refactor of async code to be more testable. 😅 Hopefully something we can discuss at mozweek!
Anyway, to answer your first question -- this change is only to make it possible for the test_selectTab_doesNotRetain unit test to pass without retaining the TabManager. That google doc I linked above describes in more detail, but it boils down to the @MainActor annotation not allowing us to use weak self, which then retains the TabManager for one extra cycle, thus causing the test_selectTab_doesNotRetain unit test to fail due to a "memory leak".
There's not really a memory leak in our code, as it would be resolved in normal operation. So I'm not sure how much we want to change our code just to make unit tests pass.
Maybe most of our TabManager tests shouldn't also be checking for memory leaks in conjunction with functionality? 🤔 Then we could avoid sleep and rewriting for the time being.
|
This pull request has conflicts when rebasing. Could you fix it @ih-codes? 🙏 |
📜 Tickets
Jira ticket
Github issue
💡 Description
This PR fixes memory leak test failures for the
selectTabmethod.This is a watered down version of #21246 where I do not attempt to make any other test improvements.
The new
test_selectTab_doesNotRetaintest passes at 20,000 iterations without failure for me.Test Note:
To test really thoroughly, you sometimes need to use xcode right click > run repeatedly option to run individual test and/or TabManagerTests suite upwards of 5,000 times.
@cyndichin This helps, but does not fix, the
addTabsForURLsI'm afraid.📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120)