MXRoomState/MXRoomMembers: Fix memory leak and copying #1040
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`MXRoomState` and `MXRoomMembers` held mutual strong references to each other via the `members` property and the `state` ivar, respectively. This led to a memory leak preventing both objects from being deallocated. Additionally, the leak was masqueraded by a bug in `MXRoomState`'s `copyWithZone:` method. The latter copied the `MXRoomMembers` object but didn't update its `state` ivar to point to the `MXRoomState` copy. As a result, copying an `MXRoomState` object did *not* introduce another leak. The present commit addresses both issues. First, the `state` ivar on `MXRoomMembers` is made weak. This seems safe because `MXRoomState` retains its `members` property and `MXRoomMembers` should not exist on its own without a room state owning it. Secondly, a new method `copyWithZone:andState:` is introduced on `MXRoomMembers`. The latter copies the receiver and updates the `state` ivar to the specified `MXRoomState`. Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
|
Thanks @Johennes ! |
manuroe
added a commit
that referenced
this pull request
Apr 14, 2021
element-hq/element-ios/issues/4204 Fix a regression introduced by #1040. While paginating, the kept MXRoomState instance could be released, creating a bad view of room members. Remove the bad dependency to MXRoomState to avoid any retain cycle.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MXRoomStateandMXRoomMemberscurrently hold mutual strong references to each other via themembersproperty and thestateivar, respectively. This leads to a memory leak preventing both objects from being deallocated. The leak is easy to reproduce in the Element iOS app. Just launch the app, use a few rooms and start the memory graph debugger. At this point you'll find orphaned objects retaining each other such as:Additionally, the leak is masqueraded by a bug in
MXRoomState'scopyWithZone:method. The latter copies theMXRoomMembersobject but doesn't update itsstateivar to point to theMXRoomStatecopy. As a result, copying anMXRoomStateobject does not introduce another leak. I've noticed this when writing a unit test for the leak. Instead of the retain cycle, there now is a chain whereMXRoomMembersis owned by one room state but points "back" to another.The present pull request addresses both issues. First, the
stateivar onMXRoomMembersis made weak. This seems safe becauseMXRoomStateretains itsmembersproperty andMXRoomMembersshould not exist on its own without a room state owning it.Secondly, a new method
copyWithZone:andState:is introduced onMXRoomMembers. The latter copies the receiver and updates thestateivar to the specifiedMXRoomState.Pull Request Checklist