Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix manage tiles screen crash if there are tiles, updating tiles #2568

Merged
merged 3 commits into from
Jun 1, 2022

Conversation

jpelgrom
Copy link
Member

@jpelgrom jpelgrom commented May 31, 2022

Summary

#2540 introduced two potential crashes when opening the 'Manage Tiles' screen if there are any tiles stored in the database and a bug for updating existing tiles. I guess everyone tested a fresh install and didn't press back?

1: A crash because we're trying to update state used by Compose while it is a snapshot on the IO thread. Fixed by switching to the main thread when updating these variables.

java.lang.IllegalStateException: Reading a state that was created after the snapshot was taken or in a snapshot that has not yet been applied
2022-05-31 19:20:29.420 11315-11371/io.homeassistant.companion.android.debug E/AndroidRuntime: FATAL EXCEPTION: DefaultDispatcher-worker-2
    Process: io.homeassistant.companion.android.debug, PID: 11315
    java.lang.IllegalStateException: Reading a state that was created after the snapshot was taken or in a snapshot that has not yet been applied
        at androidx.compose.runtime.snapshots.SnapshotKt.readError(Snapshot.kt:1826)
        at androidx.compose.runtime.snapshots.SnapshotKt.current(Snapshot.kt:2070)
        at androidx.compose.runtime.SnapshotMutableStateImpl.setValue(SnapshotState.kt:299)
        at io.homeassistant.companion.android.settings.qs.ManageTilesViewModel.setTileLabel(ManageTilesViewModel.kt:111)
        at io.homeassistant.companion.android.settings.qs.ManageTilesViewModel.updateExistingTileFields(ManageTilesViewModel.kt:81)
        at io.homeassistant.companion.android.settings.qs.ManageTilesViewModel.access$updateExistingTileFields(ManageTilesViewModel.kt:27)
        at io.homeassistant.companion.android.settings.qs.ManageTilesViewModel$selectTile$1.invokeSuspend(ManageTilesViewModel.kt:71)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
        at kotlinx.coroutines.internal.LimitedDispatcher.run(LimitedDispatcher.kt:42)
        at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:95)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:749)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664)
    	Suppressed: kotlinx.coroutines.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@b4b2720, Dispatchers.IO]

2: A crash because we launch multiple suspending functions in the init block, which can cause a race condition for a lateinit variable. Fixed by checking if it is initialized before using it.

kotlin.UninitializedPropertyAccessException: lateinit property iconPack has not been initialized
2022-05-31 19:56:23.937 15603-15603/io.homeassistant.companion.android.debug E/AndroidRuntime: FATAL EXCEPTION: main
    Process: io.homeassistant.companion.android.debug, PID: 15603
    kotlin.UninitializedPropertyAccessException: lateinit property iconPack has not been initialized
        at io.homeassistant.companion.android.settings.qs.ManageTilesViewModel.getIconPack(ManageTilesViewModel.kt:34)
        at io.homeassistant.companion.android.settings.qs.ManageTilesViewModel$updateExistingTileFields$2.invokeSuspend(ManageTilesViewModel.kt:86)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
        at kotlinx.coroutines.internal.LimitedDispatcher.run(LimitedDispatcher.kt:42)
        at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:95)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:749)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664)
    	Suppressed: kotlinx.coroutines.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@cc32aa3, Dispatchers.Main.immediate]

3: When the id of a tile is set to 0, Room will autogenerate a new ID and the existing data in the database is never updated. Fixed by correctly reading and re-using the ID of the database entry like before. (id != tileId, I'm not entirely sure why we have two IDs and that's likely why this mistake was made)

Screenshots

n/a

Link to pull request in Documentation repository

n/a

Any other notes

cc @NotWoods

@jpelgrom jpelgrom changed the title Fix crash when opening manage tiles screen if there are tiles Fix manage tiles screen crash if there are tiles, updating tiles May 31, 2022
@JBassett JBassett merged commit d95ab88 into home-assistant:master Jun 1, 2022
@jpelgrom jpelgrom deleted the fix-2540 branch June 1, 2022 15:38
@dshokouhi
Copy link
Member

hmm I still see the same "Reading a state" error here

@jpelgrom
Copy link
Member Author

jpelgrom commented Jun 1, 2022

That's unexpected, is it still crashing when trying to update tileLabel or on another field?

@dshokouhi
Copy link
Member

That's unexpected, is it still crashing when trying to update tileLabel or on another field?

Yup looks to be tile label

2022-06-01 09:02:52.869: java.lang.IllegalStateException: Reading a state that was created after the snapshot was taken or in a snapshot that has not yet been applied
	at androidx.compose.runtime.snapshots.SnapshotKt.readError(Snapshot.kt:1826)
	at androidx.compose.runtime.snapshots.SnapshotKt.current(Snapshot.kt:2070)
	at androidx.compose.runtime.SnapshotMutableStateImpl.setValue(SnapshotState.kt:299)
	at io.homeassistant.companion.android.settings.qs.ManageTilesViewModel.setTileLabel(ManageTilesViewModel.kt:111)
	at io.homeassistant.companion.android.settings.qs.ManageTilesViewModel.updateExistingTileFields(ManageTilesViewModel.kt:81)
	at io.homeassistant.companion.android.settings.qs.ManageTilesViewModel.access$updateExistingTileFields(ManageTilesViewModel.kt:27)
	at io.homeassistant.companion.android.settings.qs.ManageTilesViewModel$selectTile$1.invokeSuspend(ManageTilesViewModel.kt:71)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
	at kotlinx.coroutines.internal.LimitedDispatcher.run(LimitedDispatcher.kt:42)
	at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:95)
	at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:749)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664)
	Suppressed: kotlinx.coroutines.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@d22d30f, Dispatchers.IO]

@jpelgrom
Copy link
Member Author

jpelgrom commented Jun 1, 2022

The tile label is now set on line 91 instead of 81, are you sure that the app has been updated?

@dshokouhi
Copy link
Member

The tile label is now set on line 91 instead of 81, are you sure that the app has been updated?

ah firebase app got the best of me, usualy the releases are listed in numerical version code order but these last 3 are not. 🤦

indeed the fix works, sorry about that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants