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

iOS fix a crash when quickly dismissing a modal #1384

Merged
merged 3 commits into from
Jun 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,6 @@ val IosBugs = Screen.Selection(
ComposeAndNativeScroll,
MeasureAndLayoutCrash,
AnimationFreezeBug,
ModalMemoryLeak
ModalMemoryLeak,
ModalCrash
Copy link
Member

@MatkovIvan MatkovIvan Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add unit test instead another page in sample app

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed to do it later

)
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
/*
* Copyright 2024 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package androidx.compose.mpp.demo.bugs

import androidx.compose.foundation.background
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.Spacer
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.requiredWidth
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.wrapContentHeight
import androidx.compose.foundation.shape.RoundedCornerShape
import androidx.compose.material.Button
import androidx.compose.material.CircularProgressIndicator
import androidx.compose.material.MaterialTheme
import androidx.compose.material.Text
import androidx.compose.mpp.demo.Screen
import androidx.compose.runtime.Composable
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCoroutineScope
import androidx.compose.runtime.setValue
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.unit.dp
import androidx.compose.ui.window.Popup
import kotlin.time.Duration.Companion.milliseconds
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch

val ModalCrash = Screen.Example("ModalCrash") {
MaterialTheme {
var showContent by remember { mutableStateOf(false) }

if (showContent) {
LoadingHud()
}

val coroutineScope = rememberCoroutineScope()
Column(Modifier.fillMaxWidth(), horizontalAlignment = Alignment.CenterHorizontally) {
Button(onClick = {
showContent = true
coroutineScope.launch {
// no crash when delay is 50ms or more
delay(10.milliseconds)
showContent = false
}
}) {
Text("Click me!")
}
}
}
}

@Composable
fun LoadingHud() {
Popup {
Box(
modifier = Modifier.fillMaxSize(),
contentAlignment = Alignment.Center,
) {
Column(
modifier = Modifier
.requiredWidth(150.dp)
.wrapContentHeight()
.background(
color = Color.Gray.copy(alpha = 0.7f),
shape = RoundedCornerShape(12.dp)
)
.padding(horizontal = 16.dp, vertical = 24.dp),
horizontalAlignment = Alignment.CenterHorizontally,
) {
CircularProgressIndicator(modifier = Modifier.size(48.dp).padding(8.dp))
Spacer(Modifier.height(8.dp))
Text("Popup")
}

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ internal abstract class BaseComposeScene(

private var isInvalidationDisabled = false
private inline fun <T> postponeInvalidation(traceTag: String, crossinline block: () -> T): T = trace(traceTag) {
check(!isClosed) { "ComposeScene is closed" }
check(!isClosed) { "postponeInvalidation called after ComposeScene is closed" }
isInvalidationDisabled = true
return try {
// Try to get see the up-to-date state before running block
Expand Down Expand Up @@ -129,7 +129,7 @@ internal abstract class BaseComposeScene(
override fun hasInvalidations(): Boolean = hasPendingDraws || recomposer.hasPendingWork

override fun setContent(content: @Composable () -> Unit) = postponeInvalidation("BaseComposeScene:setContent") {
check(!isClosed) { "ComposeScene is closed" }
check(!isClosed) { "setContent called after ComposeScene is closed" }
inputHandler.onChangeContent()

/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,21 +124,21 @@ private class MultiLayerComposeSceneImpl(

override var density: Density = density
set(value) {
check(!isClosed) { "ComposeScene is closed" }
check(!isClosed) { "density set after ComposeScene is closed" }
field = value
mainOwner.density = value
}

override var layoutDirection: LayoutDirection = layoutDirection
set(value) {
check(!isClosed) { "ComposeScene is closed" }
check(!isClosed) { "layoutDirection set after ComposeScene is closed" }
field = value
mainOwner.layoutDirection = value
}

override var size: IntSize? = size
set(value) {
check(!isClosed) { "ComposeScene is closed" }
check(!isClosed) { "size set after ComposeScene is closed" }
check(value == null || (value.width >= 0f && value.height >= 0)) {
"Size of ComposeScene cannot be negative"
}
Expand Down Expand Up @@ -196,12 +196,12 @@ private class MultiLayerComposeSceneImpl(
}

override fun calculateContentSize(): IntSize {
check(!isClosed) { "ComposeScene is closed" }
check(!isClosed) { "calculateContentSize called after ComposeScene is closed" }
return mainOwner.measureInConstraints(Constraints())
}

override fun invalidatePositionInWindow() {
check(!isClosed) { "ComposeScene is closed" }
check(!isClosed) { "invalidatePositionInWindow called after ComposeScene is closed" }
mainOwner.invalidatePositionInWindow()
}

Expand Down Expand Up @@ -420,7 +420,7 @@ private class MultiLayerComposeSceneImpl(
}

private fun attachLayer(layer: AttachedComposeSceneLayer) {
check(!isClosed) { "ComposeScene is closed" }
check(!isClosed) { "attachLayer called after ComposeScene is closed" }
layers.add(layer)

if (layer.focusable) {
Expand All @@ -433,7 +433,7 @@ private class MultiLayerComposeSceneImpl(
}

private fun detachLayer(layer: AttachedComposeSceneLayer) {
check(!isClosed) { "ComposeScene is closed" }
check(!isClosed) { "detachLayer called after ComposeScene is closed" }
layers.remove(layer)

releaseFocus(layer)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,21 +102,21 @@ private class SingleLayerComposeSceneImpl(

override var density: Density = density
set(value) {
check(!isClosed) { "ComposeScene is closed" }
check(!isClosed) { "density set after ComposeScene is closed" }
field = value
mainOwner.density = value
}

override var layoutDirection: LayoutDirection = layoutDirection
set(value) {
check(!isClosed) { "ComposeScene is closed" }
check(!isClosed) { "layoutDirection set after ComposeScene is closed" }
field = value
mainOwner.layoutDirection = value
}

override var size: IntSize? = size
set(value) {
check(!isClosed) { "ComposeScene is closed" }
check(!isClosed) { "size set after ComposeScene is closed" }
check(value == null || (value.width >= 0f && value.height >= 0)) {
"Size of ComposeScene cannot be negative"
}
Expand All @@ -132,19 +132,19 @@ private class SingleLayerComposeSceneImpl(
}

override fun close() {
check(!isClosed) { "ComposeScene is already closed" }
check(!isClosed) { "close called after ComposeScene is already closed" }
onOwnerRemoved(mainOwner)
mainOwner.dispose()
super.close()
}

override fun calculateContentSize(): IntSize {
check(!isClosed) { "ComposeScene is closed" }
check(!isClosed) { "calculateContentSize called after ComposeScene is closed" }
return mainOwner.measureInConstraints(Constraints())
}

override fun invalidatePositionInWindow() {
check(!isClosed) { "ComposeScene is closed" }
check(!isClosed) { "invalidatePositionInWindow called after ComposeScene is closed" }
mainOwner.invalidatePositionInWindow()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ internal class ComposeSceneMediator(
uiKitTextInputService.stopInput()
applicationForegroundStateListener.dispose()
focusStack?.popUntilNext(renderingView)
keyboardManager.stop()
keyboardManager.dispose()
renderingView.dispose()
interactionView.dispose()
rootView.removeFromSuperview()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ internal class ComposeSceneKeyboardOffsetManager(
private val composeSceneMediatorProvider: () -> ComposeSceneMediator?,
private val onComposeSceneOffsetChanged: (Double) -> Unit,
) : KeyboardVisibilityObserver {
private var isDisposed: Boolean = false

val view get() = viewProvider()

Expand All @@ -75,6 +76,12 @@ internal class ComposeSceneKeyboardOffsetManager(
KeyboardVisibilityListener.removeObserver(this)
}

fun dispose() {
check (!isDisposed) { "ComposeSceneKeyboardOffsetManager is already disposed" }
isDisposed = true
stop()
}

/**
* Invisible view to track system keyboard animation
*/
Expand Down Expand Up @@ -263,6 +270,11 @@ internal class ComposeSceneKeyboardOffsetManager(
private var viewBottomOffset: Double = 0.0
set(newValue) {
field = newValue
onComposeSceneOffsetChanged(newValue)

// In certain edge cases the scene might be disposed before updateAnimationValues is called
// Simply don't forward the offset change in this case to avoid calling anything on closed ComposeScene.
if (!isDisposed) {
onComposeSceneOffsetChanged(newValue)
}
}
}
Loading