Keeping default constructor unnecessarily keeps class name as well [151605338]

Fixed

Bug

Status Update

No update yet.

Description

st...@gmail.com

created issue #1

Mar 16, 2020 12:51PM

Component used: Fragment
Version used: androidx.fragment-fragment-1.2.2
Devices/Android versions reproduced on:
Either Android Gradle Plugin 3.5.3 or 3.6.0, build tools 28.0.3

Related (but not equivalent) issues:

https://issuetracker.google.com/issues/148963981

https://issuetracker.google.com/issues/149665169

Concerning the following consumer proguard rules we inherit:

# The default FragmentFactory creates Fragment instances using reflection
-if public class ** extends androidx.fragment.app.Fragment
-keep public class ** extends androidx.fragment.app.Fragment {
public <init>();
}

My understanding is that FragmentFactory requires R8 to be told that the default constructor to be kept, so it can be invoked via reflection. However I don't believe that requires the class' *name* to be kept, maybe I'm wrong here? A narrower rule that would meet the first requirement would be:

-keepclassmembers,allowobfuscation

Keeping the class name also keeps the package name the class is in, which also leads to greater portions an obfuscated app's internal directory structure appearing in clear text. (As a workaround we've often packaged Activities and Views separately and accepted that they're not well obfuscated, though it was a great convenience to us this was no longer the case with fragments). As a consequence, post-upgrade to 1.2.2 we notice many fragments and their directories appearing inside our app's APK, or by manual inspection of the mapping.txt

Comments

il...@google.com <il...@google.com> Mar 23, 2020 05:26PM

Assigned to il...@google.com.

an...@google.com <an...@google.com> #2Mar 24, 2020 12:10AM

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> #3Mar 24, 2020 04:53AM

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

commit 41c9e54ce4ab51f95d19a55fa92a56b6bd018c62
Author: Ian Lake <ilake@google.com>
Date: Mon Mar 23 16:07:32 2020

Allow obfuscation of kept Fragments

Fragments that are kept solely by class
can safely be obfuscated.

By switching to keepclasseswithmembers,
only classes with a default constructor
will qualify to be kept. This also allows
shrinking of classes that are constructed
through a custom FragmentFactory.

Test: tested in sample app at

bug 148181315

#comment4
BUG: 151605338
Change-Id: I68ea8779bce80f33f27658cf87a94ccede5376b0

M fragment/fragment/proguard-rules.pro

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

il...@google.com <il...@google.com> #4Mar 24, 2020 05:26PM

Marked as fixed.

While -keepclassmembers caused a regression in the original bug, we've added allowsObfuscation to allow even kept fragments to be obfuscated.

This will be available in the upcoming Fragment 1.2.4 and 1.3.0-alpha03.

sa...@peilicke.de <sa...@peilicke.de> #5Apr 14, 2020 06:16AM

The new release 1.2.4 now leads to a direct crash on app-start, i.e. when Android tries to inflate any Fragment referenced in the main navigation graph.

il...@google.com <il...@google.com> #6Apr 14, 2020 04:16PM

Re #5 - please file a new bug with a sample project that reproduces your issue.

[Deleted User] <[Deleted User]> #7Apr 24, 2020 08:19AM

This change in 1.2.4 also leads to ClassNotFoundException when a fragment is created by providing a name attribute to a FragmentContainerView if the referenced fragment class name is not explicitly kept and obfuscation is enabled:

<androidx.fragment.app.FragmentContainerView
                ...
                android:name="fullClassName"
                ... />

This, of course, is the expected behavior, but I think it is too much of an API-breaking change for a patch release.

il...@google.com <il...@google.com> #8Apr 24, 2020 04:39PM

Re #7 - please make sure you've followed the instructions in b/142601969 as otherwise no ProGuard rules are generated for FragmentContainerView referenced classes as per the Fragment 1.2.0 Known Issues.

If you're using Android Gradle Plugin 4.1 Canary 4 or higher and you're not getting your classes properly kept and non-obfuscated when using FragmentContainerView, please file a bug against AAPT.

sa...@peilicke.de <sa...@peilicke.de> #9May 1, 2020 03:12PM

Re #6: It's essentially https://issuetracker.google.com/issues/142601969 and indeed an API-breaking change. Unfortunately, there's no proper fix for AGP 3.6 nor 4.0, thus one can't update until those are stable. It's unfortunate as other AndroidX libraries start depending on 1.3.4 (such as preferences).

aa...@gmail.com <aa...@gmail.com> #10Sep 8, 2020 08:12PM

Thanks everyone