Crash in PointerInputFilter when used inside AdapterList [157998762]

Fixed

Bug

Status Update

No update yet.

Description

an...@google.com

created issue #1

Jun 2, 2020 05:35PM

I reproduced one annoying bug, and I also saw that a lot of people facing something similar in slack, and some other channels.
Basically what happens is when you have some postponed loading like asynchronously loading the image inside an item of AdapterList and you scroll sometimes it crashes and you there is no workaround.
It is not stable reproducible but I come up with a simple repro app(and you can see the logs here as well):

https://paste.googleplex.com/6333037623115776. we need to start scrolling in different directions and apparently it will crash.
What happens as I understand:
In PointerInputEventProcessor you first collect all PointerInputFilters which were hit(for example in our case is and AdapterList itself and the clickable item inside it)
Then inside AdapterList onDrag callback we synchronously subcompose which means that the AdapterList's item could recompose and apply a new modifier(for example update the background color).
And then we execute dispatchChanges on a modifier which was already detached.
We just discussed it during the sync, but seems like the simplest workaround is to add inside Node.dispatchChanges() something like

if (!pointerInputFilter.isAttached) {
return false
}

Comments

an...@google.com <an...@google.com> #2Jun 3, 2020 03:59PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

sh...@google.com <sh...@google.com> Jun 3, 2020 10:55PM

Accepted by sh...@google.com.

sh...@google.com <sh...@google.com> #3Jun 4, 2020 10:35PM

Ah, it looks like it is actually easily producible. Seems that whenever you are touching something that is scrolled out of view in the AdapterList, it will crash! I think your suggested fix is likely enough. I will get it in ASAP so the crashing stops and then will follow up with a closer look!

ap...@google.com <ap...@google.com> #4Jun 9, 2020 09:43PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-master-dev

commit 60699735a001203befb054cc7e30067d3203291c
Author: Shep Shapard <shepshapard@google.com>
Date: Wed Jun 03 15:52:22 2020

Handling the removal of PointerInputFilters synchronously.

Subcomposition can happen during the dispatch of pointer
input events. When this occurred, it used to crash. Now it
doesn't.

Relnote: "Fixed issue where pointer input dispatch would
cause a crash if PointerInputFilters were removed via
subcomposition during disptach. This is now fixed."

Fixes: 157998762
Test: ./gradlew ui:ui-core:testDebugUnitTest --tests=androidx.ui.core.pointerinput.HitPathTrackerTest

Change-Id: Iab398032792a8dde761ce5440650c774b4d56022

M ui/ui-core/integration-tests/ui-core-demos/src/main/java/androidx/ui/core/demos/CoreDemos.kt
A ui/ui-core/integration-tests/ui-core-demos/src/main/java/androidx/ui/core/demos/gestures/SurviveThroughSubComp.kt
M ui/ui-core/src/main/java/androidx/ui/core/pointerinput/HitPathTracker.kt
M ui/ui-core/src/test/java/androidx/ui/core/pointerinput/HitPathTrackerTest.kt

https://android-review.googlesource.com/1324303

ap...@google.com <ap...@google.com> #5Jun 10, 2020 06:22PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit 59345217ef602060cd33f383381e84e97a3c1449
Author: Shep Shapard <shepshapard@google.com>
Date: Wed Jun 03 15:52:22 2020

Handle removing PointerInputFilters synchronously

Note: This change was landed previously by
aosp/1324303 but then reverted via aosp/1327417.
This is simply a small rebase and relanding.

Subcomposition can happen during the dispatch of
pointer input events. When this occurred, it
used to crash. Now it doesn't.

Relnote: "Fixed issue where pointer input
dispatch would cause a crash if
PointerInputFilters were removed via
subcomposition during disptach. This is now fixed."

Fixes: 157998762
Test: ./gradlew ui:ui-core:testDebugUnitTest --tests=androidx.ui.core.pointerinput.HitPathTrackerTest
Change-Id: I1f48bdee2fcb59eb0984c31ef4ce95d75417dcf4

M ui/ui-core/integration-tests/ui-core-demos/src/main/java/androidx/ui/core/demos/CoreDemos.kt
A ui/ui-core/integration-tests/ui-core-demos/src/main/java/androidx/ui/core/demos/gestures/PointerInputDuringSubCompDemo.kt
M ui/ui-core/src/main/java/androidx/ui/core/pointerinput/HitPathTracker.kt
M ui/ui-core/src/test/java/androidx/ui/core/pointerinput/HitPathTrackerTest.kt

https://android-review.googlesource.com/1329256

Issue 157998762