Proguard rules improvements #1043
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1043 +/- ##
=======================================
Coverage 79.74% 79.75%
=======================================
Files 416 416
Lines 10775 10775
Branches 1763 1763
=======================================
+ Hits 8593 8594 +1
Misses 1399 1399
+ Partials 783 782 -1
|
priettt
force-pushed
the
proguard-improvements
branch
from
570195f to
ea42a75
Compare
| ## Keep classes used from native code | ||
| -keep class io.embrace.android.embracesdk.payload.NativeThreadAnrSample { *; } | ||
| -keep class io.embrace.android.embracesdk.payload.NativeThreadAnrStackframe { *; } | ||
| -keep class io.embrace.android.embracesdk.anr.sigquit.SigquitDataSource { *; } |
There was a problem hiding this comment.
Do we also need to keep NativeThreadSamplerNdkDelegate, or will that work ok with R8's obfuscation? It's probably worth global-searching for any external fun in the project & checking they work.
There was a problem hiding this comment.
I verified that they work, but I'm not sure if it's because they're referenced by kept classes or if it's because obfuscation doesn't affect JNI calls. I'll check just in case because I haven't tested every external fun.
The classes referenced from the native code must be kept because they're searched for by name.
There was a problem hiding this comment.
The classes are always being kept.
Not because we are actually adding any rules for them, but I think it's because they are being initialized in modules that override kotlin.jvm.functions.Function0.invoke(), and those are also called in different places in ComponentActivity, and every activity of any app is added because of some aapt_rules.
It's super weird, and I don't want to lose much time on this, so I'll just add rules for the classes that contain external fun methods just in case 😅
| @@ -1,20 +1,33 @@ | |||
| -keepattributes Exceptions, InnerClasses, Signature, LineNumberTable, SourceFile | |||
There was a problem hiding this comment.
What's the effect of removing the rule to keep these attributes? Instinctively I'd have thought we would want to retain information about inner classes/exception metadata
There was a problem hiding this comment.
We only need LineNumberTable and SourceFile.
Exceptionskeeps info on what exceptions a method might throw (thethrowspart of a function signature). We don't use it.Signaturekeeps generics type information. If not present, instead of it'll use Object. We don't use it.InnerClasseskeeps the relationship between classes. If it's not present, inner classes might be renamed and their relationship with their outer class might be lost. This doesn't affects us per se, but some stacktraces might change. Keeping it wouldn't be a problem, but I don't think it's necessary.
priettt
force-pushed
the
proguard-improvements
branch
from
ba72d93 to
ecb4666
Compare
…ses, Signature We only need to keep LineNumberTable and SourceFile for showing crash stacktraces.
priettt
force-pushed
the
proguard-improvements
branch
from
ecb4666 to
41c2c63
Compare
Goal
Customers using R8 to obfuscate and shrink their app weren't able to shrink Embrace, because we had proguard rules instructing to keep all classes. This PR changes that.
In
android-test-app, turning this on reduced Embrace size by 70%, comparing against master (from 800kb to 250kb, it's not like we were too heavy in the first place 😅)Remove unnecessary rules:
Removed keep rule for the whole embrace package
Added rules to keep classes used by hosted SDKs, native code, okhttp, swazzled and serialized classes.
Testing
Tested in these apps:
Ran swazzler tests