From 7b574cb8a1a3d0943392f2bf7180687eff8448ae Mon Sep 17 00:00:00 2001 From: Yohei Yukawa Date: Wed, 16 Mar 2016 17:22:22 -0700 Subject: [PATCH] Add more @NonNull/@Nullable to InputMethodSettings. This follows up to a previous CL [1] for Bug 26279466. It turns out that we have not clearly defined how nonexistent key should be handled in InputMethodSettings#getEnabledInputMethodsStr(), e.g. it returns "" for when mCopyOnWrite is true but returns null when mCopyOnWrite is false. Also, since InputMethodSettings now can revert changes made during mCopyOnWrite is true, the caller may also start receiving null in the following scenario. 1. call mSettings.switchCurrentUser(userId, true). 2. call mSettings.putEnabledInputMethodsStr(str) where str is non-null. 3. call mSettings.switchCurrentUser(userId, false). 4. call mSettings.getEnabledInputMethodsStr(). If the caller of getEnabledInputMethodsStr() has assumed that it would never return null, then it would start crashing due to NPE. With this CL, getEnabledInputMethodsStr() is marked to be @NonNull we should no longer see such kind of NPE. [1]: I9c6f9bb3d51174198e5f73588637f87ea0d90e11 68645a638ad1bfb734b2b0f56b17fe206bb891c5 Bug: 27687531 Change-Id: I169ad4957e68b65c64251b0849056195b8ca4911 --- .../inputmethod/InputMethodUtils.java | 48 +++++++++++++------ .../server/InputMethodManagerService.java | 1 + 2 files changed, 35 insertions(+), 14 deletions(-) diff --git a/core/java/com/android/internal/inputmethod/InputMethodUtils.java b/core/java/com/android/internal/inputmethod/InputMethodUtils.java index 90ee05ec944b3..9461ce686dbe5 100644 --- a/core/java/com/android/internal/inputmethod/InputMethodUtils.java +++ b/core/java/com/android/internal/inputmethod/InputMethodUtils.java @@ -854,7 +854,8 @@ public class InputMethodUtils { private final HashMap mCopyOnWriteDataStore = new HashMap<>(); private boolean mCopyOnWrite = false; - private String mEnabledInputMethodsStrCache; + @NonNull + private String mEnabledInputMethodsStrCache = ""; @UserIdInt private int mCurrentUserId; private int[] mCurrentProfileIds = new int[0]; @@ -949,7 +950,7 @@ public class InputMethodUtils { // TODO: mCurrentProfileIds should be updated here. } - private void putString(final String key, final String str) { + private void putString(@NonNull final String key, @Nullable final String str) { if (mCopyOnWrite) { mCopyOnWriteDataStore.put(key, str); } else { @@ -957,12 +958,15 @@ public class InputMethodUtils { } } - private String getString(final String key) { + @Nullable + private String getString(@NonNull final String key, @Nullable final String defaultValue) { + final String result; if (mCopyOnWrite && mCopyOnWriteDataStore.containsKey(key)) { - final String result = mCopyOnWriteDataStore.get(key); - return result != null ? result : ""; + result = mCopyOnWriteDataStore.get(key); + } else { + result = Settings.Secure.getStringForUser(mResolver, key, mCurrentUserId); } - return Settings.Secure.getStringForUser(mResolver, key, mCurrentUserId); + return result != null ? result : defaultValue; } private void putInt(final String key, final int value) { @@ -1124,16 +1128,24 @@ public class InputMethodUtils { return res; } - private void putEnabledInputMethodsStr(String str) { + private void putEnabledInputMethodsStr(@Nullable String str) { if (DEBUG) { Slog.d(TAG, "putEnabledInputMethodStr: " + str); } - putString(Settings.Secure.ENABLED_INPUT_METHODS, str); - mEnabledInputMethodsStrCache = str; + if (TextUtils.isEmpty(str)) { + // OK to coalesce to null, since getEnabledInputMethodsStr() can take care of the + // empty data scenario. + putString(Settings.Secure.ENABLED_INPUT_METHODS, null); + } else { + putString(Settings.Secure.ENABLED_INPUT_METHODS, str); + } + // TODO: Update callers of putEnabledInputMethodsStr to make str @NonNull. + mEnabledInputMethodsStrCache = (str != null ? str : ""); } + @NonNull public String getEnabledInputMethodsStr() { - mEnabledInputMethodsStrCache = getString(Settings.Secure.ENABLED_INPUT_METHODS); + mEnabledInputMethodsStrCache = getString(Settings.Secure.ENABLED_INPUT_METHODS, ""); if (DEBUG) { Slog.d(TAG, "getEnabledInputMethodsStr: " + mEnabledInputMethodsStrCache + ", " + mCurrentUserId); @@ -1187,11 +1199,17 @@ public class InputMethodUtils { saveSubtypeHistory(subtypeHistory, imeId, subtypeId); } - private void putSubtypeHistoryStr(String str) { + private void putSubtypeHistoryStr(@NonNull String str) { if (DEBUG) { Slog.d(TAG, "putSubtypeHistoryStr: " + str); } - putString(Settings.Secure.INPUT_METHODS_SUBTYPE_HISTORY, str); + if (TextUtils.isEmpty(str)) { + // OK to coalesce to null, since getSubtypeHistoryStr() can take care of the empty + // data scenario. + putString(Settings.Secure.INPUT_METHODS_SUBTYPE_HISTORY, null); + } else { + putString(Settings.Secure.INPUT_METHODS_SUBTYPE_HISTORY, str); + } } public Pair getLastInputMethodAndSubtypeLocked() { @@ -1308,8 +1326,9 @@ public class InputMethodUtils { return imsList; } + @NonNull private String getSubtypeHistoryStr() { - final String history = getString(Settings.Secure.INPUT_METHODS_SUBTYPE_HISTORY); + final String history = getString(Settings.Secure.INPUT_METHODS_SUBTYPE_HISTORY, ""); if (DEBUG) { Slog.d(TAG, "getSubtypeHistoryStr: " + history); } @@ -1332,8 +1351,9 @@ public class InputMethodUtils { putInt(Settings.Secure.SELECTED_INPUT_METHOD_SUBTYPE, subtypeId); } + @Nullable public String getSelectedInputMethod() { - final String imi = getString(Settings.Secure.DEFAULT_INPUT_METHOD); + final String imi = getString(Settings.Secure.DEFAULT_INPUT_METHOD, null); if (DEBUG) { Slog.d(TAG, "getSelectedInputMethodStr: " + imi); } diff --git a/services/core/java/com/android/server/InputMethodManagerService.java b/services/core/java/com/android/server/InputMethodManagerService.java index 9b8f2d2d7d573..00a49bdb72731 100644 --- a/services/core/java/com/android/server/InputMethodManagerService.java +++ b/services/core/java/com/android/server/InputMethodManagerService.java @@ -456,6 +456,7 @@ public class InputMethodManagerService extends IInputMethodManager.Stub class SettingsObserver extends ContentObserver { int mUserId; boolean mRegistered = false; + @NonNull String mLastEnabled = ""; /**