Repository navigation
Do not crash when the activity is created in the background - #126
Conversation
startVpnService() calls startService() unguarded, from onCreate() as well as onResume(), and
Android 12 and later refuse a service start from a background app with
BackgroundServiceStartNotAllowedException. VideoActivity can be created in the background - that
is how its USB filter launches it when the adapter is plugged in with the screen off - and then
the exception escapes onCreate() and the app is gone before it has shown anything.
Reproduced on a Quest 3 by starting the activity while the headset was asleep:
FATAL EXCEPTION: main
Unable to start activity ... VideoActivity:
android.app.BackgroundServiceStartNotAllowedException: Not allowed to start service
Intent { cmp=com.openipc.pixelpilot/.WfbNgVpnService }: app is in background
at VideoActivity.startVpnService(VideoActivity.java:1298)
at VideoActivity.onCreate(VideoActivity.java:340)
The start is skipped with a warning instead. Nothing is lost by that: onResume() calls
startVpnService() again once the app is actually in front, which is when the tunnel is wanted.
The stop in onPause() and the start after the VPN consent go through the same guard, since both
can run with the app on its way to the background.
It catches IllegalStateException, which BackgroundServiceStartNotAllowedException extends,
rather than naming a class that only exists from API 31 - minSdk is lower than that.
PR Summary by QodoPrevent background VPN starts from crashing VideoActivity
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
From review: onPause() sent STOP_SERVICE through startServiceIfAllowed(), so whenever Android refused that start the stop was dropped with it and the tunnel stayed up. How often that happens is narrower than it might look. Going to sleep with the app in front, the stop went through - onPause() still ran while the app was allowed to start services, and the service logged its own shutdown. The crash this branch fixes came from onCreate() in an activity started while already asleep, not from onPause() on the way into sleep. But nothing guarantees onPause() runs before the app counts as background, and a stop routed through a call that may be refused silently is the wrong shape whether or not it bites today. stopService() would not have been the fix. Once the interface is established the system holds its own binding to a VpnService, so stopService() leaves the service - and the tunnel - running until the interface is closed, and onDestroy() never comes. The service runs in the app's own process, so onPause() now reaches the running instance and shuts it down directly: worker threads stopped, interface closed, stopSelf(). That cannot be refused. The STOP_SERVICE action does the same through the same method.
|
Code review by qodo was updated up to the latest commit 7a0bf4b |
josephnef
left a comment
josephnef
left a comment
There was a problem hiding this comment.
Approving. The analysis in the description is right: a BackgroundServiceStartNotAllowedException out of onCreate when the USB filter launches the activity with the screen off, and a stop that went through startService was subject to the same refusal. Routing the stop through the in-process instance is the correct fix given the system's own binding to an established VpnService, and running is cleared in onDestroy, so it does not leak. Catching IllegalStateException for minSdk 26 is the right call. CI is green on the reopened run.
Heads-up for sequencing: this conflicts with #129 in VideoActivity.java (same onPause region). Since this is the smaller and more urgent fix it should land first and #129 rebase on it.
2 participants
startVpnService()callsstartService()unguarded, fromonCreate()as well asonResume(),and Android 12 and later refuse a service start from a background app with
BackgroundServiceStartNotAllowedException.VideoActivitycan be created in the background —that is how its USB filter launches it when the adapter is plugged in with the screen off — and
then the exception escapes
onCreate()and the app is gone before it has shown anything.Reproduced on a Quest 3 by starting the activity while the headset was asleep:
The start is skipped with a warning instead. Nothing is lost by that:
onResume()callsstartVpnService()again once the app is actually in front, which is when the tunnel is wanted.The start after the VPN consent goes through the same guard.
The stop in
onPause()no longer goes through a service start at all. Routed through the guard,a refused start would have dropped the stop with it and left the tunnel up;
stopService()wouldnot help either, because once the interface is established the system holds its own binding to
a
VpnService. The service runs in the app's own process, soonPause()shuts the runninginstance down directly through
WfbNgVpnService.stopRunning().It catches
IllegalStateException, whichBackgroundServiceStartNotAllowedExceptionextends,rather than naming a class that only exists from API 31 —
minSdkis 26.Testing
On a Quest 3, the same launch with the headset asleep now logs
and the app survives. When the headset was put on and the app came to the front,
onResume()started the tunnel:
tun0came up at 10.5.0.3/24. Going to sleep with the app in front takes thetunnel down (
VPN Service stopping,destroyed), and waking brings it back. The process stayedthe same throughout, with nothing in the crash buffer.