From 8e81109b2f1ff3c02773893cbface624e966b5f2 Mon Sep 17 00:00:00 2001 From: Soonil Nagarkar Date: Wed, 12 Aug 2020 22:02:36 -0700 Subject: [PATCH] Reduce ServiceWatcher thrashing ServiceWatcher forced a rebind everytime something about the connected package changed. This causes a lot of thrashing, as every time a component in the bound package changes (which happens a lot in Google Play services for instance), the service was completely rebound. This was likely initially necessary because there was no way to detect when a binding failed completely. Current OS versions support onBindingDied() to detect this case however, so it should no longer be necessary to rebind everytime anything changes. Also fixes NPE crash in LocationProviderManager when debug logging is enabled. Bug: 163076048 Bug: 164035101 Test: manual Change-Id: Ida4b8c8774f68d63ab319ed342055ff050e39f95 --- .../com/android/server/ServiceWatcher.java | 47 +++++++------------ .../location/LocationProviderManager.java | 6 ++- 2 files changed, 20 insertions(+), 33 deletions(-) diff --git a/services/core/java/com/android/server/ServiceWatcher.java b/services/core/java/com/android/server/ServiceWatcher.java index 0038dc2e8da03..b78b5d945a08f 100644 --- a/services/core/java/com/android/server/ServiceWatcher.java +++ b/services/core/java/com/android/server/ServiceWatcher.java @@ -237,25 +237,14 @@ public class ServiceWatcher implements ServiceConnection { } new PackageMonitor() { - @Override - public void onPackageUpdateFinished(String packageName, int uid) { - ServiceWatcher.this.onPackageChanged(packageName); - } - - @Override - public void onPackageAdded(String packageName, int uid) { - ServiceWatcher.this.onPackageChanged(packageName); - } - - @Override - public void onPackageRemoved(String packageName, int uid) { - ServiceWatcher.this.onPackageChanged(packageName); - } - @Override public boolean onPackageChanged(String packageName, int uid, String[] components) { - ServiceWatcher.this.onPackageChanged(packageName); - return super.onPackageChanged(packageName, uid, components); + return true; + } + + @Override + public void onSomePackagesChanged() { + onBestServiceChanged(false); } }.register(mContext, UserHandle.ALL, true, mHandler); @@ -320,7 +309,7 @@ public class ServiceWatcher implements ServiceConnection { if (!mTargetService.equals(ServiceInfo.NONE)) { if (D) { - Log.i(TAG, "[" + mIntent.getAction() + "] unbinding from " + mTargetService); + Log.d(TAG, "[" + mIntent.getAction() + "] unbinding from " + mTargetService); } mContext.unbindService(this); @@ -335,9 +324,7 @@ public class ServiceWatcher implements ServiceConnection { Preconditions.checkState(mTargetService.component != null); - if (D) { - Log.i(TAG, getLogPrefix() + " binding to " + mTargetService); - } + Log.i(TAG, getLogPrefix() + " binding to " + mTargetService); Intent bindIntent = new Intent(mIntent).setComponent(mTargetService.component); if (!mContext.bindServiceAsUser(bindIntent, this, @@ -355,7 +342,7 @@ public class ServiceWatcher implements ServiceConnection { Preconditions.checkState(mBinder == null); if (D) { - Log.i(TAG, getLogPrefix() + " connected to " + component.toShortString()); + Log.d(TAG, getLogPrefix() + " connected to " + component.toShortString()); } mBinder = binder; @@ -379,7 +366,7 @@ public class ServiceWatcher implements ServiceConnection { } if (D) { - Log.i(TAG, getLogPrefix() + " disconnected from " + component.toShortString()); + Log.d(TAG, getLogPrefix() + " disconnected from " + component.toShortString()); } mBinder = null; @@ -392,13 +379,16 @@ public class ServiceWatcher implements ServiceConnection { public final void onBindingDied(ComponentName component) { Preconditions.checkState(Looper.myLooper() == mHandler.getLooper()); - if (D) { - Log.i(TAG, getLogPrefix() + " " + component.toShortString() + " died"); - } + Log.i(TAG, getLogPrefix() + " " + component.toShortString() + " died"); onBestServiceChanged(true); } + @Override + public final void onNullBinding(ComponentName component) { + Log.e(TAG, getLogPrefix() + " " + component.toShortString() + " has null binding"); + } + void onUserSwitched(@UserIdInt int userId) { mCurrentUserId = userId; onBestServiceChanged(false); @@ -410,11 +400,6 @@ public class ServiceWatcher implements ServiceConnection { } } - void onPackageChanged(String packageName) { - // force a rebind if the changed package was the currently connected package - onBestServiceChanged(packageName.equals(mTargetService.getPackageName())); - } - /** * Runs the given function asynchronously if and only if currently connected. Suppresses any * RemoteException thrown during execution. diff --git a/services/core/java/com/android/server/location/LocationProviderManager.java b/services/core/java/com/android/server/location/LocationProviderManager.java index d4f8c7e855b9f..04cc8b5294849 100644 --- a/services/core/java/com/android/server/location/LocationProviderManager.java +++ b/services/core/java/com/android/server/location/LocationProviderManager.java @@ -229,7 +229,7 @@ class LocationProviderManager extends // we cache these values because checking/calculating on the fly is more expensive private boolean mPermitted; private boolean mForeground; - @Nullable private LocationRequest mProviderLocationRequest; + private LocationRequest mProviderLocationRequest; private boolean mIsUsingHighPower; protected Registration(LocationRequest request, CallerIdentity identity, @@ -244,6 +244,8 @@ class LocationProviderManager extends } else { mWorkSource = identity.addToWorkSource(null); } + + mProviderLocationRequest = super.getRequest(); } @GuardedBy("mLock") @@ -313,7 +315,7 @@ class LocationProviderManager extends @Override public final LocationRequest getRequest() { - return Objects.requireNonNull(mProviderLocationRequest); + return mProviderLocationRequest; } public final boolean isForeground() {