Handle races caused by rapid settings changed broadcasts
- Fix b/10447517
Change-Id: I63ef98c9023cee1a15be61b966aed06dc35e9bd5
diff --git a/src/com/android/settings/location/InjectedSetting.java b/src/com/android/settings/location/InjectedSetting.java
index 01d3236..d8a3f49 100644
--- a/src/com/android/settings/location/InjectedSetting.java
+++ b/src/com/android/settings/location/InjectedSetting.java
@@ -17,12 +17,17 @@
package com.android.settings.location;
import android.content.Intent;
+import android.text.TextUtils;
+import android.util.Log;
+import com.android.internal.annotations.Immutable;
+import com.android.internal.util.Preconditions;
/**
* Specifies a setting that is being injected into Settings > Location > Location services.
*
* @see android.location.SettingInjectorService
*/
+@Immutable
class InjectedSetting {
/**
@@ -53,13 +58,30 @@
*/
public final String settingsActivity;
- public InjectedSetting(String packageName, String className,
+ private InjectedSetting(String packageName, String className,
String title, int iconId, String settingsActivity) {
- this.packageName = packageName;
- this.className = className;
- this.title = title;
+ this.packageName = Preconditions.checkNotNull(packageName, "packageName");
+ this.className = Preconditions.checkNotNull(className, "className");
+ this.title = Preconditions.checkNotNull(title, "title");
this.iconId = iconId;
- this.settingsActivity = settingsActivity;
+ this.settingsActivity = Preconditions.checkNotNull(settingsActivity);
+ }
+
+ /**
+ * Returns a new instance, or null.
+ */
+ public static InjectedSetting newInstance(String packageName, String className,
+ String title, int iconId, String settingsActivity) {
+ if (packageName == null || className == null ||
+ TextUtils.isEmpty(title) || TextUtils.isEmpty(settingsActivity)) {
+ if (Log.isLoggable(SettingsInjector.TAG, Log.WARN)) {
+ Log.w(SettingsInjector.TAG, "Illegal setting specification: package="
+ + packageName + ", class=" + className
+ + ", title=" + title + ", settingsActivity=" + settingsActivity);
+ }
+ return null;
+ }
+ return new InjectedSetting(packageName, className, title, iconId, settingsActivity);
}
@Override
@@ -81,4 +103,26 @@
intent.setClassName(packageName, className);
return intent;
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) return true;
+ if (!(o instanceof InjectedSetting)) return false;
+
+ InjectedSetting that = (InjectedSetting) o;
+
+ return packageName.equals(that.packageName) && className.equals(that.className)
+ && title.equals(that.title) && iconId == that.iconId
+ && settingsActivity.equals(that.settingsActivity);
+ }
+
+ @Override
+ public int hashCode() {
+ int result = packageName.hashCode();
+ result = 31 * result + className.hashCode();
+ result = 31 * result + title.hashCode();
+ result = 31 * result + iconId;
+ result = 31 * result + settingsActivity.hashCode();
+ return result;
+ }
}
diff --git a/src/com/android/settings/location/LocationSettings.java b/src/com/android/settings/location/LocationSettings.java
index 7d25144..1c9409c 100644
--- a/src/com/android/settings/location/LocationSettings.java
+++ b/src/com/android/settings/location/LocationSettings.java
@@ -31,6 +31,7 @@
import android.preference.PreferenceManager;
import android.preference.PreferenceScreen;
import android.provider.Settings;
+import android.util.Log;
import android.view.Gravity;
import android.widget.CompoundButton;
import android.widget.Switch;
@@ -47,6 +48,9 @@
*/
public class LocationSettings extends LocationSettingsBase
implements CompoundButton.OnCheckedChangeListener {
+
+ private static final String TAG = "LocationSettings";
+
/** Key for preference screen "Mode" */
private static final String KEY_LOCATION_MODE = "location_mode";
/** Key for preference category "Recent location requests" */
@@ -146,8 +150,6 @@
}
});
- final PreferenceManager preferenceManager = getPreferenceManager();
-
PreferenceCategory categoryRecentLocationRequests =
(PreferenceCategory) root.findPreference(KEY_RECENT_LOCATION_REQUESTS);
RecentLocationApps recentApps = new RecentLocationApps(activity, mStatsHelper);
@@ -172,6 +174,9 @@
mReceiver = new BroadcastReceiver() {
@Override
public void onReceive(Context context, Intent intent) {
+ if (Log.isLoggable(TAG, Log.DEBUG)) {
+ Log.d(TAG, "Received settings change intent: " + intent);
+ }
injector.reloadStatusMessages();
}
};
diff --git a/src/com/android/settings/location/SettingsInjector.java b/src/com/android/settings/location/SettingsInjector.java
index bdff37d..532304d 100644
--- a/src/com/android/settings/location/SettingsInjector.java
+++ b/src/com/android/settings/location/SettingsInjector.java
@@ -31,18 +31,20 @@
import android.os.Message;
import android.os.Messenger;
import android.preference.Preference;
-import android.text.TextUtils;
import android.util.AttributeSet;
import android.util.Log;
import android.util.Xml;
+import com.android.settings.R;
import org.xmlpull.v1.XmlPullParser;
import org.xmlpull.v1.XmlPullParserException;
-import com.android.settings.R;
-
import java.io.IOException;
import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.LinkedHashSet;
import java.util.List;
+import java.util.Set;
/**
* Adds the preferences specified by the {@link InjectedSetting} objects to a preference group.
@@ -59,7 +61,7 @@
* {@link SettingInjectorService#UPDATE_INTENT}.
*/
class SettingsInjector {
- private static final String TAG = "SettingsInjector";
+ static final String TAG = "SettingsInjector";
private static final long INJECTED_STATUS_UPDATE_TIMEOUT_MILLIS = 1000;
@@ -79,11 +81,35 @@
*/
public static final String ATTRIBUTES_NAME = "injected-location-setting";
- private Context mContext;
- private StatusLoader mLoader = null;
+ /**
+ * {@link Message#what} value for starting to load status values
+ * in case we aren't already in the process of loading them.
+ */
+ private static final int WHAT_RELOAD = 1;
+
+ /**
+ * {@link Message#what} value sent after receiving a status message.
+ */
+ private static final int WHAT_RECEIVED_STATUS = 2;
+
+ /**
+ * {@link Message#what} value sent after the timeout waiting for a status message.
+ */
+ private static final int WHAT_TIMEOUT = 3;
+
+ private final Context mContext;
+
+ /**
+ * The settings that were injected
+ */
+ private final Set<Setting> mSettings;
+
+ private final Handler mHandler;
public SettingsInjector(Context context) {
mContext = context;
+ mSettings = new HashSet<Setting>();
+ mHandler = new StatusLoadingHandler();
}
/**
@@ -168,6 +194,9 @@
}
}
+ /**
+ * Returns an immutable representation of the static attributes for the setting, or null.
+ */
private static InjectedSetting parseAttributes(
String packageName, String className, Resources res, AttributeSet attrs) {
@@ -175,86 +204,22 @@
try {
// Note that to help guard against malicious string injection, we do not allow dynamic
// specification of the label (setting title)
- final int labelId = sa.getResourceId(
- android.R.styleable.InjectedLocationSetting_label, 0);
final String label = sa.getString(android.R.styleable.InjectedLocationSetting_label);
final int iconId = sa.getResourceId(
android.R.styleable.InjectedLocationSetting_icon, 0);
final String settingsActivity =
sa.getString(android.R.styleable.InjectedLocationSetting_settingsActivity);
if (Log.isLoggable(TAG, Log.DEBUG)) {
- Log.d(TAG, "parsed labelId: " + labelId + ", label: " + label
- + ", iconId: " + iconId);
+ Log.d(TAG, "parsed label: " + label + ", iconId: " + iconId
+ + ", settingsActivity: " + settingsActivity);
}
- if (labelId == 0 || TextUtils.isEmpty(label) || TextUtils.isEmpty(settingsActivity)) {
- return null;
- }
- return new InjectedSetting(packageName, className,
+ return InjectedSetting.newInstance(packageName, className,
label, iconId, settingsActivity);
} finally {
sa.recycle();
}
}
- private final class StatusLoader {
- private final Intent mIntent;
- private final StatusLoader mPrev;
-
- private boolean mLoaded = false;
-
- /**
- * Creates a loader and chains with the previous loader.
- */
- public StatusLoader(Intent intent, StatusLoader prev) {
- mIntent = intent;
- mPrev = prev;
- }
-
- /**
- * Clears the loaded flag for the whole chain.
- */
- private void setNotLoaded() {
- mLoaded = false;
- if (mPrev != null) {
- mPrev.setNotLoaded();
- }
- }
-
- /**
- * Reloads the whole chain.
- */
- public void reload() {
- setNotLoaded();
- loadIfNotLoaded();
- }
-
- /**
- * If the current message hasn't been loaded, loads the status messages
- * and set time out for the next message.
- */
- public void loadIfNotLoaded() {
- if (mLoaded) {
- return;
- }
-
- mContext.startService(mIntent);
- if (mPrev != null) {
- Handler handler = new Handler() {
- @Override
- public void handleMessage(Message msg) {
- // Continue with the next item in the chain.
- mPrev.loadIfNotLoaded();
- }
- };
- // Ensure that we start loading the previous setting in the chain if the current
- // setting hasn't loaded before the timeout
- handler.sendMessageDelayed(
- Message.obtain(handler), INJECTED_STATUS_UPDATE_TIMEOUT_MILLIS);
- }
- mLoaded = true;
- }
- }
-
/**
* Gets a list of preferences that other apps have injected.
*
@@ -264,17 +229,12 @@
public List<Preference> getInjectedSettings() {
Iterable<InjectedSetting> settings = getSettings();
ArrayList<Preference> prefs = new ArrayList<Preference>();
- mLoader = null;
for (InjectedSetting setting : settings) {
Preference pref = addServiceSetting(prefs, setting);
- Intent intent = createUpdatingIntent(pref, setting, mLoader);
- mLoader = new StatusLoader(intent, mLoader);
+ mSettings.add(new Setting(setting, pref));
}
- // Start a thread to load each list item status.
- if (mLoader != null) {
- mLoader.loadIfNotLoaded();
- }
+ reloadStatusMessages();
return prefs;
}
@@ -283,9 +243,10 @@
* Reloads the status messages for all the preference items.
*/
public void reloadStatusMessages() {
- if (mLoader != null) {
- mLoader.reload();
+ if (Log.isLoggable(TAG, Log.DEBUG)) {
+ Log.d(TAG, "reloadingStatusMessages: " + mSettings);
}
+ mHandler.sendMessage(mHandler.obtainMessage(WHAT_RELOAD));
}
/**
@@ -308,33 +269,141 @@
}
/**
- * Creates an Intent to ask the receiver for the current status for the setting, and display it
- * when it replies.
+ * Loads the setting status values one at a time. Each load starts a subclass of {@link
+ * SettingInjectorService}, so to reduce memory pressure we don't want to load too many at
+ * once.
*/
- private static Intent createUpdatingIntent(
- final Preference pref, final InjectedSetting info, final StatusLoader prev) {
- final Intent receiverIntent = info.getServiceIntent();
- Handler handler = new Handler() {
- @Override
- public void handleMessage(Message msg) {
- Bundle bundle = msg.getData();
- String status = bundle.getString(SettingInjectorService.STATUS_KEY);
- boolean enabled = bundle.getBoolean(SettingInjectorService.ENABLED_KEY, true);
- if (Log.isLoggable(TAG, Log.DEBUG)) {
- Log.d(TAG, info + ": received " + msg + ", bundle: " + bundle);
- }
- pref.setSummary(status);
- pref.setEnabled(enabled);
- if (prev != null) {
- prev.loadIfNotLoaded();
- }
+ private final class StatusLoadingHandler extends Handler {
+
+ /**
+ * Settings whose status values need to be loaded. A set is used to prevent redundant loads
+ * even if {@link #reloadStatusMessages()} is called many times in rapid succession (for
+ * example, if we receive a lot of
+ * {@link android.location.SettingInjectorService#UPDATE_INTENT} broadcasts).
+ * <p/>
+ * We use a linked hash set to ensure that when {@link #reloadStatusMessages()} is called,
+ * any settings that haven't been loaded yet will finish loading before any already-loaded
+ * messages are loaded again.
+ */
+ private LinkedHashSet<Setting> mSettingsToLoad = new LinkedHashSet<Setting>();
+
+ /**
+ * Whether we're in the middle of loading settings.
+ */
+ private boolean mLoading;
+
+ @Override
+ public void handleMessage(Message msg) {
+ if (Log.isLoggable(TAG, Log.DEBUG)) {
+ Log.d(TAG, "handleMessage start: " + msg + ", mSettingsToLoad: " + mSettingsToLoad);
}
- };
- Messenger messenger = new Messenger(handler);
- receiverIntent.putExtra(SettingInjectorService.MESSENGER_KEY, messenger);
- if (Log.isLoggable(TAG, Log.DEBUG)) {
- Log.d(TAG, info + ": sending rcv-intent: " + receiverIntent + ", handler: " + handler);
+
+ switch (msg.what) {
+ case WHAT_RELOAD:
+ mSettingsToLoad.addAll(mSettings);
+ if (mLoading) {
+ // Already waiting for a service to return its status, don't ask a new one
+ return;
+ }
+ mLoading = true;
+ break;
+ case WHAT_TIMEOUT:
+ if (Log.isLoggable(TAG, Log.WARN)) {
+ final Setting setting = (Setting) msg.obj;
+ setting.timedOut = true;
+ Log.w(TAG, "Timed out trying to get status for: " + setting);
+ }
+ break;
+ case WHAT_RECEIVED_STATUS:
+ final Setting setting = (Setting) msg.obj;
+ if (setting.timedOut) {
+ // We've already restarted retrieving the next setting, don't start another
+ return;
+ }
+
+ // Received the setting without timeout, clear any previous timed out status
+ setting.timedOut = false;
+ break;
+ default:
+ throw new IllegalArgumentException("Unexpected what: " + msg);
+ }
+
+ // Remove the next setting to load from the queue, if any
+ Iterator<Setting> iter = mSettingsToLoad.iterator();
+ if (!iter.hasNext()) {
+ mLoading = false;
+ return;
+ }
+ Setting setting = iter.next();
+ iter.remove();
+
+ // Request the status value
+ Intent intent = setting.createUpdatingIntent();
+ mContext.startService(intent);
+
+ // Ensure that if receiving the status value takes too long, we start loading the
+ // next value anyway
+ Message timeoutMsg = obtainMessage(WHAT_TIMEOUT, setting);
+ removeMessages(WHAT_TIMEOUT);
+ sendMessageDelayed(timeoutMsg, INJECTED_STATUS_UPDATE_TIMEOUT_MILLIS);
+
+ if (Log.isLoggable(TAG, Log.DEBUG)) {
+ Log.d(TAG, "handleMessage end: " + msg + ", mSettingsToLoad: " + mSettingsToLoad);
+ }
}
- return receiverIntent;
+ }
+
+ /**
+ * Represents an injected setting and the corresponding preference.
+ */
+ private final class Setting {
+
+ public final InjectedSetting setting;
+ public final Preference preference;
+ public boolean timedOut = false;
+
+ private Setting(InjectedSetting setting, Preference preference) {
+ this.setting = setting;
+ this.preference = preference;
+ }
+
+ @Override
+ public String toString() {
+ return "Setting{" +
+ "setting=" + setting +
+ ", preference=" + preference +
+ ", timedOut=" + timedOut +
+ '}';
+ }
+
+ /**
+ * Creates an Intent to ask the receiver for the current status for the setting, and display
+ * it when it replies.
+ */
+ public Intent createUpdatingIntent() {
+ final Intent receiverIntent = setting.getServiceIntent();
+ Handler handler = new Handler() {
+ @Override
+ public void handleMessage(Message msg) {
+ Bundle bundle = msg.getData();
+ String status = bundle.getString(SettingInjectorService.STATUS_KEY);
+ boolean enabled = bundle.getBoolean(SettingInjectorService.ENABLED_KEY, true);
+ if (Log.isLoggable(TAG, Log.DEBUG)) {
+ Log.d(TAG, setting + ": received " + msg + ", bundle: " + bundle);
+ }
+ preference.setSummary(status);
+ preference.setEnabled(enabled);
+ mHandler.sendMessage(
+ mHandler.obtainMessage(WHAT_RECEIVED_STATUS, Setting.this));
+ }
+ };
+ Messenger messenger = new Messenger(handler);
+ receiverIntent.putExtra(SettingInjectorService.MESSENGER_KEY, messenger);
+ if (Log.isLoggable(TAG, Log.DEBUG)) {
+ Log.d(TAG, setting + ": sending rcv-intent: " + receiverIntent
+ + ", handler: " + handler);
+ }
+ return receiverIntent;
+ }
}
}