From bcb76351b311bd8e23c277337a98cba1eb711b22 Mon Sep 17 00:00:00 2001 From: Doris Ling Date: Wed, 22 Nov 2017 17:29:21 -0800 Subject: [PATCH] Fix ConcurrentModificationException in SummaryLoader. When the dashboard summary is being initialized, it will rebuild the UI while the summary loader tries to to go through the tiles to update the summary. Both is being done on a separate backgroud thread, and it will run into concurrent modification issue if the thread is being swapped while one is looping through the list. Instead of letting clients access the list of tiles directly, add a getter method in DashboardCategory to get a copy of the list of tiles for all read-only operations. Change-Id: I479669abd8d1d0a8ee9a4113d8ad2244da56f4d8 Fixes: 69677575 Test: make RunSettingsRoboTests --- .../settings/dashboard/DashboardAdapter.java | 2 +- .../settings/dashboard/DashboardData.java | 5 ++-- .../DashboardFeatureProviderImpl.java | 2 +- .../settings/dashboard/DashboardFragment.java | 2 +- .../settings/dashboard/SiteMapManager.java | 2 +- .../settings/dashboard/SummaryLoader.java | 13 +++++----- .../SettingsSearchIndexablesProvider.java | 2 +- .../dashboard/DashboardAdapterTest.java | 24 +++++++------------ .../settings/dashboard/DashboardDataTest.java | 9 +++---- .../DashboardFeatureProviderImplTest.java | 2 +- .../dashboard/DashboardFragmentTest.java | 9 ++++--- 11 files changed, 33 insertions(+), 39 deletions(-) diff --git a/src/com/android/settings/dashboard/DashboardAdapter.java b/src/com/android/settings/dashboard/DashboardAdapter.java index fd2adacc9fd..54a1eeb3fb1 100644 --- a/src/com/android/settings/dashboard/DashboardAdapter.java +++ b/src/com/android/settings/dashboard/DashboardAdapter.java @@ -477,7 +477,7 @@ public class DashboardAdapter extends RecyclerView.Adapter tiles = mCategory.getTiles(); + for (int j = 0; j < tiles.size(); j++) { + final Tile tile = tiles.get(j); addToItemList(tile, R.layout.dashboard_tile, Objects.hash(tile.title), true /* add */); } diff --git a/src/com/android/settings/dashboard/DashboardFeatureProviderImpl.java b/src/com/android/settings/dashboard/DashboardFeatureProviderImpl.java index bee822ab32f..9ef38b80f8c 100644 --- a/src/com/android/settings/dashboard/DashboardFeatureProviderImpl.java +++ b/src/com/android/settings/dashboard/DashboardFeatureProviderImpl.java @@ -91,7 +91,7 @@ public class DashboardFeatureProviderImpl implements DashboardFeatureProvider { Log.d(TAG, "NO dashboard tiles for " + TAG); return null; } - final List tiles = category.tiles; + final List tiles = category.getTiles(); if (tiles == null || tiles.isEmpty()) { Log.d(TAG, "tile list is empty, skipping category " + category.title); return null; diff --git a/src/com/android/settings/dashboard/DashboardFragment.java b/src/com/android/settings/dashboard/DashboardFragment.java index 3551d23dced..3e1e8818568 100644 --- a/src/com/android/settings/dashboard/DashboardFragment.java +++ b/src/com/android/settings/dashboard/DashboardFragment.java @@ -307,7 +307,7 @@ public abstract class DashboardFragment extends SettingsPreferenceFragment Log.d(TAG, "NO dashboard tiles for " + TAG); return; } - List tiles = category.tiles; + final List tiles = category.getTiles(); if (tiles == null) { Log.d(TAG, "tile list is empty, skipping category " + category.title); return; diff --git a/src/com/android/settings/dashboard/SiteMapManager.java b/src/com/android/settings/dashboard/SiteMapManager.java index 50d7a1892da..b54e06114b0 100644 --- a/src/com/android/settings/dashboard/SiteMapManager.java +++ b/src/com/android/settings/dashboard/SiteMapManager.java @@ -154,7 +154,7 @@ public class SiteMapManager { continue; } // Build parent-child mPairs for all children listed under this key. - for (Tile tile : category.tiles) { + for (Tile tile : category.getTiles()) { final String childTitle = tile.title.toString(); String childClass = null; if (tile.metaData != null) { diff --git a/src/com/android/settings/dashboard/SummaryLoader.java b/src/com/android/settings/dashboard/SummaryLoader.java index fe55be802a9..5af276c1343 100644 --- a/src/com/android/settings/dashboard/SummaryLoader.java +++ b/src/com/android/settings/dashboard/SummaryLoader.java @@ -216,7 +216,7 @@ public class SummaryLoader { if (category == null) { return; } - for (Tile tile : category.tiles) { + for (Tile tile : category.getTiles()) { final String key = mDashboardFeatureProvider.getDashboardKeyForTile(tile); if (mSummaryTextMap.containsKey(key)) { tile.summary = mSummaryTextMap.get(key); @@ -250,12 +250,13 @@ public class SummaryLoader { } private Tile getTileFromCategory(DashboardCategory category, ComponentName component) { - if (category == null || category.tiles == null) { + if (category == null || category.getTilesCount() == 0) { return null; } - final int tileCount = category.tiles.size(); + final List tiles = category.getTiles(); + final int tileCount = tiles.size(); for (int j = 0; j < tileCount; j++) { - final Tile tile = category.tiles.get(j); + final Tile tile = tiles.get(j); if (component.equals(tile.intent.getComponent())) { return tile; } @@ -291,10 +292,10 @@ public class SummaryLoader { case MSG_GET_CATEGORY_TILES_AND_SET_LISTENING: final DashboardCategory category = mDashboardFeatureProvider.getTilesForCategory(mCategoryKey); - if (category == null || category.tiles == null) { + if (category == null || category.getTilesCount() == 0) { return; } - final List tiles = category.tiles; + final List tiles = category.getTiles(); for (Tile tile : tiles) { makeProviderW(tile); } diff --git a/src/com/android/settings/search/SettingsSearchIndexablesProvider.java b/src/com/android/settings/search/SettingsSearchIndexablesProvider.java index 9e350829dd7..0c98b9c15ab 100644 --- a/src/com/android/settings/search/SettingsSearchIndexablesProvider.java +++ b/src/com/android/settings/search/SettingsSearchIndexablesProvider.java @@ -156,7 +156,7 @@ public class SettingsSearchIndexablesProvider extends SearchIndexablesProvider { continue; } // Build parent-child class pairs for all children listed under this key. - for (Tile tile : category.tiles) { + for (Tile tile : category.getTiles()) { String childClass = null; if (tile.metaData != null) { childClass = tile.metaData.getString( diff --git a/tests/robotests/src/com/android/settings/dashboard/DashboardAdapterTest.java b/tests/robotests/src/com/android/settings/dashboard/DashboardAdapterTest.java index c9469a8543c..fdb147083bf 100644 --- a/tests/robotests/src/com/android/settings/dashboard/DashboardAdapterTest.java +++ b/tests/robotests/src/com/android/settings/dashboard/DashboardAdapterTest.java @@ -222,14 +222,12 @@ public class DashboardAdapterTest { doReturn(mockTypedArray).when(mContext).obtainStyledAttributes(any(int[].class)); doReturn(0x89000000).when(mockTypedArray).getColor(anyInt(), anyInt()); - final DashboardCategory category = mock(DashboardCategory.class); - final List tiles = new ArrayList<>(); + final DashboardCategory category = new DashboardCategory(); final Icon mockIcon = mock(Icon.class); final Tile tile = new Tile(); tile.isIconTintable = true; tile.icon = mockIcon; - tiles.add(tile); - category.tiles = tiles; + category.addTile(tile); mDashboardAdapter.setCategory(category); @@ -250,10 +248,8 @@ public class DashboardAdapterTest { public void testBindConditionAndSuggestion_shouldSetSuggestionAdapterAndNoCrash() { mDashboardAdapter = new DashboardAdapter(mContext, null, null, null, null, null); final List suggestions = makeSuggestions("pkg1"); - final DashboardCategory category = mock(DashboardCategory.class); - final List tiles = new ArrayList<>(); - tiles.add(mock(Tile.class)); - category.tiles = tiles; + final DashboardCategory category = new DashboardCategory(); + category.addTile(mock(Tile.class)); mDashboardAdapter.setCategoriesAndSuggestions(category, suggestions); @@ -277,10 +273,8 @@ public class DashboardAdapterTest { public void testBindConditionAndSuggestion_v2_shouldSetSuggestionAdapterAndNoCrash() { mDashboardAdapter = new DashboardAdapter(mContext, null, null, null, null, null); final List suggestions = makeSuggestionsV2("pkg1"); - final DashboardCategory category = mock(DashboardCategory.class); - final List tiles = new ArrayList<>(); - tiles.add(mock(Tile.class)); - category.tiles = tiles; + final DashboardCategory category = new DashboardCategory(); + category.addTile(mock(Tile.class)); mDashboardAdapter.setSuggestionsV2(suggestions); @@ -310,10 +304,8 @@ public class DashboardAdapterTest { null /* SuggestionDismissController.Callback */); final List suggestions = new ArrayList<>(); - final DashboardCategory category = mock(DashboardCategory.class); - final List tiles = new ArrayList<>(); - tiles.add(mock(Tile.class)); - category.tiles = tiles; + final DashboardCategory category = new DashboardCategory(); + category.addTile(mock(Tile.class)); mDashboardAdapter.setCategoriesAndSuggestions(category, suggestions); final RecyclerView data = mock(RecyclerView.class); diff --git a/tests/robotests/src/com/android/settings/dashboard/DashboardDataTest.java b/tests/robotests/src/com/android/settings/dashboard/DashboardDataTest.java index a3483fb9057..33f379e8c29 100644 --- a/tests/robotests/src/com/android/settings/dashboard/DashboardDataTest.java +++ b/tests/robotests/src/com/android/settings/dashboard/DashboardDataTest.java @@ -57,13 +57,12 @@ public class DashboardDataTest { private DashboardData mDashboardDataWithOneConditions; private DashboardData mDashboardDataWithTwoConditions; private DashboardData mDashboardDataWithNoItems; + private DashboardCategory mDashboardCategory; @Mock private Tile mTestCategoryTile; @Mock private Tile mTestSuggestion; @Mock - private DashboardCategory mDashboardCategory; - @Mock private Condition mTestCondition; @Mock private Condition mSecondCondition; // condition used to test insert in DiffUtil @@ -72,6 +71,8 @@ public class DashboardDataTest { public void SetUp() { MockitoAnnotations.initMocks(this); + mDashboardCategory = new DashboardCategory(); + // Build suggestions final List suggestions = new ArrayList<>(); mTestSuggestion.title = TEST_SUGGESTION_TITLE; @@ -91,8 +92,8 @@ public class DashboardDataTest { // Build category mTestCategoryTile.title = TEST_CATEGORY_TILE_TITLE; mDashboardCategory.title = "test"; - mDashboardCategory.tiles = new ArrayList<>(); - mDashboardCategory.tiles.add(mTestCategoryTile); + + mDashboardCategory.addTile(mTestCategoryTile); // Build DashboardData mDashboardDataWithOneConditions = new DashboardData.Builder() diff --git a/tests/robotests/src/com/android/settings/dashboard/DashboardFeatureProviderImplTest.java b/tests/robotests/src/com/android/settings/dashboard/DashboardFeatureProviderImplTest.java index 559e98998e0..3d36e6ffcb3 100644 --- a/tests/robotests/src/com/android/settings/dashboard/DashboardFeatureProviderImplTest.java +++ b/tests/robotests/src/com/android/settings/dashboard/DashboardFeatureProviderImplTest.java @@ -431,7 +431,7 @@ public class DashboardFeatureProviderImplTest { mImpl = new DashboardFeatureProviderImpl(mActivity); ReflectionHelpers.setField(mImpl, "mCategoryManager", mCategoryManager); final DashboardCategory category = new DashboardCategory(); - category.tiles.add(new Tile()); + category.addTile(new Tile()); when(mCategoryManager .getTilesByCategory(any(Context.class), eq(CategoryKey.CATEGORY_HOMEPAGE))) .thenReturn(category); diff --git a/tests/robotests/src/com/android/settings/dashboard/DashboardFragmentTest.java b/tests/robotests/src/com/android/settings/dashboard/DashboardFragmentTest.java index 2ee18371b64..14f3078981c 100644 --- a/tests/robotests/src/com/android/settings/dashboard/DashboardFragmentTest.java +++ b/tests/robotests/src/com/android/settings/dashboard/DashboardFragmentTest.java @@ -64,9 +64,8 @@ public class DashboardFragmentTest { @Mock(answer = Answers.RETURNS_DEEP_STUBS) private Context mContext; @Mock - private DashboardCategory mDashboardCategory; - @Mock private FakeFeatureFactory mFakeFeatureFactory; + private DashboardCategory mDashboardCategory; private TestFragment mTestFragment; @Before @@ -74,8 +73,8 @@ public class DashboardFragmentTest { MockitoAnnotations.initMocks(this); FakeFeatureFactory.setupForTest(mContext); mFakeFeatureFactory = (FakeFeatureFactory) FeatureFactory.getFactory(mContext); - mDashboardCategory.tiles = new ArrayList<>(); - mDashboardCategory.tiles.add(new Tile()); + mDashboardCategory = new DashboardCategory(); + mDashboardCategory.addTile(new Tile()); mTestFragment = new TestFragment(ShadowApplication.getInstance().getApplicationContext()); when(mFakeFeatureFactory.dashboardFeatureProvider .getTilesForCategory(nullable(String.class))) @@ -117,7 +116,7 @@ public class DashboardFragmentTest { @Test public void displayTilesAsPreference_withEmptyCategory_shouldNotAddTiles() { - mDashboardCategory.tiles = null; + mDashboardCategory.removeTile(0); mTestFragment.onCreatePreferences(new Bundle(), "rootKey"); verify(mTestFragment.mScreen, never()).addPreference(nullable(Preference.class));