From 3cd543fb85e917129a5d711b230d69e927f01e0e Mon Sep 17 00:00:00 2001 From: Matthew Fritze Date: Wed, 4 Apr 2018 09:56:53 -0700 Subject: [PATCH] Fix crash in non-indexable keys collection Some of the AmbientDisplay preference controllers were crashing when their isAvailable methods were being called by their fragment's search index providers, which meant that the entire collection of non-indexable keys failed. Thus, all search results were showing up. In the case of a secondary user, they were able to see developer options which crashed settings when clicked. There are two issues addressed in this cl. 1. Fix the crashes so the non-indexable keys collection works 2. Contain each fragment's collection, so that if a fragment does crash, the damage is minimized. Part 1 is checking that the config in isAvailable is not null, and creating one if so. Part 2 is fixed by surrounding the collection of non-indexable keys in a try-catch, with an option in the catch to re-throw the error if a system property is set. Thus, in a new pre-submit instrumentation test, we can and docheck if any of the fragments crash when collecting non-indexable keys. Change-Id: I820bd9cb2649aa6faff7f82fcf575a62e41dc4fc Fixes: 77486668 Test: atest NonIndexableCrashTest, robotests --- ...ntDisplayAlwaysOnPreferenceController.java | 3 ++ ...playNotificationsPreferenceController.java | 3 ++ .../SettingsSearchIndexablesProvider.java | 28 ++++++++++++++++++- .../SettingsSearchIndexablesProviderTest.java | 25 ++++++++++++++++- 4 files changed, 57 insertions(+), 2 deletions(-) diff --git a/src/com/android/settings/display/AmbientDisplayAlwaysOnPreferenceController.java b/src/com/android/settings/display/AmbientDisplayAlwaysOnPreferenceController.java index e0efaacb87c..5de49bb3ea5 100644 --- a/src/com/android/settings/display/AmbientDisplayAlwaysOnPreferenceController.java +++ b/src/com/android/settings/display/AmbientDisplayAlwaysOnPreferenceController.java @@ -47,6 +47,9 @@ public class AmbientDisplayAlwaysOnPreferenceController extends TogglePreference @Override public int getAvailabilityStatus() { + if (mConfig == null) { + mConfig = new AmbientDisplayConfiguration(mContext); + } return isAvailable(mConfig) ? AVAILABLE : DISABLED_UNSUPPORTED; } diff --git a/src/com/android/settings/display/AmbientDisplayNotificationsPreferenceController.java b/src/com/android/settings/display/AmbientDisplayNotificationsPreferenceController.java index ab769e18252..9cff0884b27 100644 --- a/src/com/android/settings/display/AmbientDisplayNotificationsPreferenceController.java +++ b/src/com/android/settings/display/AmbientDisplayNotificationsPreferenceController.java @@ -81,6 +81,9 @@ public class AmbientDisplayNotificationsPreferenceController extends @Override public int getAvailabilityStatus() { + if (mConfig == null) { + mConfig = new AmbientDisplayConfiguration(mContext); + } return mConfig.pulseOnNotificationAvailable() ? AVAILABLE : DISABLED_UNSUPPORTED; } diff --git a/src/com/android/settings/search/SettingsSearchIndexablesProvider.java b/src/com/android/settings/search/SettingsSearchIndexablesProvider.java index 3ef1b8550af..b9134e5c985 100644 --- a/src/com/android/settings/search/SettingsSearchIndexablesProvider.java +++ b/src/com/android/settings/search/SettingsSearchIndexablesProvider.java @@ -41,6 +41,7 @@ import static android.provider.SearchIndexablesContract.INDEXABLES_RAW_COLUMNS; import static android.provider.SearchIndexablesContract.INDEXABLES_XML_RES_COLUMNS; import static android.provider.SearchIndexablesContract.NON_INDEXABLES_KEYS_COLUMNS; import static android.provider.SearchIndexablesContract.SITE_MAP_COLUMNS; + import static com.android.settings.dashboard.DashboardFragmentRegistry.CATEGORY_KEY_TO_PARENT_MAP; import android.content.Context; @@ -63,7 +64,16 @@ import java.util.Collection; import java.util.List; public class SettingsSearchIndexablesProvider extends SearchIndexablesProvider { + public static final boolean DEBUG = false; + + /** + * Flag for a system property which checks if we should crash if there are issues in the + * indexing pipeline. + */ + public static final String SYSPROP_CRASH_ON_ERROR = + "debug.com.android.settings.search.crash_on_error"; + private static final String TAG = "SettingsSearchProvider"; private static final Collection INVALID_KEYS; @@ -183,7 +193,23 @@ public class SettingsSearchIndexablesProvider extends SearchIndexablesProvider { final long startTime = System.currentTimeMillis(); Indexable.SearchIndexProvider provider = DatabaseIndexingUtils.getSearchIndexProvider( clazz); - List providerNonIndexableKeys = provider.getNonIndexableKeys(context); + + List providerNonIndexableKeys; + try { + providerNonIndexableKeys = provider.getNonIndexableKeys(context); + } catch (Exception e) { + // Catch a generic crash. In the absence of the catch, the background thread will + // silently fail anyway, so we aren't losing information by catching the exception. + // We crash when the system property exists so that we can test if crashes need to + // be fixed. + // The gain is that if there is a crash in a specific controller, we don't lose all + // non-indexable keys, but we can still find specific crashes in development. + if (System.getProperty(SYSPROP_CRASH_ON_ERROR) != null) { + throw new RuntimeException(e); + } + Log.e(TAG, "Error trying to get non-indexable keys from: " + clazz.getName() , e); + continue; + } if (providerNonIndexableKeys == null || providerNonIndexableKeys.isEmpty()) { if (DEBUG) { diff --git a/tests/unit/src/com/android/settings/search/SettingsSearchIndexablesProviderTest.java b/tests/unit/src/com/android/settings/search/SettingsSearchIndexablesProviderTest.java index aed94a0ea67..3659fdbcf37 100644 --- a/tests/unit/src/com/android/settings/search/SettingsSearchIndexablesProviderTest.java +++ b/tests/unit/src/com/android/settings/search/SettingsSearchIndexablesProviderTest.java @@ -21,11 +21,13 @@ import static com.google.common.truth.Truth.assertThat; import android.content.Context; import android.database.Cursor; import android.net.Uri; +import android.platform.test.annotations.Presubmit; import android.provider.SearchIndexablesContract; import android.support.test.InstrumentationRegistry; import android.support.test.filters.SmallTest; import android.support.test.runner.AndroidJUnit4; +import org.junit.After; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -36,12 +38,16 @@ public class SettingsSearchIndexablesProviderTest { private Context mContext; - @Before public void setUp() { mContext = InstrumentationRegistry.getTargetContext(); } + @After + public void cleanUp() { + System.clearProperty(SettingsSearchIndexablesProvider.SYSPROP_CRASH_ON_ERROR); + } + @Test public void testSiteMapPairsFetched() { final Uri uri = Uri.parse("content://" + mContext.getPackageName() + "/" + @@ -59,4 +65,21 @@ public class SettingsSearchIndexablesProviderTest { .isNotEmpty(); } } + + /** + * All {@link Indexable.SearchIndexProvider} should collect a list of non-indexable keys + * without crashing. This test enables crashing of individual providers in the indexing pipeline + * and checks that there are no crashes. + */ + @Test + @Presubmit + public void nonIndexableKeys_shouldNotCrash() { + // Allow crashes in the indexing pipeline. + System.setProperty(SettingsSearchIndexablesProvider.SYSPROP_CRASH_ON_ERROR, + "enabled"); + + final Uri uri = Uri.parse("content://" + mContext.getPackageName() + "/" + + SearchIndexablesContract.NON_INDEXABLES_KEYS_PATH); + mContext.getContentResolver().query(uri, null, null, null, null); + } }