From 4ee3ac26768e4113a3e8522c4533104fad8e1708 Mon Sep 17 00:00:00 2001 From: Soonil Nagarkar Date: Fri, 8 Feb 2019 19:19:24 -0800 Subject: [PATCH] Fix LOCATION_PROVIDERS_ALLOWED with provider status Ensures that LOCATION_PROVIDERS_ALLOWED is properly updated through all location provider state changes, including location on/off. Remove all initialization of LOCATION_PROVIDERS_ALLOWED, as the setting is now completely controlled by LMS. Bug: 124300200 Test: Manually Change-Id: Ic715347515bcc417d873c48113ce4303685c0aa7 --- .../SettingsProvider/res/values/defaults.xml | 4 +- .../providers/settings/DatabaseHelper.java | 3 - .../providers/settings/SettingsProvider.java | 32 ++-- .../server/LocationManagerService.java | 138 ++++++++++-------- 4 files changed, 96 insertions(+), 81 deletions(-) diff --git a/packages/SettingsProvider/res/values/defaults.xml b/packages/SettingsProvider/res/values/defaults.xml index 26ea6ab2e8be9..65e0c0fbc9916 100644 --- a/packages/SettingsProvider/res/values/defaults.xml +++ b/packages/SettingsProvider/res/values/defaults.xml @@ -40,8 +40,8 @@ false false true - - gps,network + + 3 true true true diff --git a/packages/SettingsProvider/src/com/android/providers/settings/DatabaseHelper.java b/packages/SettingsProvider/src/com/android/providers/settings/DatabaseHelper.java index 0ee16a9e9ed25..5e2b7c86ee934 100644 --- a/packages/SettingsProvider/src/com/android/providers/settings/DatabaseHelper.java +++ b/packages/SettingsProvider/src/com/android/providers/settings/DatabaseHelper.java @@ -2340,9 +2340,6 @@ class DatabaseHelper extends SQLiteOpenHelper { stmt = db.compileStatement("INSERT OR IGNORE INTO secure(name,value)" + " VALUES(?,?);"); - loadStringSetting(stmt, Settings.Secure.LOCATION_PROVIDERS_ALLOWED, - R.string.def_location_providers_allowed); - // Don't do this. The SystemServer will initialize ADB_ENABLED from a // persistent system property instead. //loadSetting(stmt, Settings.Secure.ADB_ENABLED, 0); diff --git a/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java b/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java index ceafbfa4da150..d6c33a3b077cb 100644 --- a/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java +++ b/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java @@ -969,6 +969,11 @@ public class SettingsProvider extends ContentProvider { try { synchronized (mLock) { Setting setting = getSecureSetting( + Settings.Secure.LOCATION_MODE, userId); + updateSecureSetting(Settings.Secure.LOCATION_MODE, + setting != null ? setting.getValue() : null, null, + true, userId, true); + setting = getSecureSetting( Settings.Secure.LOCATION_PROVIDERS_ALLOWED, userId); updateSecureSetting(Settings.Secure.LOCATION_PROVIDERS_ALLOWED, setting != null ? setting.getValue() : null, null, @@ -4229,23 +4234,18 @@ public class SettingsProvider extends ContentProvider { final Setting locationProvidersAllowed = secureSettings.getSettingLocked( Secure.LOCATION_PROVIDERS_ALLOWED); - String defLocationMode = Integer.toString( - !TextUtils.isEmpty(locationProvidersAllowed.getValue()) - ? Secure.LOCATION_MODE_ON - : Secure.LOCATION_MODE_OFF); + final int defLocationMode; + if (locationProvidersAllowed.isNull()) { + defLocationMode = getContext().getResources().getInteger( + R.integer.def_location_mode); + } else { + defLocationMode = + !TextUtils.isEmpty(locationProvidersAllowed.getValue()) + ? Secure.LOCATION_MODE_ON + : Secure.LOCATION_MODE_OFF; + } secureSettings.insertSettingLocked( - Secure.LOCATION_MODE, defLocationMode, - null, true, SettingsState.SYSTEM_PACKAGE_NAME); - - // also reset LOCATION_PROVIDERS_ALLOWED back to the default value - this - // setting is now only for debug/test purposes, and will likely be removed - // in a later release. LocationManagerService is responsible for adjusting - // these settings to the proper state. - - String defLocationProvidersAllowed = getContext().getResources().getString( - R.string.def_location_providers_allowed); - secureSettings.insertSettingLocked( - Secure.LOCATION_PROVIDERS_ALLOWED, defLocationProvidersAllowed, + Secure.LOCATION_MODE, Integer.toString(defLocationMode), null, true, SettingsState.SYSTEM_PACKAGE_NAME); } diff --git a/services/core/java/com/android/server/LocationManagerService.java b/services/core/java/com/android/server/LocationManagerService.java index d8b96e43114a9..2d62850a33c88 100644 --- a/services/core/java/com/android/server/LocationManagerService.java +++ b/services/core/java/com/android/server/LocationManagerService.java @@ -337,7 +337,7 @@ public class LocationManagerService extends ILocationManager.Stub { @Override public void onChange(boolean selfChange) { synchronized (mLock) { - onProviderAllowedChangedLocked(true); + onProviderAllowedChangedLocked(); } } }, UserHandle.USER_ALL); @@ -436,6 +436,10 @@ public class LocationManagerService extends ILocationManager.Stub { @GuardedBy("mLock") private void onLocationModeChangedLocked(boolean broadcast) { + if (D) { + Log.d(TAG, "location enabled is now " + isLocationEnabled()); + } + for (LocationProvider p : mProviders) { p.onLocationModeChangedLocked(); } @@ -448,16 +452,10 @@ public class LocationManagerService extends ILocationManager.Stub { } @GuardedBy("mLock") - private void onProviderAllowedChangedLocked(boolean broadcast) { + private void onProviderAllowedChangedLocked() { for (LocationProvider p : mProviders) { p.onAllowedChangedLocked(); } - - if (broadcast) { - mContext.sendBroadcastAsUser( - new Intent(LocationManager.PROVIDERS_CHANGED_ACTION), - UserHandle.ALL); - } } @GuardedBy("mLock") @@ -827,6 +825,10 @@ public class LocationManagerService extends ILocationManager.Stub { return; } + if (D) { + Log.d(TAG, "foreground user is changing to " + userId); + } + // let providers know the current user is on the way out before changing the user for (LocationProvider p : mProviders) { p.onUserChangingLocked(); @@ -839,7 +841,12 @@ public class LocationManagerService extends ILocationManager.Stub { // if the user changes, per-user settings may also have changed onLocationModeChangedLocked(false); - onProviderAllowedChangedLocked(false); + onProviderAllowedChangedLocked(); + + // always force useability to be rechecked, even if no per-user settings have changed + for (LocationProvider p : mProviders) { + p.onUseableChangedLocked(false); + } } private class LocationProvider implements AbstractLocationProvider.LocationProviderManager { @@ -891,9 +898,13 @@ public class LocationManagerService extends ILocationManager.Stub { public void attachLocked(AbstractLocationProvider provider) { checkNotNull(provider); checkState(mProvider == null); - mProvider = provider; - onUseableChangedLocked(); + if (D) { + Log.d(TAG, mName + " provider attached"); + } + + mProvider = provider; + onUseableChangedLocked(false); } public String getName() { @@ -1054,26 +1065,12 @@ public class LocationManagerService extends ILocationManager.Stub { return; } - mEnabled = enabled; - - // update provider allowed settings to reflect enabled status - if (mIsManagedBySettings) { - if (mEnabled && !mAllowed) { - Settings.Secure.putStringForUser( - mContext.getContentResolver(), - Settings.Secure.LOCATION_PROVIDERS_ALLOWED, - "+" + mName, - mCurrentUserId); - } else if (!mEnabled && mAllowed) { - Settings.Secure.putStringForUser( - mContext.getContentResolver(), - Settings.Secure.LOCATION_PROVIDERS_ALLOWED, - "-" + mName, - mCurrentUserId); - } + if (D) { + Log.d(TAG, mName + " provider enabled is now " + mEnabled); } - onUseableChangedLocked(); + mEnabled = enabled; + onUseableChangedLocked(false); } }); } @@ -1091,41 +1088,28 @@ public class LocationManagerService extends ILocationManager.Stub { @GuardedBy("mLock") public void onLocationModeChangedLocked() { - onUseableChangedLocked(); - } - - private boolean isAllowed() { - return isAllowedForUser(mCurrentUserId); - } - - private boolean isAllowedForUser(int userId) { - String allowedProviders = Settings.Secure.getStringForUser( - mContext.getContentResolver(), - Settings.Secure.LOCATION_PROVIDERS_ALLOWED, - userId); - return TextUtils.delimitedStringContains(allowedProviders, ',', mName); + onUseableChangedLocked(false); } @GuardedBy("mLock") public void onAllowedChangedLocked() { if (mIsManagedBySettings) { - boolean allowed = isAllowed(); + String allowedProviders = Settings.Secure.getStringForUser( + mContext.getContentResolver(), + Settings.Secure.LOCATION_PROVIDERS_ALLOWED, + mCurrentUserId); + boolean allowed = TextUtils.delimitedStringContains(allowedProviders, ',', mName); + if (allowed == mAllowed) { return; } - mAllowed = allowed; - // make a best effort to keep the setting matching the real enabled state of the - // provider so that legacy applications aren't broken. - if (mAllowed && !mEnabled) { - Settings.Secure.putStringForUser( - mContext.getContentResolver(), - Settings.Secure.LOCATION_PROVIDERS_ALLOWED, - "-" + mName, - mCurrentUserId); + if (D) { + Log.d(TAG, mName + " provider allowed is now " + mAllowed); } - onUseableChangedLocked(); + mAllowed = allowed; + onUseableChangedLocked(true); } } @@ -1140,17 +1124,49 @@ public class LocationManagerService extends ILocationManager.Stub { } @GuardedBy("mLock") - public void onUseableChangedLocked() { + private boolean isUseableIgnoringAllowedLocked() { + return mProvider != null && mProviders.contains(this) && isLocationEnabled() + && mEnabled; + } + + @GuardedBy("mLock") + public void onUseableChangedLocked(boolean isAllowedChanged) { // if any property that contributes to "useability" here changes state, it MUST result // in a direct or indrect call to onUseableChangedLocked. this allows the provider to // guarantee that it will always eventually reach the correct state. - boolean useable = mProvider != null - && mProviders.contains(this) && isLocationEnabled() && mAllowed && mEnabled; + boolean useableIgnoringAllowed = isUseableIgnoringAllowedLocked(); + boolean useable = useableIgnoringAllowed && mAllowed; + + // update deprecated provider allowed settings for backwards compatibility, and do this + // even if there is no change in overall useability state. this may result in trying to + // overwrite the same value, but Settings handles deduping this. + if (mIsManagedBySettings) { + // a "-" change derived from the allowed setting should not be overwritten, but a + // "+" change should be corrected if necessary + if (useableIgnoringAllowed && !isAllowedChanged) { + Settings.Secure.putStringForUser( + mContext.getContentResolver(), + Settings.Secure.LOCATION_PROVIDERS_ALLOWED, + "+" + mName, + mCurrentUserId); + } else if (!useableIgnoringAllowed) { + Settings.Secure.putStringForUser( + mContext.getContentResolver(), + Settings.Secure.LOCATION_PROVIDERS_ALLOWED, + "-" + mName, + mCurrentUserId); + } + } + if (useable == mUseable) { return; } mUseable = useable; + if (D) { + Log.d(TAG, mName + " provider useable is now " + mUseable); + } + if (!mUseable) { // If any provider has been disabled, clear all last locations for all // providers. This is to be on the safe side in case a provider has location @@ -1160,6 +1176,10 @@ public class LocationManagerService extends ILocationManager.Stub { } updateProviderUseableLocked(this); + + mContext.sendBroadcastAsUser( + new Intent(LocationManager.PROVIDERS_CHANGED_ACTION), + UserHandle.ALL); } @GuardedBy("mLock") @@ -1720,7 +1740,7 @@ public class LocationManagerService extends ILocationManager.Stub { mProviders.add(provider); provider.onAllowedChangedLocked(); // allowed state may change while provider was inactive - provider.onUseableChangedLocked(); + provider.onUseableChangedLocked(false); } @GuardedBy("mLock") @@ -1728,7 +1748,7 @@ public class LocationManagerService extends ILocationManager.Stub { if (mProviders.remove(provider)) { long identity = Binder.clearCallingIdentity(); try { - provider.onUseableChangedLocked(); + provider.onUseableChangedLocked(false); } finally { Binder.restoreCallingIdentity(identity); } @@ -2113,7 +2133,6 @@ public class LocationManagerService extends ILocationManager.Stub { } } - if (D) Log.d(TAG, "provider request: " + provider + " " + providerRequest); provider.setRequestLocked(providerRequest, worksource); } @@ -2507,7 +2526,6 @@ public class LocationManagerService extends ILocationManager.Stub { @Override public Location getLastLocation(LocationRequest r, String packageName) { - if (D) Log.d(TAG, "getLastLocation: " + r); synchronized (mLock) { LocationRequest request = r != null ? r : DEFAULT_LOCATION_REQUEST; int allowedResolutionLevel = getCallerAllowedResolutionLevel();