Status Update
Comments
ma...@google.com <ma...@google.com>
an...@google.com <an...@google.com>
da...@gmail.com <da...@gmail.com> #2
Branch: androidx-main
commit 700259f0afe267dfe78b93db932a3cfd827a119d
Author: Sherry Hu <shuanghu@google.com>
Date: Mon May 10 14:23:09 2021
Add transition motion between fold and unfold.
Bug: 186211031
Test: manual
Change-Id: Id60f07311eca2d94ef91dc28ae45823a475160b4
M slidingpanelayout/slidingpanelayout/build.gradle
M slidingpanelayout/slidingpanelayout/src/androidTest/java/androidx/slidingpanelayout/widget/FoldTest.kt
M slidingpanelayout/slidingpanelayout/src/main/java/androidx/slidingpanelayout/widget/SlidingPaneLayout.java
ch...@google.com <ch...@google.com> #3
The reason this is happening is that,
BoxWithConstraints
creates a separate composition that is registered with theRecomposer
- When composition starts the
Recomposer
collects all invalidated compistions (in thie case, all composition that readvalue
) and schedules them to recompose. - It then recomposes all the recompositions with no regard to any dependency order (it is in order of registration which is loosly dependency order but not guaranteed to be).
- The primary composition detects that
BoxWithConstraints
should be removed and schedules it and the sub-composition to be disposed but the disposal doesn't occur untilapplyChanges()
- The sub-composition then recomposes with the
value
which violates the implied assumption.
We do not currently guarantee that the code invoked in a sub-composition of content that is removed from the parent composition prevents the contained sub-compositions from executing.
To fix this would require,
- Ensuring all composition is performed in strict dependency order (parent first, sub-compositions after parents).
- Communicate to the sub-compsoitions when they are to be disposed by the parent and ignore compositions in the round they are to be disposed.
As we didn't intended this constraint to hold I am changing this is not technically a bug. As the constraint seems reasonable and ensuring it holds does not seem too expensive nor would break currently working code, we should enforce this constraint and I am changing this to be a feature request and keeping it open.
ae...@gmail.com <ae...@gmail.com> #4
ol...@gmail.com <ol...@gmail.com> #5
This is a critical bug please fix as soon as possible! I get a lot of crashes because of this! This is not a Feature Request!
If crashes appear in your own code you can mitigate the issue by moving all crash-causing state reads outside of SubcomposeLayout
, e.g. my sample from the issue description would may look like this:
Column {
var value by remember { mutableStateOf(true) }
Button(
onClick = {
value = false
}
) {
Text("Change state")
}
if (value) {
val actualValue = value
BoxWithConstraints() {
assert(actualValue)
}
}
}
In this case the issue doesn't occur, because the state is read in the parent composition. Alternatively, you can extract the SubcomposeLayout into a separate function and pass values as parameters. It does the same trick:
Column {
var value by remember { mutableStateOf(true) }
Button(
onClick = {
value = false
}
) {
Text("Change state")
}
if (value) {
SomeLayout(value)
}
}
@Composable
private fun SomeLayout(value: Boolean) {
BoxWithConstraints() {
assert(value)
}
}
ae...@gmail.com <ae...@gmail.com> #6
Fatal Exception: java.lang.IllegalStateException: You cannot access the NavBackStackEntry's ViewModels after the NavBackStackEntry is destroyed.
at androidx.navigation.NavBackStackEntry.getViewModelStore(NavBackStackEntry.kt:213)
at com.google.accompanist.navigation.animation.AnimatedNavHostKt.AnimatedNavHost(AnimatedNavHost.kt:142)
at com.google.accompanist.navigation.animation.AnimatedNavHostKt.AnimatedNavHost(AnimatedNavHost.kt:89)
at com.myApp.screens.root.InnerNavigationKt.InnerNavigation(InnerNavigation.kt:79)
at com.myApp.mainContainer.MainScreenKt$MainScreen$7$1$1$3.invoke(MainScreen.kt:245)
at com.myApp.mainContainer.MainScreenKt$MainScreen$7$1$1$3.invoke(MainScreen.kt:244)
at androidx.compose.runtime.internal.ComposableLambdaImpl.invoke(ComposableLambda.jvm.kt:117)
at androidx.compose.runtime.internal.ComposableLambdaImpl$invoke$1.invoke(ComposableLambdaImpl.java:129)
at androidx.compose.runtime.internal.ComposableLambdaImpl$invoke$1.invoke(ComposableLambdaImpl.java:128)
at androidx.compose.runtime.RecomposeScopeImpl.compose(RecomposeScopeImpl.java:169)
at androidx.compose.runtime.ComposerImpl.recomposeToGroupEnd(Composer.kt:2469)
at androidx.compose.runtime.ComposerImpl.skipCurrentGroup(Composer.kt:2738)
at androidx.compose.runtime.ComposerImpl.doCompose(Composer.kt:3354)
at androidx.compose.runtime.ComposerImpl.recompose$runtime_release(Composer.kt:3304)
at androidx.compose.runtime.CompositionImpl.recompose(Composition.kt:775)
at androidx.compose.runtime.Recomposer.performRecompose(Recomposer.kt:1085)
at androidx.compose.runtime.Recomposer.access$performRecompose(Recomposer.kt:124)
at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$1.invoke(Recomposer.kt:566)
at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$1.invoke(Recomposer.kt:536)
at androidx.compose.ui.platform.AndroidUiFrameClock$withFrameNanos$2$callback$1.doFrame(AndroidUiFrameClock.android.kt:34)
at androidx.compose.ui.platform.AndroidUiDispatcher.performFrameDispatch(AndroidUiDispatcher.java:109)
at androidx.compose.ui.platform.AndroidUiDispatcher.access$performFrameDispatch(AndroidUiDispatcher.java:41)
at androidx.compose.ui.platform.AndroidUiDispatcher$dispatchCallback$1.doFrame(AndroidUiDispatcher.android.kt:69)
at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1008)
at android.view.Choreographer.doCallbacks(Choreographer.java:809)
at android.view.Choreographer.doFrame(Choreographer.java:740)
at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:995)
at android.os.Handler.handleCallback(Handler.java:938)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loop(Looper.java:246)
at android.app.ActivityThread.main(ActivityThread.java:8653)
at java.lang.reflect.Method.invoke(Method.java)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:602)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1130)
an...@gmail.com <an...@gmail.com> #7
Happens systematically on every user who has disabled animations from accessibility and causes apps to have a preoccupying number of crashes on multiple devices affecting the overall statics and perceived degraded user experiences.
Please fix as soon as possible. This is not a feature request, this is a bug.
jo...@gmail.com <jo...@gmail.com> #8
ae...@gmail.com <ae...@gmail.com> #9
This does not help unfortunately.
ol...@gmail.com <ol...@gmail.com> #10
I have nested NavHost and this is what happens in the inner one.
But does NavHost/AnimatedNavHost even use SubcomposeLayout? Are you sure it is related to this issue?
ae...@gmail.com <ae...@gmail.com> #11
Please see the duplicated issue:
as...@gmail.com <as...@gmail.com> #12
Please treat this as a critical bug.
Fatal Exception: java.lang.IllegalStateException: You cannot access the NavBackStackEntry's ViewModels after the NavBackStackEntry is destroyed.
at androidx.navigation.NavBackStackEntry.getViewModelStore(NavBackStackEntry.kt:202)
at androidx.navigation.compose.NavHostKt.NavHost(NavHost.kt:104)
at androidx.navigation.compose.NavHostKt.NavHost(NavHost.kt:67)
ch...@google.com <ch...@google.com> #13
Compose does not ensure repeatable read consistency across a sub-composition boundary (that is we do not guarantee that a mutable value read by the parent composition will be the same value read by the sub-composition).
However, as I noted above, we will be changing the runtime to avoid recomposing sub-compositions that have been detected in the parent composition as being removed, point (2) in
These two changes will change the rules so that the assumption how data dependencies are handled across sub-composition boundaries, as is implied by the assert
above in setContent
of a Composition
to be violated. When and why setContent
is called is in the hands to the code that created the composition, not the recomposer, or the rest of the runtime, which is why we cannot guarantee the assert assumption will always hold.
Theses changes will not ensure repeatable read consistency across a sub-composition boundary, but they will make it much easier for code to avoid requiring it by avoiding recomposing code that is being removed. Because repeatable read consistency is not guaranteed it is recommended that code not rely on it and avoid it by avoiding repeating reads as shown in the examples in
I am sorry if my calling this a feature request above implied a lack of priority. We are treating this change as a high priority change for the next feature release. As it requires an API change and a change to the rules of how composition works I think of it as a feature request but will leave it marked as a bug type to avoid confusion.
ap...@google.com <ap...@google.com> #14
Branch: androidx-main
commit 9f3a404d3117ed29b3ced1f2e29134237bf6baa1
Author: Ben Trengrove <bentrengrove@google.com>
Date: Thu Jun 01 15:56:39 2023
Skip recomposition of subcompositions that will be removed
Compose does not ensure repeatable read consistency across a sub-composition boundary (that is we do not guarantee that a mutable value read by the parent composition will be the same value read by the sub-composition). However in cases where we can detect that a subcomposition will be removed in the next applyChanges, we can skip recomposing that
subcomposition.
This is accomplished by keeping a map of removed compositions reported via the composition context.
When a composition is marked for deletion, it checks through its marked groups for any nested composition contexts. For each one it finds, reports its deletion to the recomposer via the context. When the nested composition is
recomposed, if the recomposers map of removed compositions contains the composition, it is skipped.
This change is an improvement on current behaviour but could be improved by ensuring parent compositions are always recomposed before any nested compositions. This mostly already occurs but making this enforced would further improve this situation and is why the linked bug is not yet considered fixed.
Bug: 254645321
Relnote: Skip recomposition of subcompositions that will be removed
Test: testSubcompositionDisposedInParent
Change-Id: Ieeb9919897a9f9b4274ddc77e66608a673cd1112
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composer.kt
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/CompositionContext.kt
M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Recomposer.kt
M compose/runtime/runtime/src/nonEmulatorCommonTest/kotlin/androidx/compose/runtime/RecomposerTests.kt
ch...@google.com <ch...@google.com> #15
As Ben and I found a relatively fast and safe way to fix this that didn't involve an API changes, we have started the process to cherry-pick this to earlier releases.
ri...@ffw.com <ri...@ffw.com> #16
Which version is this in / going to be in?
ch...@google.com <ch...@google.com> #17
This issue will be updated when it lands in a stable release.
ae...@gmail.com <ae...@gmail.com> #18
ch...@google.com <ch...@google.com> #19
I could not reproduce the crash above with 1.6.0-alpha01
The steps I took where:
- Created a new android project using
Android Studio Giraffe | 2022.3.1 RC 1
- Replaced
Greeting
with the code in #1 (fixing the imports) - Ran the program
- Pressed the button (crash reproduced)
- Added the line
implementation("androidx.compose.runtime:runtime:1.6.0-alpha01")
to the dependencies - Update the compile sdk to 34.
- Sync'ed gradle
- Ran the program
- Pressed the button (no crash)
ch...@google.com <ch...@google.com> #20
Note the above fix is only a partial fix. There are cases that will continue to violate the implied constraint until the second part of the fix lands (explained above).
Also note that this doesn't guarantee the constraint implied by the assert will always hold, only that the runtime will try to avoid recomposing sub-compositions that might be exposed to the constraint. Any use of sub-composition can violate this constraint as the value might change between the time the main composition composes and the sub-composition composes and before the main composition can be recomposed to remove the sub-composition.
If you require consistency between a composition and its subcomposition you need to capture the state as #5, above. This change just avoids a common case would violate this constraint.
Description
Jetpack Compose version: 1.2.1 (also tested on 1.3.0-rc01)
Jetpack Compose component(s) used: foundation, material
Here is the most simplified version of the code that led me to mysterious crashes when using ModalBottomSheetLayout:
When the button is clicked the code crashes, although you would not expect this just looking at the code. You would expect BoxWithConstraints to be removed from composition as soon as the
value
becomesfalse
.And if you replace BoxWithConstraints with regular Box it would work without assertion crash however.
If I understand correctly, the SubcomposeLayout causes its content to be recomposed without regards to the outer check. Which may be fine if you know about this behaviour and expect it. However in case of higher level composables such as ModalBottomSheetLayout it hides the underlying behaviour which may lead to similar unexpected crashes.
I don't know but to me it seems like a flaw. Please correct me if I'm just not doing something correctly.