fix: harden against top Google Play vitals crashes - #28
Conversation
Address the top crashes in the live release (v3.4.1 / 341004): - ScreenOrientation.set(): reuse the already-added overlay view instead of creating a new View and calling updateViewLayout() on it (which threw IllegalArgumentException "View not attached to window manager"); wrap all WindowManager add/update/remove calls in try/catch. - BluetoothDevicesViewModel: guard getBondedDevices() against SecurityException and re-check the Bluetooth runtime permission on the BT-state-changed path before loading devices. - ModeService: enter the foreground with a dependency-free notification, then stop cleanly (START_NOT_STICKY) when Koin is not started instead of crashing on the first get(); make onDestroy() skip DI-dependent teardown when Koin is absent so the crash is not merely relocated. - BroadcastService: call startForeground() first and unconditionally to avoid ForegroundServiceDidNotStartInTimeException; return START_NOT_STICKY to break the background auto-restart crash loop; guard the background start site against ForegroundServiceStartNotAllowedException; assign the receiver field only after registerReceiver() succeeds; guard unregister()/onDestroy() against absent Koin. - CarWidgetApplication: register notification channels before startKoin so a foreground-service notification can always be posted even if DI init fails. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a2a23b4b-49b1-4b4a-85d3-8bd95d5bc5e9
…updater Removes the legacy androidx JobIntentService that caused a Play vitals crash (IllegalArgumentException: Given work is not active) - a race in JobIntentService completeWork. Widget updates now run from Provider.onUpdate() via goAsync() plus the Koin AppCoroutineScope on Dispatchers.Default, with try/finally so the pending result is always finished. Deletes UpdateWidgetJob and its exported manifest service entry. Direct updateAppWidget calls never re-broadcast APPWIDGET_UPDATE, so the old WorkManager update-loop concern (issuetracker 115575872) does not apply. Off-main execution and fire-and-forget semantics for the other callers are preserved. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a2a23b4b-49b1-4b4a-85d3-8bd95d5bc5e9
There was a problem hiding this comment.
🟡 Not ready to approve
There are correctness/robustness gaps in the new widget update goAsync() flow and service restart guarding that can still leak pending results or contradict the stated crash-mitigation behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR targets top Google Play Vitals crashes by hardening the in-car and widget subsystems: foreground-service startup is made more deadline-safe, DI (Koin) restart edge-cases are handled defensively, Bluetooth permission-related crashes are avoided, overlay orientation handling is stabilized, and a legacy widget update JobIntentService path is removed.
Changes:
- Make in-car services more resilient to foreground-service timing and DI initialization edge cases.
- Wrap Bluetooth paired-device enumeration with permission checks and
SecurityExceptionhandling. - Replace
UpdateWidgetJobwith agoAsync()+ coroutine-driven widget update path; remove the exported manifest service entry.
File summaries
| File | Description |
|---|---|
| docs/ANDROID_EXPORTED_GUIDE.md | Updates exported-components documentation to reflect removal of UpdateWidgetJob service. |
| compose/src/androidMain/kotlin/info/anodsplace/carwidget/incar/ScreenOrientation.kt | Reuses overlay view and defends WindowManager operations to prevent orientation-related crashes. |
| compose/src/androidMain/kotlin/info/anodsplace/carwidget/incar/BluetoothDevicesViewModel.kt | Adds permission gating and SecurityException handling around bonded-device loading. |
| app/src/main/java/com/anod/car/home/UpdateWidgetJob.kt | Removes legacy JobIntentService widget updater that could crash under race conditions. |
| app/src/main/java/com/anod/car/home/incar/ModeService.kt | Starts foreground with a minimal notification and adds Koin-not-started guard paths. |
| app/src/main/java/com/anod/car/home/incar/BroadcastService.kt | Reorders/guards foreground entry and receiver registration/unregistration for robustness. |
| app/src/main/java/com/anod/car/home/CarWidgetApplication.kt | Registers notification channels before starting Koin to support dependency-free FGS notifications. |
| app/src/main/java/com/anod/car/home/appwidget/Provider.kt | Moves widget updates into AppWidgetProvider.onUpdate() using goAsync() + app coroutine scope. |
| app/src/main/AndroidManifest.xml | Removes exported <service> entry for UpdateWidgetJob. |
Review details
Suppressed comments (1)
app/src/main/java/com/anod/car/home/appwidget/Provider.kt:84
onCompleteis only invoked from inside the coroutine, butget<AppCoroutineScope>()happens before launching; if DI lookup throws, the update aborts without callingonComplete(andonUpdate()'sPendingResult.finish()callback won’t run). Catch failures aroundget()and invokeonCompletebefore returning.
val scope: AppCoroutineScope = get()
scope.launch(Dispatchers.Default) {
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Addresses rubber-duck review of the JobIntentService replacement: - Wrap the requestUpdate hand-off in try/catch so a synchronous failure (e.g. Koin not ready) still invokes onComplete and finishes the goAsync() broadcast instead of leaving it dangling. - Serialize widget rebuilds with a Mutex held across suspension, since WidgetViewBuilder.firstTimeInit() does a check-then-write on shared storage; concurrent rebuilds of the same widget could duplicate default shortcuts. limitedParallelism(1) would not hold across the inner withContext in create(). - Isolate per-widget failures so one failing widget no longer skips the rest of the batch. - Resolve applicationContext before launching so the coroutine never captures an Activity/Service context. - Make the goAsync completion callback idempotent (AtomicBoolean) and exception-safe so a stray or throwing finish() cannot crash the app. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a2a23b4b-49b1-4b4a-85d3-8bd95d5bc5e9
There was a problem hiding this comment.
🟡 Not ready to approve
There are correctness/clarity issues in the updated code (including a concrete null-parameter crash risk in ScreenOrientation and misleading concurrency comments), and the ModeService sticky return behavior still appears to contradict the PR’s stated mitigation intent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
app/src/main/java/com/anod/car/home/appwidget/Provider.kt:81
- The
updateMutexcomment is inaccurate/misleading: this mutex serializes all widget updates (not only “the same widget”), and there are no suspension points inside the critical section to justify thelimitedParallelism(1)comparison. Please adjust the comment to match the actual behavior/intent.
// Serializes widget rebuilds so two concurrent updates of the same widget can't race
// (WidgetViewBuilder.firstTimeInit performs a check-then-write on shared storage). The
// mutex is held across suspension points, which limitedParallelism(1) would not do.
private val updateMutex = Mutex()
compose/src/androidMain/kotlin/info/anodsplace/carwidget/incar/ScreenOrientation.kt:56
updateViewLayout()is called withlayoutParamsthat can be null (platform type accepts it), which can still crash at runtime (e.g., NPE) and currently won’t be caught. CapturelayoutParamsinto a non-null localvaland gate the update on it being non-null.
val currentView = overlayView
if (viewAdded && currentView != null) {
layoutParams?.screenOrientation = orientation
try {
windowManager.updateViewLayout(currentView, layoutParams)
app/src/main/java/com/anod/car/home/incar/BroadcastService.kt:113
- The catch comment is no longer correct after assigning
receiveronly after successful registration; anIllegalArgumentExceptionhere is more likely due to double-unregister or a context mismatch. Update the comment to reflect the real failure modes.
} catch (e: IllegalArgumentException) {
// Receiver was created but never successfully registered.
AppLog.e(e)
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…ice against missing Koin ModeService success path returned START_REDELIVER_INTENT, which lets the OS auto-restart the foreground service from the background and re-exposes the top Play vitals crash (ForegroundServiceDidNotStartInTimeException) that also implicates ModeService. Return START_NOT_STICKY to match BroadcastService and the PR's crash-fix intent; in-car mode is re-triggered on demand. BroadcastService.onStartCommand now explicitly stops when Koin is not started (GlobalContext.getOrNull() == null) before calling register()/get<InCarSettings>(), mirroring ModeService instead of relying on a downstream throw. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a2a23b4b-49b1-4b4a-85d3-8bd95d5bc5e9
There was a problem hiding this comment.
🟡 Not ready to approve
ScreenOrientation.set() still calls WindowManager.updateViewLayout() with a nullable layoutParams, which can crash with an NPE if state becomes inconsistent (the PR is explicitly crash-hardening this path).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
compose/src/androidMain/kotlin/info/anodsplace/carwidget/incar/BluetoothDevicesViewModel.kt:131
checkPermission()returns true when Bluetooth runtime permissions are missing, but the name reads like it would return true when permissions are granted. This is easy to misread in call sites likeif (checkPermission()) { return emptyList() }and can lead to inverted logic in future changes. Consider renaming to something likeisBluetoothPermissionMissing(); at minimum, add a short KDoc clarifying the boolean semantics.
if (checkPermission()) {
app/src/main/java/com/anod/car/home/appwidget/Provider.kt:81
- The comment for
updateMutexsays it serializes updates of “the same widget”, but this mutex actually serializes all widget rebuilds across the process. Either update the comment to match the behavior, or switch to a per-widget keyed lock if you only intend to serialize per-widget work.
// Serializes widget rebuilds so two concurrent updates of the same widget can't race
// (WidgetViewBuilder.firstTimeInit performs a check-then-write on shared storage). The
// mutex is held across suspension points, which limitedParallelism(1) would not do.
private val updateMutex = Mutex()
compose/src/androidMain/kotlin/info/anodsplace/carwidget/incar/ScreenOrientation.kt:56
updateViewLayout(currentView, layoutParams)passes a nullablelayoutParams(platform type), which can crash with an NPE if the state ever becomes inconsistent (e.g.,viewAddedtrue butlayoutParamsnull after an exception). Capture the params into a non-null local and only callupdateViewLayoutwhen both view and params are present; otherwise reset state and fall back to re-adding the overlay.
windowManager.updateViewLayout(currentView, layoutParams)
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…eview nits Address Copilot review feedback: - ScreenOrientation.set() no longer passes a nullable layoutParams to WindowManager.updateViewLayout() (an NPE there would not be caught by the IllegalArgumentException handler). Capture params into a non-null local, and on inconsistent state remove the stale overlay and re-add it instead. - Correct the Provider.updateMutex comment: it serializes all widget rebuilds process-wide, not only same-widget updates. - Correct the BroadcastService.unregister() catch comment to describe the real failure modes (double unregister / context mismatch). - Rename BluetoothDevicesViewModel.checkPermission() to isBluetoothPermissionMissing() so the inverted boolean reads correctly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a2a23b4b-49b1-4b4a-85d3-8bd95d5bc5e9
There was a problem hiding this comment.
🟡 Human review recommended
It changes Android foreground-service and widget-update lifecycle behavior in ways that are difficult to fully validate without targeted device/OS-level runtime verification.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Why
The live release (v3.4.1, versionCode 341004) is producing the top entries in Google Play vitals. This branch fixes the highest-frequency crashes, all in the in-car / widget subsystem, plus one long-tail widget-update crash.
What changed
Grouped by crash:
Foreground-service start timeout (highest volume, ~79 reports) -
BroadcastServiceandModeServicenow callstartForeground()first and unconditionally with a dependency-free notification, and returnSTART_NOT_STICKY. On API 31+,START_STICKYlet the OS auto-restart a killed service from the background; a slow cold start then missed the 10sstartForeground()deadline and crash-looped withForegroundServiceDidNotStartInTimeException.Koin not started - both services guard against being restarted into a process where Koin has not initialized yet. The guard also covers
onDestroy()/unregister()(which callget<InCarSettings>()), sincestopSelf()re-enters those paths.Channels.register()now runs beforestartKoinso the safe notification's channel always exists.Bluetooth SecurityException -
BluetoothDevicesViewModelwrapsgetBondedDevices()in a permission check plus try/catch and requests permissions when missing instead of crashing.ScreenOrientation IllegalArgumentException - the overlay view is reused instead of re-added, and all
WindowManageroperations are wrapped defensively.UpdateWidgetJob JobIntentService race - replaced the legacy
JobIntentService(which threwIllegalArgumentException: Given work is not active) with agoAsync()+ KoinAppCoroutineScopecoroutine driven fromProvider.onUpdate(). Deletes the class and its exported manifest<service>entry.Notes for reviewers
START_NOT_STICKYis intentional and safe: in-car detection is driven by the manifest-registeredModeBroadcastReceiver(plus boot/activity re-arm paths), independent of the service staying alive. The only behavior lost is a wired HEADSET_PLUG/DOCK_EVENT immediately after an OS kill with no preceding manifest-deliverable event.updateAppWidgetcalls, which never re-broadcastAPPWIDGET_UPDATE, so the historical WorkManager update-loop concern (issuetracker 115575872) does not apply here.goAsync()fits since updates complete well under its ~10s budget.Verified with
:app:compileDebugKotlinand:app:testDebugUnitTest.