Simplify bytecode instrumentation implementation

embrace-bytecode-instrumentation-tests/src/test/java/io/embrace/gradle/plugin/instrumentation/InstrumentedBytecodeTestCases.kt

@@ -32,19 +32,19 @@ import okhttp3.OkHttpClient
 import org.objectweb.asm.ClassVisitor
 
 private val onClickFactory: ClassVisitorFactory = { visitor ->
-    OnClickClassAdapter(ASM_API_VERSION, visitor) {}
+    OnClickClassAdapter(ASM_API_VERSION, visitor)
 }
 
 private val onLongClickFactory: ClassVisitorFactory = { visitor ->
-    OnLongClickClassAdapter(ASM_API_VERSION, visitor) {}
+    OnLongClickClassAdapter(ASM_API_VERSION, visitor)
 }
 
 private val webviewFactory: ClassVisitorFactory = { visitor ->
-    WebViewClientClassAdapter(ASM_API_VERSION, visitor) {}
+    WebViewClientClassAdapter(ASM_API_VERSION, visitor)
 }
 
 private val okHttpFactory: ClassVisitorFactory = { visitor ->
-    OkHttpClassAdapter(ASM_API_VERSION, visitor) {}
+    OkHttpClassAdapter(ASM_API_VERSION, visitor)
 }
 
 /**

embrace-gradle-plugin/src/main/java/io/embrace/android/gradle/plugin/instrumentation/EmbraceClassVisitorFactory.kt

@@ -3,7 +3,6 @@ package io.embrace.android.gradle.plugin.instrumentation
 import com.android.build.api.instrumentation.AsmClassVisitorFactory
 import com.android.build.api.instrumentation.ClassContext
 import com.android.build.api.instrumentation.ClassData
-import io.embrace.android.gradle.plugin.Logger
 import org.objectweb.asm.ClassVisitor
 
 /**
@@ -25,9 +24,7 @@ abstract class EmbraceClassVisitorFactory : AsmClassVisitorFactory<BytecodeInstr
             nextClassVisitor,
             instrumentationContext,
             parameters
-        ) {
-            logger.info(it())
-        }
+        )
     }
 
     override fun isInstrumentable(classData: ClassData): Boolean {
@@ -46,5 +43,3 @@ abstract class EmbraceClassVisitorFactory : AsmClassVisitorFactory<BytecodeInstr
         return true
     }
 }
-
-private val logger = Logger(EmbraceClassVisitorFactory::class.java)

embrace-gradle-plugin/src/main/java/io/embrace/android/gradle/plugin/instrumentation/EmbraceClassVisitorFactoryDelegate.kt

@@ -15,8 +15,7 @@ internal fun createClassVisitorImpl(
     classContext: ClassContext,
     nextClassVisitor: ClassVisitor,
     instrumentationContext: InstrumentationContext,
-    parameters: Property<BytecodeInstrumentationParams>,
-    logger: (() -> String) -> Unit,
+    parameters: Property<BytecodeInstrumentationParams>
 ): ClassVisitor {
     val api = instrumentationContext.apiVersion.get()
     var visitor = nextClassVisitor
@@ -34,25 +33,20 @@ internal fun createClassVisitorImpl(
     if (parameters.get().shouldInstrumentFirebaseMessaging.get() &&
         FirebaseMessagingServiceClassAdapter.accept(classContext)
     ) {
-        visitor = FirebaseMessagingServiceClassAdapter(api, visitor, logger)
-        logger { "Added FirebaseMessagingServiceClassAdapter for $className." }
+        visitor = FirebaseMessagingServiceClassAdapter(api, visitor)
     }
 
     if (parameters.get().shouldInstrumentWebview.get() && WebViewClientClassAdapter.accept(classContext)) {
-        visitor = WebViewClientClassAdapter(api, visitor, logger)
-        logger { "Added WebViewClientClassAdapter for $className." }
+        visitor = WebViewClientClassAdapter(api, visitor)
     }
     if (parameters.get().shouldInstrumentOkHttp.get() && OkHttpClassAdapter.accept(classContext)) {
-        visitor = OkHttpClassAdapter(api, visitor, logger)
-        logger { "Added OkHttpClassAdapter for $className." }
+        visitor = OkHttpClassAdapter(api, visitor)
     }
     if (parameters.get().shouldInstrumentOnLongClick.get() && OnLongClickClassAdapter.accept(classContext)) {
-        visitor = OnLongClickClassAdapter(api, visitor, logger)
-        logger { "Added OnLongClickClassAdapter for $className." }
+        visitor = OnLongClickClassAdapter(api, visitor)
     }
     if (parameters.get().shouldInstrumentOnClick.get() && OnClickClassAdapter.accept(classContext)) {
-        visitor = OnClickClassAdapter(api, visitor, logger)
-        logger { "Added OnClickClassAdapter for $className." }
+        visitor = OnClickClassAdapter(api, visitor)
     }
     return visitor
 }

embrace-gradle-plugin/src/main/java/io/embrace/android/gradle/plugin/instrumentation/visitor/BytecodeMethodInsertionParams.kt

@@ -0,0 +1,33 @@
+package io.embrace.android.gradle.plugin.instrumentation.visitor
+
+/**
+ * The parameters that should be used to insert a function call into the body of a method.
+ */
+internal data class BytecodeMethodInsertionParams(
+
+    /**
+     * The fully qualified class name containing the method to be instrumented.
+     */
+    val owner: String,
+
+    /**
+     * The function name that will be instrumented.
+     */
+    val name: String,
+
+    /**
+     * The type signature of the function to be instrumented.
+     */
+    val descriptor: String,
+
+    /**
+     * The starting index of the local variable on the operand stack. We assume that by default the bytecode
+     * entrypoint will take the current object as its first parameter, then all other parameters.
+     *
+     * For example, an instrumentation entrypoint for a View.OnClickListener would have the following
+     * signature: instrumentedMethodName(android.view.View.OnClickListener thiz, android.view.View view).
+     *
+     * In future we can revisit this decision, given that the current object is not usually used.
+     */
+    val startVarIndex: Int = 0,
+)

embrace-gradle-plugin/src/main/java/io/embrace/android/gradle/plugin/instrumentation/visitor/FirebaseMessagingServiceClassAdapter.kt

@@ -11,34 +11,38 @@ import org.objectweb.asm.MethodVisitor
 class FirebaseMessagingServiceClassAdapter(
     api: Int,
     internal val nextClassVisitor: ClassVisitor?,
-    private val logger: (() -> String) -> Unit
 ) : ClassVisitor(api, nextClassVisitor) {
 
     companion object : ClassVisitFilter {
         private const val CLASS_NAME = "com.google.firebase.messaging.FirebaseMessagingService"
         private const val METHOD_NAME = "onMessageReceived"
         private const val METHOD_DESC = "(Lcom/google/firebase/messaging/RemoteMessage;)V"
 
-        @Suppress("UnstableApiUsage")
         override fun accept(classContext: ClassContext): Boolean {
-            if (classContext.currentClassData.superClasses.contains(CLASS_NAME)) {
-                return true
-            }
-            return false
+            return classContext.currentClassData.superClasses.contains(CLASS_NAME)
         }
     }
+
     override fun visitMethod(
         access: Int,
         name: String,
         desc: String,
         signature: String?,
-        exceptions: Array<String>?
+        exceptions: Array<String>?,
     ): MethodVisitor? {
         val nextMethodVisitor = super.visitMethod(access, name, desc, signature, exceptions)
 
         return if (METHOD_NAME == name && METHOD_DESC == desc) {
-            logger { "FirebaseMessagingServiceClassAdapter: instrumented method $name $desc" }
-            FirebaseMessagingServiceMethodAdapter(api, nextMethodVisitor)
+            InstrumentationTargetMethodVisitor(
+                api = api,
+                methodVisitor = nextMethodVisitor,
+                params = BytecodeMethodInsertionParams(
+                    owner = "io/embrace/android/embracesdk/fcm/swazzle/callback/com/android/fcm/FirebaseSwazzledHooks",
+                    name = "_onMessageReceived",
+                    descriptor = "(Lcom/google/firebase/messaging/RemoteMessage;)V",
+                    startVarIndex = 1,
+                )
+            )
         } else {
             nextMethodVisitor
         }

embrace-gradle-plugin/src/main/java/io/embrace/android/gradle/plugin/instrumentation/visitor/InstrumentationTargetMethodVisitor.kt

@@ -0,0 +1,38 @@
+package io.embrace.android.gradle.plugin.instrumentation.visitor
+
+import org.objectweb.asm.MethodVisitor
+import org.objectweb.asm.Opcodes
+import org.objectweb.asm.Type
+
+/**
+ * Visits a method that should be rewritten to include an API call to instrumentation
+ * and inserts a call to a static method at the very start.
+ */
+internal class InstrumentationTargetMethodVisitor(
+    api: Int,
+    methodVisitor: MethodVisitor?,
+    private val params: BytecodeMethodInsertionParams,
+) : MethodVisitor(api, methodVisitor) {
+
+    override fun visitCode() {
+        // count how many parameters are in the method descriptor
+        val paramCount = Type.getArgumentTypes(params.descriptor).size
+
+        // load local variables and push onto the operand stack
+        repeat(paramCount) {
+            visitVarInsn(Opcodes.ALOAD, it + params.startVarIndex)
+        }
+
+        // invoke the target method
+        visitMethodInsn(
+            Opcodes.INVOKESTATIC,
+            params.owner,
+            params.name,
+            params.descriptor,
+            false
+        )
+
+        // call super last to reduce chance of interference with other bytecode instrumentation
+        super.visitCode()
+    }
+}

embrace-gradle-plugin/src/main/java/io/embrace/android/gradle/plugin/instrumentation/visitor/OkHttpClassAdapter.kt

@@ -10,7 +10,6 @@ import org.objectweb.asm.MethodVisitor
 class OkHttpClassAdapter(
     api: Int,
     internal val nextClassVisitor: ClassVisitor?,
-    private val logger: (() -> String) -> Unit
 ) : ClassVisitor(api, nextClassVisitor) {
 
     companion object : ClassVisitFilter {
@@ -28,13 +27,20 @@ class OkHttpClassAdapter(
         name: String,
         desc: String,
         signature: String?,
-        exceptions: Array<String>?
+        exceptions: Array<String>?,
     ): MethodVisitor? {
         val nextMethodVisitor = super.visitMethod(access, name, desc, signature, exceptions)
 
         return if (METHOD_NAME_BUILD == name && METHOD_DESC_BUILD == desc) {
-            logger { "OkHttpClassAdapter: instrumented method $name $desc" }
-            OkHttpMethodAdapter(api, nextMethodVisitor)
+            InstrumentationTargetMethodVisitor(
+                api = api,
+                methodVisitor = nextMethodVisitor,
+                params = BytecodeMethodInsertionParams(
+                    owner = "io/embrace/android/embracesdk/okhttp3/swazzle/callback/okhttp3/OkHttpClient\$Builder",
+                    name = "_preBuild",
+                    descriptor = "(Lokhttp3/OkHttpClient\$Builder;)V",
+                )
+            )
         } else {
             nextMethodVisitor
         }

embrace-gradle-plugin/src/main/java/io/embrace/android/gradle/plugin/instrumentation/visitor/OnClickClassAdapter.kt

@@ -11,7 +11,6 @@ import org.objectweb.asm.MethodVisitor
 class OnClickClassAdapter(
     api: Int,
     internal val nextClassVisitor: ClassVisitor?,
-    private val logger: (() -> String) -> Unit
 ) : ClassVisitor(api, nextClassVisitor) {
 
     companion object : ClassVisitFilter {
@@ -26,15 +25,21 @@ class OnClickClassAdapter(
         name: String,
         desc: String,
         signature: String?,
-        exceptions: Array<String>?
+        exceptions: Array<String>?,
     ): MethodVisitor? {
         val nextMethodVisitor = super.visitMethod(access, name, desc, signature, exceptions)
 
         return if (METHOD_NAME == name && METHOD_DESC == desc && !isStatic(access)) {
-            logger { "OnClickClassAdapter: instrumented method $name $desc" }
-            OnClickMethodAdapter(api, nextMethodVisitor)
+            InstrumentationTargetMethodVisitor(
+                api = api,
+                methodVisitor = nextMethodVisitor,
+                params = BytecodeMethodInsertionParams(
+                    owner = "io/embrace/android/embracesdk/ViewSwazzledHooks\$OnClickListener",
+                    name = "_preOnClick",
+                    descriptor = "(Landroid/view/View\$OnClickListener;Landroid/view/View;)V",
+                )
+            )
         } else if (METHOD_DESC == desc && isStatic(access) && isSynthetic(access)) {
-            logger { "OnClickClassAdapter: instrumented synthetic method $name $desc" }
             OnClickStaticMethodAdapter(api, nextMethodVisitor)
         } else {
             nextMethodVisitor