From 21aed344fc1836de952b3ce7e4bee0d046f2def4 Mon Sep 17 00:00:00 2001 From: Matthew Fritze Date: Wed, 16 Aug 2017 15:45:52 -0700 Subject: [PATCH] Prevent race condition between SavedQuery and Query Loader SearchFragment and SavedQueryLoader share a loader manager and had been using the same loader id for different loaders. Occaisionally (in monkey tests) two loaders with the same IDs would be started and crash when they finished. The loaders now have different IDs. Change-Id: I11e9b7365605fbcef44cf7d2323183415422f5c8 Fixes: 64756515 Test: robotests --- .../settings/search/SavedQueryController.java | 26 ++++---- .../settings/search/SearchFragment.java | 60 ++++++++++--------- .../settings/search/SearchFragmentTest.java | 4 +- 3 files changed, 48 insertions(+), 42 deletions(-) diff --git a/src/com/android/settings/search/SavedQueryController.java b/src/com/android/settings/search/SavedQueryController.java index 545b4150105..eaba3d6bba4 100644 --- a/src/com/android/settings/search/SavedQueryController.java +++ b/src/com/android/settings/search/SavedQueryController.java @@ -34,10 +34,6 @@ public class SavedQueryController implements LoaderManager.LoaderCallbacks, MenuItem.OnMenuItemClickListener { // TODO: make a generic background task manager to handle one-off tasks like this one. - - private static final int LOADER_ID_SAVE_QUERY_TASK = 0; - private static final int LOADER_ID_REMOVE_QUERY_TASK = 1; - private static final int LOADER_ID_SAVED_QUERIES = 2; private static final String ARG_QUERY = "remove_query"; private static final String TAG = "SearchSavedQueryCtrl"; @@ -60,11 +56,11 @@ public class SavedQueryController implements LoaderManager.LoaderCallbacks, @Override public Loader onCreateLoader(int id, Bundle args) { switch (id) { - case LOADER_ID_SAVE_QUERY_TASK: + case SearchFragment.SearchLoaderId.SAVE_QUERY_TASK: return new SavedQueryRecorder(mContext, args.getString(ARG_QUERY)); - case LOADER_ID_REMOVE_QUERY_TASK: + case SearchFragment.SearchLoaderId.REMOVE_QUERY_TASK: return new SavedQueryRemover(mContext); - case LOADER_ID_SAVED_QUERIES: + case SearchFragment.SearchLoaderId.SAVED_QUERIES: return mSearchFeatureProvider.getSavedQueryLoader(mContext); } return null; @@ -73,10 +69,11 @@ public class SavedQueryController implements LoaderManager.LoaderCallbacks, @Override public void onLoadFinished(Loader loader, Object data) { switch (loader.getId()) { - case LOADER_ID_REMOVE_QUERY_TASK: - mLoaderManager.restartLoader(LOADER_ID_SAVED_QUERIES, null, this); + case SearchFragment.SearchLoaderId.REMOVE_QUERY_TASK: + mLoaderManager.restartLoader(SearchFragment.SearchLoaderId.SAVED_QUERIES, + null /* args */, this /* callback */); break; - case LOADER_ID_SAVED_QUERIES: + case SearchFragment.SearchLoaderId.SAVED_QUERIES: if (SettingsSearchIndexablesProvider.DEBUG) { Log.d(TAG, "Saved queries loaded"); } @@ -107,7 +104,8 @@ public class SavedQueryController implements LoaderManager.LoaderCallbacks, public void saveQuery(String query) { final Bundle args = new Bundle(); args.putString(ARG_QUERY, query); - mLoaderManager.restartLoader(LOADER_ID_SAVE_QUERY_TASK, args, this); + mLoaderManager.restartLoader(SearchFragment.SearchLoaderId.SAVE_QUERY_TASK, args, + this /* callback */); } /** @@ -115,13 +113,15 @@ public class SavedQueryController implements LoaderManager.LoaderCallbacks, */ public void removeQueries() { final Bundle args = new Bundle(); - mLoaderManager.restartLoader(LOADER_ID_REMOVE_QUERY_TASK, args, this); + mLoaderManager.restartLoader(SearchFragment.SearchLoaderId.REMOVE_QUERY_TASK, args, + this /* callback */); } public void loadSavedQueries() { if (SettingsSearchIndexablesProvider.DEBUG) { Log.d(TAG, "loading saved queries"); } - mLoaderManager.restartLoader(LOADER_ID_SAVED_QUERIES, null, this); + mLoaderManager.restartLoader(SearchFragment.SearchLoaderId.SAVED_QUERIES, null /* args */, + this /* callback */); } } diff --git a/src/com/android/settings/search/SearchFragment.java b/src/com/android/settings/search/SearchFragment.java index ccc4d61ff73..092404c7690 100644 --- a/src/com/android/settings/search/SearchFragment.java +++ b/src/com/android/settings/search/SearchFragment.java @@ -77,15 +77,19 @@ public class SearchFragment extends InstrumentedFragment implements SearchView.O private static final String STATE_NEVER_ENTERED_QUERY = "state_never_entered_query"; private static final String STATE_RESULT_CLICK_COUNT = "state_result_click_count"; - // Loader IDs - @VisibleForTesting - static final int LOADER_ID_DATABASE = 1; - @VisibleForTesting - static final int LOADER_ID_INSTALLED_APPS = 2; - @VisibleForTesting - static final int LOADER_ID_ACCESSIBILITY_SERVICES = 3; - @VisibleForTesting - static final int LOADER_ID_INPUT_DEVICES = 4; + static final class SearchLoaderId { + // Search Query IDs + public static final int DATABASE = 1; + public static final int INSTALLED_APPS = 2; + public static final int ACCESSIBILITY_SERVICES = 3; + public static final int INPUT_DEVICES = 4; + + // Saved Query IDs + public static final int SAVE_QUERY_TASK = 5; + public static final int REMOVE_QUERY_TASK = 6; + public static final int SAVED_QUERIES = 7; + } + private static final int NUM_QUERY_LOADERS = 4; @@ -283,10 +287,10 @@ public class SearchFragment extends InstrumentedFragment implements SearchView.O if (isEmptyQuery) { final LoaderManager loaderManager = getLoaderManager(); - loaderManager.destroyLoader(LOADER_ID_DATABASE); - loaderManager.destroyLoader(LOADER_ID_INSTALLED_APPS); - loaderManager.destroyLoader(LOADER_ID_ACCESSIBILITY_SERVICES); - loaderManager.destroyLoader(LOADER_ID_INPUT_DEVICES); + loaderManager.destroyLoader(SearchLoaderId.DATABASE); + loaderManager.destroyLoader(SearchLoaderId.INSTALLED_APPS); + loaderManager.destroyLoader(SearchLoaderId.ACCESSIBILITY_SERVICES); + loaderManager.destroyLoader(SearchLoaderId.INPUT_DEVICES); mShowingSavedQuery = true; mSavedQueryController.loadSavedQueries(); mSearchFeatureProvider.hideFeedbackButton(); @@ -311,13 +315,13 @@ public class SearchFragment extends InstrumentedFragment implements SearchView.O final Activity activity = getActivity(); switch (id) { - case LOADER_ID_DATABASE: + case SearchLoaderId.DATABASE: return mSearchFeatureProvider.getDatabaseSearchLoader(activity, mQuery); - case LOADER_ID_INSTALLED_APPS: + case SearchLoaderId.INSTALLED_APPS: return mSearchFeatureProvider.getInstalledAppSearchLoader(activity, mQuery); - case LOADER_ID_ACCESSIBILITY_SERVICES: + case SearchLoaderId.ACCESSIBILITY_SERVICES: return mSearchFeatureProvider.getAccessibilityServiceResultLoader(activity, mQuery); - case LOADER_ID_INPUT_DEVICES: + case SearchLoaderId.INPUT_DEVICES: return mSearchFeatureProvider.getInputDeviceResultLoader(activity, mQuery); default: return null; @@ -351,13 +355,13 @@ public class SearchFragment extends InstrumentedFragment implements SearchView.O mSavedQueryController.loadSavedQueries(); } else { final LoaderManager loaderManager = getLoaderManager(); - loaderManager.initLoader(LOADER_ID_DATABASE, null /* args */, this /* callback */); + loaderManager.initLoader(SearchLoaderId.DATABASE, null /* args */, this /* callback */); loaderManager.initLoader( - LOADER_ID_INSTALLED_APPS, null /* args */, this /* callback */); + SearchLoaderId.INSTALLED_APPS, null /* args */, this /* callback */); loaderManager.initLoader( - LOADER_ID_ACCESSIBILITY_SERVICES, null /* args */, this /* callback */); + SearchLoaderId.ACCESSIBILITY_SERVICES, null /* args */, this /* callback */); loaderManager.initLoader( - LOADER_ID_INPUT_DEVICES, null /* args */, this /* callback */); + SearchLoaderId.INPUT_DEVICES, null /* args */, this /* callback */); } requery(); @@ -395,12 +399,14 @@ public class SearchFragment extends InstrumentedFragment implements SearchView.O mShowingSavedQuery = false; final LoaderManager loaderManager = getLoaderManager(); mUnfinishedLoadersCount.set(NUM_QUERY_LOADERS); - loaderManager.restartLoader(LOADER_ID_DATABASE, null /* args */, this /* callback */); - loaderManager.restartLoader(LOADER_ID_INSTALLED_APPS, null /* args */, this /* callback */); - loaderManager.restartLoader(LOADER_ID_ACCESSIBILITY_SERVICES, null /* args */, - this /* callback */); - loaderManager.restartLoader(LOADER_ID_INPUT_DEVICES, null /* args */, - this /* callback */); + loaderManager.restartLoader( + SearchLoaderId.DATABASE, null /* args */, this /* callback */); + loaderManager.restartLoader( + SearchLoaderId.INSTALLED_APPS, null /* args */, this /* callback */); + loaderManager.restartLoader( + SearchLoaderId.ACCESSIBILITY_SERVICES, null /* args */, this /* callback */); + loaderManager.restartLoader( + SearchLoaderId.INPUT_DEVICES, null /* args */, this /* callback */); } public String getQuery() { diff --git a/tests/robotests/src/com/android/settings/search/SearchFragmentTest.java b/tests/robotests/src/com/android/settings/search/SearchFragmentTest.java index 71c31aa149b..d78a8184f52 100644 --- a/tests/robotests/src/com/android/settings/search/SearchFragmentTest.java +++ b/tests/robotests/src/com/android/settings/search/SearchFragmentTest.java @@ -410,9 +410,9 @@ public class SearchFragmentTest { fragment.onIndexingFinished(); - verify(loaderManager).initLoader(eq(SearchFragment.LOADER_ID_DATABASE), + verify(loaderManager).initLoader(eq(SearchFragment.SearchLoaderId.DATABASE), eq(null), any(LoaderManager.LoaderCallbacks.class)); - verify(loaderManager).initLoader(eq(SearchFragment.LOADER_ID_INSTALLED_APPS), + verify(loaderManager).initLoader(eq(SearchFragment.SearchLoaderId.INSTALLED_APPS), eq(null), any(LoaderManager.LoaderCallbacks.class)); }