| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…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
|
Sorry, something went wrong.
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.
Sorry, something went wrong.
| 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.
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.
Sorry, something went wrong.
| 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
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 AgentReview 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.
Sorry, something went wrong.
There was a problem hiding this comment.
+1
Sorry, something went wrong.
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.
Sorry, something went wrong.
| 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)
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
| } | ||
|
|
||
| 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).
Sorry, something went wrong.
|
|
||
| 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.)
Sorry, something went wrong.
|
|
||
| // 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.
Sorry, something went wrong.
| // 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
Sorry, something went wrong.
| 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.
+1
Sorry, something went wrong.
| 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)
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
|
|
||
| ### 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)?
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Summary
Test plan
Requires
🤖 Generated with Claude Code