From a6d7b8c9d9dd74348e8773ea6f6db3cd57e0c76f Mon Sep 17 00:00:00 2001 From: Chaohui Wang Date: Wed, 31 Jul 2024 14:58:35 +0800 Subject: [PATCH] Improve the robustness of updateNonIndexableKeys - Isolate preference controller crash in the same page Current if one search item throw exception, entire page's search will broken, can isolate issue by controller to improve. - Expose preference controller crash in debuggable build Current the search index exception is silently ignored, expose the exception in debuggable builds to expose issue earlier. - Change default to hide items instead of shown when crash Fix: 352455031 Fix: 352455432 Fix: 352455540 Flag: EXEMPT bug fix Test: manual - Settings Search Change-Id: I2cdbd58543be8fe6617e42d9c02789791186f53b --- .../search/BaseSearchIndexProvider.java | 54 ++++++++++++++----- .../SettingsSearchIndexablesProvider.java | 16 +++--- 2 files changed, 48 insertions(+), 22 deletions(-) diff --git a/src/com/android/settings/search/BaseSearchIndexProvider.java b/src/com/android/settings/search/BaseSearchIndexProvider.java index cc05270cc0e..b6a91677d95 100644 --- a/src/com/android/settings/search/BaseSearchIndexProvider.java +++ b/src/com/android/settings/search/BaseSearchIndexProvider.java @@ -21,9 +21,11 @@ import static com.android.settings.core.PreferenceXmlParserUtils.METADATA_SEARCH import static com.android.settings.core.PreferenceXmlParserUtils.MetadataFlag.FLAG_INCLUDE_PREF_SCREEN; import static com.android.settings.core.PreferenceXmlParserUtils.MetadataFlag.FLAG_NEED_KEY; import static com.android.settings.core.PreferenceXmlParserUtils.MetadataFlag.FLAG_NEED_SEARCHABLE; +import static com.android.settings.search.SettingsSearchIndexablesProvider.SYSPROP_CRASH_ON_ERROR; import android.annotation.XmlRes; import android.content.Context; +import android.os.Build; import android.os.Bundle; import android.provider.SearchIndexableResource; import android.util.Log; @@ -131,24 +133,48 @@ public class BaseSearchIndexProvider implements Indexable.SearchIndexProvider { return nonIndexableKeys; } nonIndexableKeys.addAll(getNonIndexableKeysFromXml(context, false /* suppressAllPage */)); + updateNonIndexableKeysFromControllers(context, nonIndexableKeys); + return nonIndexableKeys; + } + + private void updateNonIndexableKeysFromControllers( + Context context, List nonIndexableKeys) { final List controllers = getPreferenceControllers(context); - if (controllers != null && !controllers.isEmpty()) { + if (controllers != null) { for (AbstractPreferenceController controller : controllers) { - if (controller instanceof PreferenceControllerMixin) { - ((PreferenceControllerMixin) controller) - .updateNonIndexableKeys(nonIndexableKeys); - } else if (controller instanceof BasePreferenceController) { - ((BasePreferenceController) controller).updateNonIndexableKeys( - nonIndexableKeys); - } else { - Log.e(TAG, controller.getClass().getName() - + " must implement " + PreferenceControllerMixin.class.getName() - + " treating the key non-indexable"); - nonIndexableKeys.add(controller.getPreferenceKey()); - } + updateNonIndexableKeysFromController(nonIndexableKeys, controller); } } - return nonIndexableKeys; + } + + private static void updateNonIndexableKeysFromController( + List nonIndexableKeys, AbstractPreferenceController controller) { + try { + if (controller instanceof PreferenceControllerMixin controllerMixin) { + controllerMixin.updateNonIndexableKeys(nonIndexableKeys); + } else if (controller instanceof BasePreferenceController basePreferenceController) { + basePreferenceController.updateNonIndexableKeys(nonIndexableKeys); + } else { + Log.e(TAG, controller.getClass().getName() + + " must implement " + PreferenceControllerMixin.class.getName() + + " treating the key non-indexable"); + nonIndexableKeys.add(controller.getPreferenceKey()); + } + } catch (Exception e) { + String msg = "Error trying to get non-indexable keys from: " + controller; + // 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 on debuggable build or 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 (Build.IS_DEBUGGABLE || System.getProperty(SYSPROP_CRASH_ON_ERROR) != null) { + throw new RuntimeException(msg, e); + } + Log.e(TAG, msg, e); + // When there is an error, treat the key as non-indexable. + nonIndexableKeys.add(controller.getPreferenceKey()); + } } public List getPreferenceControllers(Context context) { diff --git a/src/com/android/settings/search/SettingsSearchIndexablesProvider.java b/src/com/android/settings/search/SettingsSearchIndexablesProvider.java index 3b04186756f..cdcb323aae9 100644 --- a/src/com/android/settings/search/SettingsSearchIndexablesProvider.java +++ b/src/com/android/settings/search/SettingsSearchIndexablesProvider.java @@ -50,6 +50,7 @@ import android.content.Context; import android.database.Cursor; import android.database.MatrixCursor; import android.net.Uri; +import android.os.Build; import android.provider.SearchIndexableResource; import android.provider.SearchIndexablesContract; import android.provider.SearchIndexablesProvider; @@ -283,17 +284,16 @@ public class SettingsSearchIndexablesProvider extends SearchIndexablesProvider { try { providerNonIndexableKeys = provider.getNonIndexableKeys(context); } catch (Exception e) { + String msg = "Error trying to get non-indexable keys from: " + + bundle.getTargetClass().getName(); // 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); + // We crash on debuggable build or when the system property exists, so that we can + // test if crashes need to be fixed. + if (Build.IS_DEBUGGABLE || System.getProperty(SYSPROP_CRASH_ON_ERROR) != null) { + throw new RuntimeException(msg, e); } - Log.e(TAG, "Error trying to get non-indexable keys from: " - + bundle.getTargetClass().getName(), e); + Log.e(TAG, msg, e); continue; }