From 92e9d3a1279e2f50b79f9ee7b343eb1fc834ada0 Mon Sep 17 00:00:00 2001 From: Chris Smith Date: Fri, 12 May 2023 11:50:39 +0100 Subject: [PATCH] Fix a few FFI leaks (#405) Fix a few FFI leaks These are instances where we obtain an FFIObject and don't call Close on it to release the underlying reference on the Rust side. The worst instance here was leaking an object per room member every time we refreshed the member list --- .../assertions/assertRoomListSynced.yaml | 5 +++ .maestro/tests/roomList/searchRoomList.yaml | 1 + .../impl/root/CreateRoomRootPresenter.kt | 11 ++++--- .../invitelist/impl/InviteListPresenter.kt | 8 +++-- .../matrix/impl/room/RoomMemberMapper.kt | 19 ++++++----- .../matrix/impl/room/RustMatrixRoom.kt | 7 ++-- .../impl/timeline/MatrixTimelineItemMapper.kt | 4 +-- .../timeline/item/event/EventMessageMapper.kt | 8 ++--- .../item/event/EventTimelineItemMapper.kt | 24 +++++++------- .../item/event/TimelineEventContentMapper.kt | 6 ++-- .../android/samples/minimal/RoomListScreen.kt | 33 ++++++++----------- 11 files changed, 67 insertions(+), 59 deletions(-) create mode 100644 .maestro/tests/assertions/assertRoomListSynced.yaml diff --git a/.maestro/tests/assertions/assertRoomListSynced.yaml b/.maestro/tests/assertions/assertRoomListSynced.yaml new file mode 100644 index 0000000000..2d13c17df9 --- /dev/null +++ b/.maestro/tests/assertions/assertRoomListSynced.yaml @@ -0,0 +1,5 @@ +appId: ${APP_ID} +--- +- extendedWaitUntil: + visible: ${ROOM_NAME} + timeout: 10_000 diff --git a/.maestro/tests/roomList/searchRoomList.yaml b/.maestro/tests/roomList/searchRoomList.yaml index fe859defbb..45138eb9aa 100644 --- a/.maestro/tests/roomList/searchRoomList.yaml +++ b/.maestro/tests/roomList/searchRoomList.yaml @@ -1,5 +1,6 @@ appId: ${APP_ID} --- +- runFlow: ../assertions/assertRoomListSynced.yaml - tapOn: "search" - inputText: ${ROOM_NAME.substring(0, 3)} - takeScreenshot: build/maestro/400-SearchRoom diff --git a/features/createroom/impl/src/main/kotlin/io/element/android/features/createroom/impl/root/CreateRoomRootPresenter.kt b/features/createroom/impl/src/main/kotlin/io/element/android/features/createroom/impl/root/CreateRoomRootPresenter.kt index 60056872db..0ac9611fc7 100644 --- a/features/createroom/impl/src/main/kotlin/io/element/android/features/createroom/impl/root/CreateRoomRootPresenter.kt +++ b/features/createroom/impl/src/main/kotlin/io/element/android/features/createroom/impl/root/CreateRoomRootPresenter.kt @@ -65,11 +65,12 @@ class CreateRoomRootPresenter @Inject constructor( fun startDm(matrixUser: MatrixUser) { startDmAction.value = Async.Uninitialized - val existingDM = matrixClient.findDM(matrixUser.userId) - if (existingDM == null) { - localCoroutineScope.createDM(matrixUser, startDmAction) - } else { - startDmAction.value = Async.Success(existingDM.roomId) + matrixClient.findDM(matrixUser.userId).use { existingDM -> + if (existingDM == null) { + localCoroutineScope.createDM(matrixUser, startDmAction) + } else { + startDmAction.value = Async.Success(existingDM.roomId) + } } } diff --git a/features/invitelist/impl/src/main/kotlin/io/element/android/features/invitelist/impl/InviteListPresenter.kt b/features/invitelist/impl/src/main/kotlin/io/element/android/features/invitelist/impl/InviteListPresenter.kt index dc9302b908..db9fec3153 100644 --- a/features/invitelist/impl/src/main/kotlin/io/element/android/features/invitelist/impl/InviteListPresenter.kt +++ b/features/invitelist/impl/src/main/kotlin/io/element/android/features/invitelist/impl/InviteListPresenter.kt @@ -131,14 +131,18 @@ class InviteListPresenter @Inject constructor( private fun CoroutineScope.acceptInvite(roomId: RoomId, acceptedAction: MutableState>) = launch { suspend { - client.getRoom(roomId)?.acceptInvitation()?.getOrThrow() + client.getRoom(roomId)?.use { + it.acceptInvitation().getOrThrow() + } roomId }.execute(acceptedAction) } private fun CoroutineScope.declineInvite(roomId: RoomId, declinedAction: MutableState>) = launch { suspend { - client.getRoom(roomId)?.rejectInvitation()?.getOrThrow() ?: Unit + client.getRoom(roomId)?.use { + it.rejectInvitation().getOrThrow() + } ?: Unit }.execute(declinedAction) } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomMemberMapper.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomMemberMapper.kt index 2d2be4f36a..e79a8088aa 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomMemberMapper.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomMemberMapper.kt @@ -24,17 +24,18 @@ import org.matrix.rustcomponents.sdk.RoomMember as RustRoomMember object RoomMemberMapper { - fun map(roomMember: RustRoomMember): RoomMember = + fun map(roomMember: RustRoomMember): RoomMember = roomMember.use { RoomMember( - UserId(roomMember.userId()), - roomMember.displayName(), - roomMember.avatarUrl(), - mapMembership(roomMember.membership()), - roomMember.isNameAmbiguous(), - roomMember.powerLevel(), - roomMember.normalizedPowerLevel(), - roomMember.isIgnored(), + UserId(it.userId()), + it.displayName(), + it.avatarUrl(), + mapMembership(it.membership()), + it.isNameAmbiguous(), + it.powerLevel(), + it.normalizedPowerLevel(), + it.isIgnored(), ) + } fun mapMembership(membershipState: RustMembershipState): RoomMembershipState = when (membershipState) { diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt index 3930ebebe6..ae35449a8e 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt @@ -161,9 +161,10 @@ class RustMatrixRoom( override suspend fun sendMessage(message: String): Result = withContext(coroutineDispatchers.io) { val transactionId = genTransactionId() - val content = messageEventContentFromMarkdown(message) - runCatching { - innerRoom.send(content, transactionId) + messageEventContentFromMarkdown(message).use { content -> + runCatching { + innerRoom.send(content, transactionId) + } } } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/MatrixTimelineItemMapper.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/MatrixTimelineItemMapper.kt index 7151f3550b..5b4eaa9e36 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/MatrixTimelineItemMapper.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/MatrixTimelineItemMapper.kt @@ -27,12 +27,12 @@ class MatrixTimelineItemMapper( ) { fun map(timelineItem: TimelineItem): MatrixTimelineItem = timelineItem.use { - val asEvent = timelineItem.asEvent() + val asEvent = it.asEvent() if (asEvent != null) { val eventTimelineItem = eventTimelineItemMapper.map(asEvent) return MatrixTimelineItem.Event(eventTimelineItem) } - val asVirtual = timelineItem.asVirtual() + val asVirtual = it.asVirtual() if (asVirtual != null) { val virtualTimelineItem = virtualTimelineItemMapper.map(asVirtual) return MatrixTimelineItem.Virtual(virtualTimelineItem) diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/item/event/EventMessageMapper.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/item/event/EventMessageMapper.kt index a000783e86..674e02a13d 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/item/event/EventMessageMapper.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/item/event/EventMessageMapper.kt @@ -39,7 +39,7 @@ import org.matrix.rustcomponents.sdk.MessageFormat as RustMessageFormat class EventMessageMapper { fun map(message: Message): MessageContent = message.use { - val type = message.msgtype().use { type -> + val type = it.msgtype().use { type -> when (type) { is MessageType.Audio -> { AudioMessageType(type.content.body, type.content.source.useUrl(), type.content.info?.map()) @@ -68,9 +68,9 @@ class EventMessageMapper { } } MessageContent( - body = message.body(), - inReplyTo = message.inReplyTo()?.eventId?.let(::EventId), - isEdited = message.isEdited(), + body = it.body(), + inReplyTo = it.inReplyTo()?.eventId?.let(::EventId), + isEdited = it.isEdited(), type = type ) } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/item/event/EventTimelineItemMapper.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/item/event/EventTimelineItemMapper.kt index 3ec412ae7d..726b448099 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/item/event/EventTimelineItemMapper.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/item/event/EventTimelineItemMapper.kt @@ -31,18 +31,18 @@ class EventTimelineItemMapper(private val contentMapper: TimelineEventContentMap fun map(eventTimelineItem: RustEventTimelineItem): EventTimelineItem = eventTimelineItem.use { EventTimelineItem( - uniqueIdentifier = eventTimelineItem.uniqueIdentifier(), - eventId = eventTimelineItem.eventId()?.let { EventId(it) }, - isEditable = eventTimelineItem.isEditable(), - isLocal = eventTimelineItem.isLocal(), - isOwn = eventTimelineItem.isOwn(), - isRemote = eventTimelineItem.isRemote(), - localSendState = eventTimelineItem.localSendState()?.map(), - reactions = eventTimelineItem.reactions().map(), - sender = UserId(eventTimelineItem.sender()), - senderProfile = eventTimelineItem.senderProfile().map(), - timestamp = eventTimelineItem.timestamp().toLong(), - content = contentMapper.map(eventTimelineItem.content()) + uniqueIdentifier = it.uniqueIdentifier(), + eventId = it.eventId()?.let { EventId(it) }, + isEditable = it.isEditable(), + isLocal = it.isLocal(), + isOwn = it.isOwn(), + isRemote = it.isRemote(), + localSendState = it.localSendState()?.map(), + reactions = it.reactions().map(), + sender = UserId(it.sender()), + senderProfile = it.senderProfile().map(), + timestamp = it.timestamp().toLong(), + content = contentMapper.map(it.content()) ) } } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/item/event/TimelineEventContentMapper.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/item/event/TimelineEventContentMapper.kt index 51e84b441f..f776c52670 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/item/event/TimelineEventContentMapper.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/item/event/TimelineEventContentMapper.kt @@ -17,6 +17,7 @@ package io.element.android.libraries.matrix.impl.timeline.item.event import io.element.android.libraries.matrix.api.core.UserId +import io.element.android.libraries.matrix.api.timeline.item.event.EventContent import io.element.android.libraries.matrix.api.timeline.item.event.FailedToParseMessageLikeContent import io.element.android.libraries.matrix.api.timeline.item.event.FailedToParseStateContent import io.element.android.libraries.matrix.api.timeline.item.event.MembershipChange @@ -26,7 +27,6 @@ import io.element.android.libraries.matrix.api.timeline.item.event.RedactedConte import io.element.android.libraries.matrix.api.timeline.item.event.RoomMembershipContent import io.element.android.libraries.matrix.api.timeline.item.event.StateContent import io.element.android.libraries.matrix.api.timeline.item.event.StickerContent -import io.element.android.libraries.matrix.api.timeline.item.event.EventContent import io.element.android.libraries.matrix.api.timeline.item.event.UnableToDecryptContent import io.element.android.libraries.matrix.api.timeline.item.event.UnknownContent import io.element.android.libraries.matrix.impl.media.map @@ -39,7 +39,7 @@ import org.matrix.rustcomponents.sdk.OtherState as RustOtherState class TimelineEventContentMapper(private val eventMessageMapper: EventMessageMapper = EventMessageMapper()) { fun map(content: TimelineItemContent): EventContent = content.use { - when (val kind = content.kind()) { + when (val kind = it.kind()) { is TimelineItemContentKind.FailedToParseMessageLike -> { FailedToParseMessageLikeContent( eventType = kind.eventType, @@ -54,7 +54,7 @@ class TimelineEventContentMapper(private val eventMessageMapper: EventMessageMap ) } TimelineItemContentKind.Message -> { - val message = content.asMessage() + val message = it.asMessage() if (message == null) { UnknownContent } else { diff --git a/samples/minimal/src/main/kotlin/io/element/android/samples/minimal/RoomListScreen.kt b/samples/minimal/src/main/kotlin/io/element/android/samples/minimal/RoomListScreen.kt index df1f052e79..aab7d5ca5f 100644 --- a/samples/minimal/src/main/kotlin/io/element/android/samples/minimal/RoomListScreen.kt +++ b/samples/minimal/src/main/kotlin/io/element/android/samples/minimal/RoomListScreen.kt @@ -33,17 +33,16 @@ import io.element.android.libraries.dateformatter.impl.LocalDateTimeProvider import io.element.android.libraries.designsystem.utils.SnackbarDispatcher import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.core.RoomId -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.asCoroutineDispatcher import kotlinx.coroutines.launch +import kotlinx.coroutines.withContext import kotlinx.datetime.Clock import kotlinx.datetime.TimeZone import java.util.Locale -import java.util.concurrent.Executors class RoomListScreen( context: Context, private val matrixClient: MatrixClient, + private val coroutineDispatchers: CoroutineDispatchers = Singleton.coroutineDispatchers, ) { private val clock = Clock.System private val locale = Locale.getDefault() @@ -58,28 +57,24 @@ class RoomListScreen( sessionVerificationService = sessionVerificationService, networkMonitor = NetworkMonitorImpl(context), snackbarDispatcher = SnackbarDispatcher(), - inviteStateDataSource = DefaultInviteStateDataSource( - matrixClient, - DefaultSeenInvitesStore(context), - CoroutineDispatchers( - io = Dispatchers.IO, - computation = Dispatchers.Default, - main = Dispatchers.Main, - diffUpdateDispatcher = Executors.newSingleThreadExecutor().asCoroutineDispatcher() - ) - ) + inviteStateDataSource = DefaultInviteStateDataSource(matrixClient, DefaultSeenInvitesStore(context), coroutineDispatchers) ) @Composable fun Content(modifier: Modifier = Modifier) { fun onRoomClicked(roomId: RoomId) { - val room = matrixClient.getRoom(roomId)!! - val timeline = room.timeline() Singleton.appScope.launch { - timeline.apply { - initialize() - paginateBackwards(20, 50) - dispose() + withContext(coroutineDispatchers.io) { + matrixClient.getRoom(roomId)!!.use { room -> + val timeline = room.timeline() + + timeline.apply { + // TODO This doesn't work reliably as initialize is asynchronous, and the timeline can't be used until it's finished + initialize() + paginateBackwards(20, 50) + dispose() + } + } } } }