Crash the debug app when accessing a non-SDK API #13354
Conversation
Only on debug builds
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
| System.err.println("Non-SDK API violation detected: ${v.message}") | ||
| Process.killProcess(Process.myPid()) | ||
| exitProcess(10) |
There was a problem hiding this comment.
This code is similar to the default that Android uses when we enable penaltyDeath().
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #13354 +/- ##
============================================
- Coverage 41.14% 41.13% -0.01%
Complexity 6431 6431
============================================
Files 1322 1323 +1
Lines 77211 77219 +8
Branches 10658 10658
============================================
Hits 31765 31765
- Misses 42621 42629 +8
Partials 2825 2825 ☔ View full report in Codecov by Sentry. |
| @Suppress("MagicNumber") | ||
| class NonSdkApiViolationHandler : OnVmViolationListener { | ||
| override fun onVmViolation(v: Violation) { | ||
| if (v !is NonSdkApiUsedViolation) return |
There was a problem hiding this comment.
Would it make sense to crash on all violations in debugging or are there some that we don't want too?
There was a problem hiding this comment.
Personally I won't do this, IMO not all violations are worth an urgent fix, so it's case by case, so if the app crashes, then it'll block the other developer's workflows.
The Non-SDK is the exception because Google are fighting their usage which could lead to production crashes if it goes unnoticed.
There was a problem hiding this comment.
Maybe the fact that you found violations now when the code/libraries were added a long time ago, and nobody noticed that, may prove the point that early detection is good?
WooCommerce/src/debug/kotlin/com/woocommerce/android/NonSdkApiViolationHandler.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
LGTM! I checked with just throwing an exception and what currently in the PR and I frankly prefer the expcetion - at least there is a stack trace
now it just:
2025-01-21 14:55:00.064 5512-5512 System.err com.woocommerce.android.dev W Non-SDK API violation detected: Ljava/lang/StackTraceElement;-><init>()V
2025-01-21 14:55:00.064 5512-5512 Process com.woocommerce.android.dev I Sending signal. PID: 5512 SIG: 9
I think we won't figureout for a while why it crashes
wdyt?
|
@kidinov this is not ready for merge, after more testing of the app without my patch above, I found out that we already had some NonSdk violations, which means if we merge this no one will be able to launch the app. The violation comes from |
hichamboushaba
added
the
status: do not merge
|
Another violation if we try to get around the one from |
Isn't this just postponing the resolution of the problems? If they change the behavior of nonpublic methods, it may just break it with the next SDK version, which will go unnoticed by us and end up in the production |
Yes maybe, but is there something else that we can do? These are usages by third party libraries, and not by our code. The way I see this, iff the libraries are still using those APIs, it's probably because they don't find an alternative, and if the API is still allowed by Google, then maybe it's because they didn't provide this alternative yet, so IMO all we can so is wait until it happens, at which time we could have updated libraries that solve the issue. |
Why do you think that API is allowed by google? They use reflection to access not public interfaces, it's not API
I'd say we may try:
|
I say allowed in the sense that they don't block it yet, but it's still a hidden API (using
Personally I think it doesn't bring enough advantages to justify the effort, but that's just a personal opinion, feel free to involve the rest of the team or to proceed with your approach. |
|
Version |
Description
As discussed here p91TBi-cHS-p2#comment-13866, this PR adds a specific violation listener that crashes the app when accessing a non-SDK API.
Steps to reproduce
Non-SDK API violation detected: Landroid/app/Activity;->setDisablePreviewScreenshots(Z)VTesting information
The tests that have been performed
^
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: