add a system_server API for listening to GosPackageState changes; fix racy dispatch of GosPackageState change callback#406
Conversation
| changeCallbacksThread.getThreadHandler().post(() -> { | ||
| synchronized (changeCallbacks) { | ||
| for (GosPackageStateChangeCallback callback : changeCallbacks) { | ||
| callback.onGosPackageStateChanged(uid, updatedGosPs, userId); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Technically not safe from callbacks that remove themselves
e.g. see
mListeners correctly in dispatchVisibilityChanged, as it doesn't use the iterator and thus avoids the COWIterator's snapshots and doesn't even use the listener snapshot passed into the function)
Also what's our policy here for uncaught (runtime) exceptions from a callback? Currently it seems like it'll just crash
There was a problem hiding this comment.
Also what's our policy here for uncaught (runtime) exceptions from a callback?
This is an internal system_server API, I think we should rely on the caller for exception handling.
| @Override | ||
| public void onGosPackageStateChanged(GosPackageState state) { | ||
| // this is a oneway method, caller (ActivityManager) will not be blocked | ||
| ActivityThreadHooks.onGosPackageStateChanged(mInitialApplication, state, false); | ||
| public void onGosPackageStateChanged() { | ||
| Context ctx = mInitialApplication; | ||
| GosPackageState state = GosPackageState.getForSelf(ctx); | ||
| ActivityThreadHooks.onGosPackageStateChanged(ctx, state, false); | ||
| } |
There was a problem hiding this comment.
There's a window where mInitialApplication can be null, e.g. if an app overrides Application#attachBaseContext and takes too long, and then storage/contact scopes is enabled during that time.
There seems to already be AOSP code here that accounts for this, e.g. see how mUpdateHttpProxyOnBind is used in ActivityThread#updateHttpProxy, etc.
e.g. as a (contrived) NPE example, consider an app that has this Application:
class SlowStartApplication : Application() {
override fun attachBaseContext(base: Context) {
super.attachBaseContext(base)
Log.i(TAG, "attachBaseContext: blocking startup for ${STARTUP_BLOCK_MS}ms")
Log.i(TAG, "attachBaseContext: enable Storage Scopes or Contact Scopes while this process is blocked")
SystemClock.sleep(STARTUP_BLOCK_MS)
Log.i(TAG, "attachBaseContext: startup block finished")
}
override fun onCreate() {
super.onCreate()
Log.i(TAG, "onCreate: application startup completed")
}
companion object {
const val TAG = "SampleApp"
private const val STARTUP_BLOCK_MS = 12_000L
}
}Then attachBaseContext's blocking/slow code can be triggered by the app receiving a broadcast, so it's not always from a user launching the app then going to configure storage/contact scopes. The app can receive a broadcast and run its application code while the user is turning on storage/contact scopes
There was a problem hiding this comment.
NPE when such an app receives broadcast to run its slow attachBaseContext code and Storage Scopes is enabled while it's blocked:
6933-6933 SampleApp I attachBaseContext: blocking startup for 12000ms
6933-6933 SampleApp I attachBaseContext: enable Storage Scopes or Contact Scopes while this process is blocked
6933-6947 Binder W Caught a RuntimeException from the binder stub implementation.
java.lang.NullPointerException: Attempt to invoke virtual method 'java.lang.String android.content.Context.getPackageName()' on a null object reference
at android.content.pm.GosPackageState.getForSelf(GosPackageState.java:244)
at android.app.ActivityThread$ApplicationThread.onGosPackageStateChanged(ActivityThread.java:2522)
at android.app.IApplicationThread$Stub.onTransact(IApplicationThread.java:1595)
at android.app.ActivityThread$ApplicationThread.onTransact(ActivityThread.java:1248)
at android.os.Binder.execTransactInternal(Binder.java:1417)
at android.os.Binder.execTransact(Binder.java:1364)
6933-6933 SampleApp I attachBaseContext: startup block finished
6933-6933 SampleApp I onCreate: application startup completed
NPE can also be triggered in the old code with Contact Scopes since that uses the ctx
See comment in GosPackageStatePmHooks change.
mInitialApplication is null in the early part of the app process initialization sequence.
CopyOnWriteArrayList iterator makes a list snapshot, which provides support for changeCallbacks removing themselves upon invocation.
Instead, make GosPackageStateChangeCallbacks specify the Handler for callback dispatch.
| } | ||
|
|
||
| /** | ||
| * Called after each successful GosPackageState update on a separate callbacks thread. |
There was a problem hiding this comment.
Stale javadoc since it now uses handler
2 participants
No description provided.