NPE when trying to record drawContent() [389046242]

Fixed

Bug

Status Update

No update yet.

Description

lp...@google.com

created issue #1

Jan 10, 2025 01:21PM

Some more details in b/388353336

Start of trace:

 java.lang.NullPointerException	
        at androidx.compose.ui.node.LayoutNodeDrawScope.drawContent(LayoutNodeDrawScope.kt:46)
	at androidx.compose.foundation.draganddrop.CacheDrawScopeDragShadowCallback$cachePicture$1$1$1.invoke(AndroidDragAndDropSource.android.kt:55)
	at androidx.compose.foundation.draganddrop.CacheDrawScopeDragShadowCallback$cachePicture$1$1$1.invoke(AndroidDragAndDropSource.android.kt:55)
	at androidx.compose.ui.draw.CacheDrawScope$record$1.invoke(DrawModifier.kt:347)
	at androidx.compose.ui.draw.CacheDrawScope$record$1.invoke(DrawModifier.kt:338)
	at androidx.compose.ui.graphics.layer.GraphicsLayer.draw$ui_graphics_release(AndroidGraphicsLayer.android.kt:1078)

This happens because when we draw we use a shared ContentDrawScope implementation that we render all DrawModifierNodes in. We do this by mutating a property as we draw successive nodes. However, this means that if anyone captures a reference to this draw scope and tries to draw it later on, this will cause a NPE error as we don't know what the 'current node' that needs to be drawn in.

For the general case this is an error and scopes shouldn't be captured and stored outside the scope in this way, but for GraphicsLayer this is needed when recording drawContent to support some edge cases, such as when we need to draw a layer implementation that can't be drawn in software, within software rendering (such as drawing to a Bitmap). For this we just end up re-invoking the draw block we captured.

We considered some solutions where we would allocate a new draw scope instance per modifier, but this adds some overhead and 'storing' these draw scopes for future use is challenging. (we considered putting them on the node object, in a map on the node coordinator, or some other chain structure on the node coordinator, but none of these are really great)

Instead we can explicitly support the GraphicsLayer.record case by overriding the DrawScope#GraphicsLayer.record function and making it handle this case properly. This can still cause issues if using the non-DrawScope scoped record function, but developers should move to the DrawScope one instead.

Comments

ap...@google.com <ap...@google.com> #2Jan 13, 2025 02:35PM

Marked as fixed.

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

ap...@google.com <ap...@google.com> #3Jan 14, 2025 12:58PM

Project: platform/frameworks/support
Branch: androidx-main
Author: Louis Pullen-Freilich <lpf@google.com>
Link: https://android-review.googlesource.com/3450530

Revert^2 "Fixes issue with dragAndDropSource preventing child invalidations"

Expand for full commit details

Revert^2 "Fixes issue with dragAndDropSource preventing child invalidations" 
 
This reverts commit 3d5fd7d04bdd7f02ee2b215af68b2832e06134bf. 
 
Reason for revert: Underlying bug b/389046242 is now resolved 
 
Fixes: b/379682458 
Test: DragAndDropSourceTest 
Test: AndroidDragAndDropIntegrationTest 
 
Change-Id: I07cb2a4091b46d862628bf17625ae9d9eb087641

Files:

A compose/foundation/foundation/src/androidInstrumentedTest/kotlin/androidx/compose/foundation/draganddrop/DragAndDropSourceTest.kt
M compose/foundation/foundation/src/androidMain/kotlin/androidx/compose/foundation/draganddrop/AndroidDragAndDropSource.android.kt

Hash: 3c48a0fbd3c2902a6a7acb6c8b474a4928b1be41
Date: Mon Jan 13 09:46:38 2025

na...@google.com <na...@google.com> #4Jan 29, 2025 06:12PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.compose.foundation:foundation:1.8.0-beta01
androidx.compose.foundation:foundation-android:1.8.0-beta01
androidx.compose.foundation:foundation-jvmstubs:1.8.0-beta01
androidx.compose.foundation:foundation-linuxx64stubs:1.8.0-beta01
androidx.compose.ui:ui-android:1.8.0-beta01
androidx.compose.ui:ui-jvmstubs:1.8.0-beta01
androidx.compose.ui:ui-linuxx64stubs:1.8.0-beta01

Issue 389046242

Description

Issue summary

Comments

ap...@google.com <ap...@google.com> #2Jan 13, 2025 02:35PM

ap...@google.com <ap...@google.com> #3Jan 14, 2025 12:58PM

na...@google.com <na...@google.com> #4Jan 29, 2025 06:12PM

Add comment

Issue metadata