Keep rule for fragment is too broad [149665169]

Fixed

Bug

Status Update

No update yet.

Description

sg...@google.com

created issue #1

Feb 17, 2020 07:55AM

Component used: Fragment Version used: Master as of Feb 17 2020

Just noticed the Proguard rule which was added in ag/1216798 (and updated in ag/1227696), which looks like this:

-if public class ** extends androidx.fragment.app.Fragment
-keep public class ** extends androidx.fragment.app.Fragment {
    public <init>();
}

I think the ** in the conditional -keep should have been <1> to refer back to the class implementing androidx.fragment.app.Fragment (and then the extends part does not matter then). Otherwise if one class extends androidx.fragment.app.Fragment then the default constructor for all classes extending androidx.fragment.app.Fragment will be kept.

Like this:

-if public class ** extends androidx.fragment.app.Fragment
-keep public class <1> {
    public <init>();
}```

Comments

il...@google.com <il...@google.com> #2Feb 18, 2020 06:06PM

Yigit, do you have time to fix it?

reemission of the same liveData is racy

sg...@google.com <sg...@google.com> #3Feb 21, 2020 08:48AM

yea i'll take it.

ap...@google.com <ap...@google.com> #4Feb 25, 2020 06:11PM

Marked as fixed.

Thanks for the detailed analysis. This may not be an issue anymore since we've started using Main.immediate there but I' not sure; I'll try to create a test case.

il...@google.com <il...@google.com> #5Feb 25, 2020 06:27PM

just emitting same live data reproduces the issue.

@Test
fun raceTest() {
val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData)
emitSource(subLiveData) //crashes
}
subject.addObserver().apply {
testScope.advanceUntilIdle()
}
}

an...@google.com <an...@google.com> #6Mar 10, 2020 11:50PM

With 2.2.0-alpha04 (that use Main.immediate), the issue seems to be still there (I tested it by calling emitSource() twice, like your test case)

tb...@gmail.com <tb...@gmail.com> #7Mar 20, 2020 07:07PM

yea sorry immediate does not fix it.
I actually have a WIP fix for it:

https://android-review.googlesource.com/c/platform/frameworks/support/+/1112186

if your case is the one i found (emitting same LiveData multiple times, as shown in #5) you can work around it by adding a dummy transformation.

val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData.map {it })
emitSource(subLiveData.map {it} )
}

il...@google.com <il...@google.com> #8Mar 20, 2020 07:24PM

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

commit af12e75e6b4110f48e44ca121466943909de8f06
Author: Yigit Boyar <yboyar@google.com>
Date: Tue Sep 03 12:58:11 2019

Fix coroutine livedata race condition

This CL fixes a bug in liveData builder where emitting same
LiveData source twice would make it crash because the second
emission registry could possibly happen before first one is
removed as source.

We fix it by using a suspending dispose function. It does feel
a bit hacky but we cannot make DisposableHandle.dispose async
and we do not want to block there. This does not mean that there
is a problem if developer disposes it manually since our emit
functions take care of making sure it disposes (and there is
no other way to add source to the underlying MediatorLiveData)

Bug: 140249349
Test: BuildLiveDataTest#raceTest_*

Change-Id: I0b464c242a583da4669af195cf2504e2adc4de40

M lifecycle/lifecycle-livedata-ktx/api/2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/current.txt
M lifecycle/lifecycle-livedata-ktx/api/public_plus_experimental_2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/public_plus_experimental_current.txt
M lifecycle/lifecycle-livedata-ktx/api/restricted_2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/restricted_current.txt
M lifecycle/lifecycle-livedata-ktx/src/main/java/androidx/lifecycle/CoroutineLiveData.kt
M lifecycle/lifecycle-livedata-ktx/src/test/java/androidx/lifecycle/BuildLiveDataTest.kt

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

https://goto.google.com/android-sha1/af12e75e6b4110f48e44ca121466943909de8f06

dy...@gmail.com <dy...@gmail.com> #9Mar 22, 2020 09:04AM

What's the reason for having Proguard rules included with the Fragment library in the first place?

I'm asking because right now, this rule is actually preventing me from obfuscating, shrinking and optimizing my fragments for reasons I don't understand. One reason I could think of is reflection. Instead, is it possible to completely remove them and suggest developers to add these rules themselves?

il...@google.com <il...@google.com> #10Mar 23, 2020 05:31PM

Re #9 - please star b/151605338 for obfuscation. Fragment 1.2.3 (with this fix) already allows shrinking and the removal of unused Fragment classes.