Clean up unsafe NewApi calls and prevent future unsafe calls [141326133]

Fixed

Internal Cleanup

Status Update

No update yet.

Description

al...@google.com

created issue #1

Sep 19, 2019 09:30PM

We have a number of places where we make a call to a new API wrapped with an SDK_INT check; however, this is only safe if the actual method reference is within a different class such as a static inner class.

Any class that makes such a call will fail ART optimization and degrade overall performance.

All unsafe calls should be migrated into API level-specific static inner classes, e.g. the way BiometricManager handles API 29 calls in Api29Impl here:

https://cs.corp.google.com/aosp-androidx/biometric/src/main/java/androidx/biometric/BiometricManager.java?l=76

We will also need a way to prevent developers from making these types of mistakes in the future, such as a Lint rule.

Comments

al...@google.com <al...@google.com> #2Sep 19, 2019 09:30PM

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

ob...@google.com <ob...@google.com> Sep 19, 2019 09:47PM

Accepted by ob...@google.com.

al...@google.com <al...@google.com> #3Sep 24, 2019 01:49PM

Stretch goal: Write a Lint rule to detect unsafe calls *and* an associated auto-fix that conforms to the pattern already used in Jetpack.

ob...@google.com <ob...@google.com> #4Oct 28, 2019 04:34PM

Turns out that R8 might inline methods in separate classes to call site and then we're back to square one. The alternative is to have R8 do some constant checking (SDK_INT) and create separate classes accordingly as a workaround for previous SDK, and have ART check for that as a permanent workaround

ap...@google.com <ap...@google.com> #5Aug 21, 2020 03:34PM

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

commit d50d11e3a4e35caa033be78ed48bad10b5e660bf
Author: Oussama Ben Abdelbaki <obenabde@google.com>
Date: Tue Aug 18 11:47:44 2020

Add @DoNotInline annotation

The annotation annotates only methods and makes it such that the
method is never inlined during optimization but can still be
shrunk and obfuscated.

Relnote: Adding @DoNotInline annotation which instructs proguard to not inline the annotated method.
Test: None
Bug: 141326133
Change-Id: I3dfe8732c062e500b19d1cf2e3812389c7fc3cca

M annotation/annotation/api/current.txt
M annotation/annotation/api/public_plus_experimental_current.txt
M annotation/annotation/api/restricted_current.txt
A annotation/annotation/src/main/java/androidx/annotation/DoNotInline.java
M annotation/annotation/src/main/resources/META-INF/proguard/

androidx-annotations.pro

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

ap...@google.com <ap...@google.com> #6Sep 1, 2020 02:28PM

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

commit dd0931fe099fc9f344607bdcb04beae9949578f3
Author: Oussama Ben Abdelbaki <obenabde@google.com>
Date: Wed Aug 05 12:17:43 2020

Add new lint check to detect practices that could inhibit ART optimization

Bug contains context.

Bug: 141326133
Test: gw lintDebug
Change-Id: I1a4340e48ad11d8bc11e1f9c9be87ba7a6e9befa

A activity/activity/lint-baseline.xml
M appcompat/appcompat-resources/lint-baseline.xml
M appcompat/appcompat/lint-baseline.xml
M arch/core-runtime/lint-baseline.xml
A autofill/autofill/lint-baseline.xml
M benchmark/common/lint-baseline.xml
M biometric/biometric/lint-baseline.xml
A browser/browser/lint-baseline.xml
M buildSrc/src/main/kotlin/androidx/build/LintConfiguration.kt
M buildSrc/src/main/kotlin/androidx/build/dependencies/Dependencies.kt
A camera/camera-camera2-pipe/lint-baseline.xml
M camera/camera-camera2/lint-baseline.xml
M camera/camera-core/lint-baseline.xml
M camera/camera-testing/lint-baseline.xml
A camera/camera-view/lint-baseline.xml
M camera/integration-tests/extensionstestlib/lint-baseline.xml
D compose/runtime/runtime/lint-baseline.xml
M compose/test-utils/lint-baseline.xml
A compose/ui/ui-graphics/lint-baseline.xml
A compose/ui/ui-text-android/lint-baseline.xml
A compose/ui/ui-text/lint-baseline.xml
A compose/ui/ui/lint-baseline.xml
M contentpager/contentpager/lint-baseline.xml
M core/core-animation/lint-baseline.xml
A core/core-ktx/lint-baseline.xml
M core/core/lint-baseline.xml
A datastore/datastore-core/lint-baseline.xml
A documentfile/documentfile/lint-baseline.xml
M drawerlayout/drawerlayout/lint-baseline.xml
M dynamic-animation/dynamic-animation/lint-baseline.xml
M emoji/core/lint-baseline.xml
M exifinterface/exifinterface/lint-baseline.xml
A fragment/fragment/lint-baseline.xml
M leanback/leanback/lint-baseline.xml
A lifecycle/lifecycle-livedata-ktx/lint-baseline.xml
A lifecycle/lifecycle-process/lint-baseline.xml
M lint-checks/build.gradle
M lint-checks/src/main/java/androidx/build/lint/AndroidXIssueRegistry.kt
A lint-checks/src/main/java/androidx/build/lint/UnsafeNewApiCallsDetector.kt
M media/media/lint-baseline.xml
M media2/common/lint-baseline.xml
M media2/player/lint-baseline.xml
M media2/session/lint-baseline.xml
M media2/session/version-compat-tests/common/lint-baseline.xml
M media2/widget/lint-baseline.xml
M mediarouter/mediarouter/lint-baseline.xml
M preference/preference/lint-baseline.xml
A print/print/lint-baseline.xml
M room/guava/lint-baseline.xml
A room/ktx/lint-baseline.xml
M room/runtime/lint-baseline.xml
M security/crypto/lint-baseline.xml
M slices/builders/lint-baseline.xml
M slices/core/lint-baseline.xml
M slices/view/lint-baseline.xml
M sqlite/sqlite-framework/lint-baseline.xml
M sqlite/sqlite/lint-baseline.xml
M testutils/testutils-runtime/lint-baseline.xml
M textclassifier/textclassifier/lint-baseline.xml
A tracing/tracing/lint-baseline.xml
M transition/transition/lint-baseline.xml
M tv-provider/tv-provider/lint-baseline.xml
A ui/ui-graphics/lint-baseline.xml
A ui/ui-test/lint-baseline.xml
A ui/ui-text-android/lint-baseline.xml
A ui/ui-tooling/lint-baseline.xml
M vectordrawable/vectordrawable-animated/lint-baseline.xml
M versionedparcelable/versionedparcelable/lint-baseline.xml
M viewpager2/viewpager2/lint-baseline.xml
A window/window/lint-baseline.xml
M work/workmanager-testing/lint-baseline.xml
M work/workmanager/lint-baseline.xml

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

ap...@google.com <ap...@google.com> #7Sep 1, 2020 08:31PM

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

commit 69d3a1a7dc9a5c574728039180a1ca97a716c658
Author: Jeff Gaston <jeffrygaston@google.com>
Date: Tue Sep 01 12:23:14 2020

Spelling buf -> but

Bug: 141326133
Change-Id: Ie25a0db7320cd135aa6108a0a73f8dfbd7590779

M lint-checks/src/main/java/androidx/build/lint/UnsafeNewApiCallsDetector.kt

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

al...@google.com <al...@google.com> #8Oct 20, 2020 02:10PM

Relatedly, b/171298014 filed against API guidelines. We'll want to recommend @DoNotInline.

ob...@google.com <ob...@google.com> #9Oct 27, 2020 11:43AM

Alan, I think we said we could have some sort of event where library owners do this? Or we send an email urging owners to do this?

al...@google.com <al...@google.com> #10Oct 27, 2020 01:53PM

We've got a couple of other baseline clean-up tasks pending. At some point we'll run a Fix It by dumping all the baselines into a spreadsheet and automating the bug filing.

For now, we can send an email explaining (1) that we baselined violations across almost every library, (2) violations are known to lead to crashes on certain device versions, and (3) how to see where you violations are (ex. run the linter from Studio).

I can handle that if you've got other P1's to close out, just assign to me.

ob...@google.com <ob...@google.com> #11Nov 4, 2020 01:35PM

Assigned to al...@google.com.

Alan, since you probably know how to explain this better than I do, I might as well assign it to you now.

al...@google.com <al...@google.com> Apr 29, 2021 06:42PM

Accepted by al...@google.com.

al...@google.com <al...@google.com> #12May 20, 2021 03:07PM

Marked as fixed.

Bugs filed and discussion kicked off on how to best address them.

na...@google.com <na...@google.com> #13Sep 21, 2022 05:54PM

This bug was linked in a change in the following release(s):

androidx.drawerlayout:drawerlayout:1.2.0-alpha01