feat(instrumentation): Add binder IPC call tracing#1159
Conversation
…acer adapter presence The SentryIpcTracer adapter class is introduced in sentry-java 8.40.0 (getsentry/sentry-java#5326). Bump VERSION_BINDER_IPC so the instrumentation no-ops when the adapter is not present on the classpath. Adds unit test coverage for the registry lookup and the ASM visitor (tracer wrapping, static vs instance handling, non-binder pass-through). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md # plugin-build/src/main/kotlin/io/sentry/android/gradle/AndroidComponentsConfig.kt
|
SentryBinderAdapter.onCallStart now returns a nullable Object token instead of an int cookie, and onCallEnd accepts that token. Update the emitted bytecode descriptors accordingly and pin the full method descriptors in the visitor tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
to avoid any confusion, let's remove IPC and stick with "Binder" everywhere
…flag Rewrite the static/instance opcode guard in BinderMethodVisitor as an explicit check and document why it is required, extract the duplicated onCallEnd emission, and add comments explaining the try/finally rewrite. Rename binderIpcEnabled to binderEnabled and enable the isSentryClass guard in Binder. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c99a18d. Configure here.
| sentryModules.isAtLeast(SentryModules.SENTRY_ANDROID_CORE, SentryVersions.VERSION_APP_START) && | ||
| parameters.appStartEnabled.get() | ||
|
|
||
| fun isBinderInstrEnabled(): Boolean = parameters.binderEnabled.get() |
There was a problem hiding this comment.
Missing SDK version gate
High Severity
isBinderInstrEnabled() only checks the Gradle binder.enabled flag (default true) and never verifies sentry-android-core is at least the version that ships SentryBinderAdapter. With the flag on and an older SDK, the transform still injects SentryBinderAdapter calls and the app can fail at runtime when those paths run.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c99a18d. Configure here.
| sentryModules.isAtLeast(SentryModules.SENTRY_ANDROID_CORE, SentryVersions.VERSION_APP_START) && | ||
| parameters.appStartEnabled.get() | ||
|
|
||
| fun isBinderInstrEnabled(): Boolean = parameters.binderEnabled.get() |
There was a problem hiding this comment.
Bug: The isBinderInstrEnabled() function is missing a version check, causing runtime crashes (NoClassDefFoundError) for users with older sentry-android-core versions when binder instrumentation is enabled by default.
Severity: CRITICAL
Suggested Fix
Update the isBinderInstrEnabled() function to include a version check for sentry-android-core. The check should verify that the SDK version is at least VERSION_BINDER (8.40.0) before enabling instrumentation, similar to other instrumentation checks in the file. The implementation should be sentryModules.isAtLeast(SENTRY_ANDROID_CORE, VERSION_BINDER) && parameters.binderEnabled.get().
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
plugin-build/src/main/kotlin/io/sentry/android/gradle/services/SentryModulesService.kt#L123
Potential issue: The function `isBinderInstrEnabled()` only checks if the feature is
enabled by the user, but it omits a crucial version check for the `sentry-android-core`
dependency. All other similar instrumentation features in the codebase include a version
guard. Because this check is missing, if a project uses an older version of
`sentry-android-core` (specifically, a version below 8.40.0 which does not include the
`SentryBinderAdapter` class), the build plugin will still inject bytecode that calls
this missing class. This will result in a `NoClassDefFoundError` at runtime, causing the
application to crash whenever an instrumented Binder call is executed.
Did we get this right? 👍 / 👎 to inform future reviews.
0xadam-brown
left a comment
There was a problem hiding this comment.
Thanks @markushi – looks excellent! 🥇
Just the missing SDK version gate + TODOs that are essential. A few additional comments on my end, but no other blockers.
| open class BinderExtension @Inject constructor(objects: ObjectFactory) { | ||
| /** | ||
| * Enables or disables Binder IPC call instrumentation. Defaults to true. This requires | ||
| * sentry-android-core version TODO or above, and needs to be enabled. See |
There was a problem hiding this comment.
h: TODO (same below)
(though I'm sure they're on your radar)
| open class BinderExtension @Inject constructor(objects: ObjectFactory) { | ||
| /** | ||
| * Enables or disables Binder IPC call instrumentation. Defaults to true. This requires | ||
| * sentry-android-core version TODO or above, and needs to be enabled. See |
There was a problem hiding this comment.
l: Should we remove the "and needs to be enabled" since we enable it by default, or is that phrase referring to something else?
| } | ||
|
|
||
| override fun isInstrumentable(data: ClassContext) = !data.isSentryClass() | ||
| } |
There was a problem hiding this comment.
m: Any concerns about impact on build time?
Our hash map lookups are fast, and I see we also perform the same scan for logging, etc., so the effect will be incremental for anyone who hasn't disabled the defaults. Not a blocker, just making sure we considered it + curious to hear how we think about these sorts of scans (eg, okay to add indefinitely b/c cost is marginal vs something we weigh carefully each time out).
|
|
||
| data class BinderMethodSpec( | ||
| val owner: String, | ||
| val name: String, |
There was a problem hiding this comment.
l: name -> method[Name]
(I found myself making this translation as I read through the diff. Up to you, and ignore if ASM always uses name, of course.)
|
|
||
| // region content & package management | ||
|
|
||
| specs.addAll( |
There was a problem hiding this comment.
Amazing work gathering all of these!
Do we have a maintenance strategy going forward, by chance? Could be worth considering a Warden job, etc. at some point focused on hard-to-cover maintenance gaps.
| // is LIFO, so arguments are popped in reverse order. | ||
| val argTypes = Method(name, descriptor).argumentTypes | ||
| val argLocals = IntArray(argTypes.size) | ||
| for (i in argLocals.size - 1 downTo 0) { |
There was a problem hiding this comment.
super nit if you want it: argsLocal.size - 1 -> argsLocals.lastIndex
| sentryModules.isAtLeast(SentryModules.SENTRY_ANDROID_CORE, SentryVersions.VERSION_APP_START) && | ||
| parameters.appStartEnabled.get() | ||
|
|
||
| fun isBinderInstrEnabled(): Boolean = parameters.binderEnabled.get() |
| fun `lookup returns spec for known instance binder method`() { | ||
| val spec = BinderMethodRegistry.lookup("android/content/ContentResolver", "query") | ||
| assertNotNull(spec) | ||
| assertEquals("ContentResolver", spec.component) |
There was a problem hiding this comment.
l: Worth asserting on entire spec content? ie, also name and owner
(same below, esp covering subtypes separately, which would benefit most)
| val text = disassemble(instrumented) | ||
|
|
||
| assertFalse(text.contains("SentryBinderAdapter")) | ||
| } |
There was a problem hiding this comment.
l/m: Could be worth adding a test that actually loads the instrumented class to trigger bytecode verification (eg, no corrupted stack) and to verify that we can recover the binder method return value.
eg, create minimal stubs (a recording SentryBinderAdapter + a fake BluetoothAdapter.isEnabled() or whatever) -> instrument a synthetic class that calls BluetoothAdapter.isEnabled() and returns true -> load and invoke via reflection -> verify true is actually returned.
|
|
||
| ### Features | ||
|
|
||
| - Add Binder IPC call instrumentation ([#1159](https://github.com/getsentry/sentry-android-gradle-plugin/pull/1159)) |
There was a problem hiding this comment.
l: Do we normally also tell folks how to enable/disable (+ inform them that it's auto-enabled)?


Summary
SentryIpcTracer.onCallStart/onCallEndcalls so the SDK can emit spans/metrics for cross-process hops.io.sentry.android.core.internal.binder.SentryBinderAdapteris added in feat(android): binder IPC tracing adapter for Gradle plugin instrumentation sentry-java#5326 and will ship in sentry-javaTBD. Instrumentation is planned to no-op whensentry-android-coreis below that version.tracingInstrumentation.binder.enabledextension property, defaults totrue.Test plan
./gradlew :plugin-build:test --tests "io.sentry.android.gradle.instrumentation.binder.*"(9 tests, all pass)BinderMethodRegistryTest— registry lookup (unknown owner, unknown method, instance, static, Context subtype coverage)BinderIpcMethodVisitorTest— ASM-level visitor test: wraps known instance + static calls with tracer, emits correct component/method LDC constants, adds try/catch handler, leaves unknown calls and opcode-mismatched calls untouchedspotlessApplycleanRequires
🤖 Generated with Claude Code