Detect minified classes and skip instrumentation to avoid build problems #362
Conversation
| @Test | ||
| fun `tests that something happens`() { | ||
| """^\w(\\${'$'}(\w))*${'$'}""".toRegex().matches("a${'$'}a") | ||
| } |
There was a problem hiding this comment.
The test name here is not clear.
There was a problem hiding this comment.
Oops that was just there for me to figure out why the regex failed to match.
| @@ -23,6 +23,7 @@ class SentryPluginWithMinifiedLibsTest : | |||
| implementation 'com.google.android.gms:play-services-mlkit-text-recognition:18.0.0' | |||
| implementation 'com.adcolony:sdk:4.7.1' | |||
| implementation 'com.appboy:android-sdk-ui:19.0.0' | |||
| implementation files("testlib/stripe-problematic.jar") | |||
There was a problem hiding this comment.
Can we actually add a dependency directly to the stripe lib that is problematic?
| """^(([\w\${'$'}]\/[\w\${'$'}]{1,2})|([\w\${'$'}]{2}\/[\w\${'$'}]\/[\w\${'$'}]))(\/|${'$'})""".toRegex() | ||
| /* ktlint-enable max-line-length */ | ||
|
|
||
| fun classNameLooksMinified(simpleClassName: String, fullClassName: String): Boolean { |
There was a problem hiding this comment.
| fun classNameLooksMinified(simpleClassName: String, fullClassName: String): Boolean { | |
| private fun classNameLooksMinified(simpleClassName: String, fullClassName: String): Boolean { |
There was a problem hiding this comment.
Can't access it directly from test if I do that.
There was a problem hiding this comment.
Maybe we add the annotation that is only visible for the test then?
There was a problem hiding this comment.
I don't think they're available in SAGP (yet).
| /** | ||
| * Looks at the constant pool entries and searches for R8 markers | ||
| */ | ||
| internal fun ClassReader.isMinifiedClass(): Boolean { | ||
| val charBuffer = CharArray(maxStringLength) | ||
| return isR8Minified(this) || classNameLooksMinified(this.getSimpleClassName(), this.className) |
There was a problem hiding this comment.
Since there are many different ways of checking it now, I am wondering if we'd not have false positives.
There was a problem hiding this comment.
Do we call our logger in case isMinifiedClass returns true? Let's do it if not, so people know why something isn't being instrumented.
There was a problem hiding this comment.
Log statement is in place:
SentryPlugin.logger.info {
"$simpleClassName skipped from instrumentation because it's a minified class."
}
I guess it's better to skip instrumentation for too many classes rather than break the build. But yes, we'll surely have some false positives due to the regex.
|
Let's upgrade the Gradle and AGP testing matrix with the latest version to be sure that we are not breaking something new either. |
|
Created #363 for the build matrix. |
|
Thanks for digging into the dark side of bytecode man. @adinauer . |
adinauer
deleted the
feat/fix-build-with-file-instr-for-minified-dependencies
branch
📜 Description
Detect minified classes using a Regex and skip visiting it to avoid
ArrayIndexOutOfBoundsExceptiondue to emptydstFrame.inputStackArray inorg.objectweb.asm.Frame:1233.💡 Motivation and Context
Fixes #359 and #360
💚 How did you test it?
Local build and added tests including a minimal reproduction JAR
📝 Checklist
🔮 Next steps