From 77fe8c1e9e0ca06ab723a471c97913ae127959b5 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Tue, 30 May 2017 14:39:14 -0600 Subject: [PATCH] Consistent "low storage" behavior. Fix several bugs related to storage accounting. Since getDataBytes() already includes cached data, we need to subtract it to avoid blaming apps for it. We also need to blame app code on someone, so we blame it on the current user. StorageStatsManager was fixed awhile back to only return the app code size on the requested storage volume, so we can remove the system app checks. Subtract "appBytes" from external storage accounting, since it's already been blamed elsewhere against specific apps. Pass along storage results from all users on the device, and subtract them all when estimating size of "system" data. To avoid embarrassing estimation bugs, make sure that "system" data is at least 1GB. Bug: 38008706 Test: cts-tradefed run commandAndExit cts-dev -m CtsJobSchedulerTestCases -t android.jobscheduler.cts.StorageConstraintTest Test: cts-tradefed run commandAndExit cts-dev -m CtsAppSecurityHostTestCases -t android.appsecurity.cts.StorageHostTest Change-Id: Ide1e6d0690e5ad4e751c87891f63ba1036434619 --- .../deviceinfo/StorageDashboardFragment.java | 2 +- .../deviceinfo/StorageProfileFragment.java | 3 +- .../storage/StorageAsyncLoader.java | 35 ++++++++--------- .../StorageItemPreferenceController.java | 38 ++++++++++++------- .../MusicViewHolderControllerTest.java | 2 +- .../StorageProfileFragmentTest.java | 15 +++++--- .../storage/SecondaryUserControllerTest.java | 2 +- .../StorageItemPreferenceControllerTest.java | 12 +++--- .../storage/UserProfileControllerTest.java | 4 +- .../storage/StorageAsyncLoaderTest.java | 17 +-------- 10 files changed, 66 insertions(+), 64 deletions(-) diff --git a/src/com/android/settings/deviceinfo/StorageDashboardFragment.java b/src/com/android/settings/deviceinfo/StorageDashboardFragment.java index 4974f784480..13b3d0b8c19 100644 --- a/src/com/android/settings/deviceinfo/StorageDashboardFragment.java +++ b/src/com/android/settings/deviceinfo/StorageDashboardFragment.java @@ -131,7 +131,7 @@ public class StorageDashboardFragment extends DashboardFragment } } - mPreferenceController.onLoadFinished(mAppsResult.get(UserHandle.myUserId())); + mPreferenceController.onLoadFinished(mAppsResult, UserHandle.myUserId()); updateSecondaryUserControllers(mSecondaryUsers, mAppsResult); // setLoading always causes a flicker, so let's avoid doing it. diff --git a/src/com/android/settings/deviceinfo/StorageProfileFragment.java b/src/com/android/settings/deviceinfo/StorageProfileFragment.java index 7e2d94187f8..dee6793398b 100644 --- a/src/com/android/settings/deviceinfo/StorageProfileFragment.java +++ b/src/com/android/settings/deviceinfo/StorageProfileFragment.java @@ -120,7 +120,8 @@ public class StorageProfileFragment extends DashboardFragment @Override public void onLoadFinished(Loader> loader, SparseArray result) { - mPreferenceController.onLoadFinished(scrubAppsFromResult(result.get(mUserId))); + scrubAppsFromResult(result.get(mUserId)); + mPreferenceController.onLoadFinished(result, mUserId); } @Override diff --git a/src/com/android/settings/deviceinfo/storage/StorageAsyncLoader.java b/src/com/android/settings/deviceinfo/storage/StorageAsyncLoader.java index 86528046f58..74474b38cca 100644 --- a/src/com/android/settings/deviceinfo/storage/StorageAsyncLoader.java +++ b/src/com/android/settings/deviceinfo/storage/StorageAsyncLoader.java @@ -22,8 +22,8 @@ import static android.content.pm.ApplicationInfo.CATEGORY_VIDEO; import android.content.Context; import android.content.pm.ApplicationInfo; -import android.content.pm.UserInfo; import android.content.pm.PackageManager.NameNotFoundException; +import android.content.pm.UserInfo; import android.os.UserHandle; import android.util.Log; import android.util.SparseArray; @@ -87,45 +87,43 @@ public class StorageAsyncLoader stats = mStatsManager.getStatsForPackage(mUuid, app.packageName, myUser); } catch (NameNotFoundException | IOException e) { // This may happen if the package was removed during our calculation. - Log.w("App unexpectedly not found", e); + Log.w(TAG, "App unexpectedly not found", e); continue; } - long attributedAppSizeInBytes = stats.getDataBytes(); - // This matches how the package manager calculates sizes -- by zeroing out code sizes of - // system apps which are not updated. My initial tests suggest that this results in the - // original code size being counted for updated system apps when they shouldn't, but - // I am not sure how to avoid this problem without specifically going in to find that - // code size. - if (!app.isSystemApp() || app.isUpdatedSystemApp()) { - attributedAppSizeInBytes += stats.getCodeBytes(); - } else { - result.systemSize += stats.getCodeBytes(); + long blamedSize = stats.getDataBytes() - stats.getCacheBytes(); + + // Only count app code against the current user; we don't want + // double-counting on multi-user devices. + if (userId == UserHandle.myUserId()) { + blamedSize += stats.getCodeBytes(); } + switch (app.category) { case CATEGORY_GAME: - result.gamesSize += attributedAppSizeInBytes; + result.gamesSize += blamedSize; break; case CATEGORY_AUDIO: - result.musicAppsSize += attributedAppSizeInBytes; + result.musicAppsSize += blamedSize; break; case CATEGORY_VIDEO: - result.videoAppsSize += attributedAppSizeInBytes; + result.videoAppsSize += blamedSize; break; default: // The deprecated game flag does not set the category. if ((app.flags & ApplicationInfo.FLAG_IS_GAME) != 0) { - result.gamesSize += attributedAppSizeInBytes; + result.gamesSize += blamedSize; break; } - result.otherAppsSize += attributedAppSizeInBytes; + result.otherAppsSize += blamedSize; break; } } Log.d(TAG, "Loading external stats"); try { - result.externalStats = mStatsManager.getExternalStorageStats(mUuid, UserHandle.of(userId)); + result.externalStats = mStatsManager.getExternalStorageStats(mUuid, + UserHandle.of(userId)); } catch (IOException e) { Log.w(TAG, e); } @@ -142,7 +140,6 @@ public class StorageAsyncLoader public long musicAppsSize; public long videoAppsSize; public long otherAppsSize; - public long systemSize; public StorageStatsSource.ExternalStorageStats externalStats; } diff --git a/src/com/android/settings/deviceinfo/storage/StorageItemPreferenceController.java b/src/com/android/settings/deviceinfo/storage/StorageItemPreferenceController.java index f8df375588e..cebd1145b19 100644 --- a/src/com/android/settings/deviceinfo/storage/StorageItemPreferenceController.java +++ b/src/com/android/settings/deviceinfo/storage/StorageItemPreferenceController.java @@ -23,6 +23,7 @@ import android.content.Intent; import android.content.pm.PackageManager; import android.content.res.TypedArray; import android.graphics.drawable.Drawable; +import android.net.TrafficStats; import android.os.Bundle; import android.os.UserHandle; import android.os.storage.VolumeInfo; @@ -30,6 +31,7 @@ import android.support.annotation.VisibleForTesting; import android.support.v7.preference.Preference; import android.support.v7.preference.PreferenceScreen; import android.util.Log; +import android.util.SparseArray; import com.android.internal.logging.nano.MetricsProto.MetricsEvent; import com.android.settings.R; @@ -237,7 +239,10 @@ public class StorageItemPreferenceController extends PreferenceController { setFilesPreferenceVisibility(); } - public void onLoadFinished(StorageAsyncLoader.AppsStorageResult data) { + public void onLoadFinished(SparseArray result, + int userId) { + final StorageAsyncLoader.AppsStorageResult data = result.get(userId); + // TODO(b/35927909): Figure out how to split out apps which are only installed for work // profiles in order to attribute those app's code bytes only to that profile. mPhotoPreference.setStorageSize( @@ -248,23 +253,30 @@ public class StorageItemPreferenceController extends PreferenceController { mMoviesPreference.setStorageSize(data.videoAppsSize, mTotalSize); mAppPreference.setStorageSize(data.otherAppsSize, mTotalSize); - long unattributedExternalBytes = + long otherExternalBytes = data.externalStats.totalBytes - data.externalStats.audioBytes - data.externalStats.videoBytes - - data.externalStats.imageBytes; - mFilePreference.setStorageSize(unattributedExternalBytes, mTotalSize); + - data.externalStats.imageBytes + - data.externalStats.appBytes; + mFilePreference.setStorageSize(otherExternalBytes, mTotalSize); - // We define the system size as everything we can't classify. if (mSystemPreference != null) { - mSystemPreference.setStorageSize( - mUsedBytes - - data.externalStats.totalBytes - - data.musicAppsSize - - data.gamesSize - - data.videoAppsSize - - data.otherAppsSize, - mTotalSize); + // Everything else that hasn't already been attributed is tracked as + // belonging to system. + long attributedSize = 0; + for (int i = 0; i < result.size(); i++) { + final StorageAsyncLoader.AppsStorageResult otherData = result.valueAt(i); + attributedSize += otherData.gamesSize + + otherData.musicAppsSize + + otherData.videoAppsSize + + otherData.otherAppsSize; + attributedSize += otherData.externalStats.totalBytes + - otherData.externalStats.appBytes; + } + + final long systemSize = Math.max(TrafficStats.GB_IN_BYTES, mUsedBytes - attributedSize); + mSystemPreference.setStorageSize(systemSize, mTotalSize); } } diff --git a/tests/robotests/src/com/android/settings/applications/MusicViewHolderControllerTest.java b/tests/robotests/src/com/android/settings/applications/MusicViewHolderControllerTest.java index 644014129f7..779e0b647cd 100644 --- a/tests/robotests/src/com/android/settings/applications/MusicViewHolderControllerTest.java +++ b/tests/robotests/src/com/android/settings/applications/MusicViewHolderControllerTest.java @@ -83,7 +83,7 @@ public class MusicViewHolderControllerTest { @Test public void storageShouldRepresentStorageStatsQuery() throws Exception { when(mSource.getExternalStorageStats(any(String.class), any(UserHandle.class))).thenReturn( - new StorageStatsSource.ExternalStorageStats(1, 1, 0, 0)); + new StorageStatsSource.ExternalStorageStats(1, 1, 0, 0, 0)); mController.queryStats(); mController.setupView(mHolder); diff --git a/tests/robotests/src/com/android/settings/deviceinfo/StorageProfileFragmentTest.java b/tests/robotests/src/com/android/settings/deviceinfo/StorageProfileFragmentTest.java index 8d48e633918..03f15bbec15 100644 --- a/tests/robotests/src/com/android/settings/deviceinfo/StorageProfileFragmentTest.java +++ b/tests/robotests/src/com/android/settings/deviceinfo/StorageProfileFragmentTest.java @@ -17,6 +17,7 @@ package com.android.settings.deviceinfo; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Matchers.anyInt; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -31,11 +32,16 @@ import com.android.settingslib.applications.StorageStatsSource; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.MockitoAnnotations; import org.robolectric.annotation.Config; @RunWith(SettingsRobolectricTestRunner.class) @Config(manifest = TestConfig.MANIFEST_PATH, sdk = TestConfig.SDK_VERSION) public class StorageProfileFragmentTest { + @Captor + private ArgumentCaptor> mCaptor; + @Test public void verifyAppSizesAreZeroedOut() { StorageItemPreferenceController controller = mock(StorageItemPreferenceController.class); @@ -45,18 +51,17 @@ public class StorageProfileFragmentTest { result.otherAppsSize = 200; result.gamesSize = 300; result.videoAppsSize = 400; - result.externalStats = new StorageStatsSource.ExternalStorageStats(6, 1, 2, 3); + result.externalStats = new StorageStatsSource.ExternalStorageStats(6, 1, 2, 3, 0); SparseArray resultsArray = new SparseArray<>(); resultsArray.put(0, result); fragment.setPreferenceController(controller); fragment.onLoadFinished(null, resultsArray); - ArgumentCaptor resultCaptor = ArgumentCaptor.forClass( - StorageAsyncLoader.AppsStorageResult.class); - verify(controller).onLoadFinished(resultCaptor.capture()); + MockitoAnnotations.initMocks(this); + verify(controller).onLoadFinished(mCaptor.capture(), anyInt()); - StorageAsyncLoader.AppsStorageResult extractedResult = resultCaptor.getValue(); + StorageAsyncLoader.AppsStorageResult extractedResult = mCaptor.getValue().get(0); assertThat(extractedResult.musicAppsSize).isEqualTo(0); assertThat(extractedResult.videoAppsSize).isEqualTo(0); assertThat(extractedResult.otherAppsSize).isEqualTo(0); diff --git a/tests/robotests/src/com/android/settings/deviceinfo/storage/SecondaryUserControllerTest.java b/tests/robotests/src/com/android/settings/deviceinfo/storage/SecondaryUserControllerTest.java index 32def6956b8..dc1c2860777 100644 --- a/tests/robotests/src/com/android/settings/deviceinfo/storage/SecondaryUserControllerTest.java +++ b/tests/robotests/src/com/android/settings/deviceinfo/storage/SecondaryUserControllerTest.java @@ -168,7 +168,7 @@ public class SecondaryUserControllerTest { MEGABYTE_IN_BYTES * 30, MEGABYTE_IN_BYTES * 10, MEGABYTE_IN_BYTES * 10, - MEGABYTE_IN_BYTES * 10); + MEGABYTE_IN_BYTES * 10, 0); result.put(10, userResult); mController.handleResult(result); diff --git a/tests/robotests/src/com/android/settings/deviceinfo/storage/StorageItemPreferenceControllerTest.java b/tests/robotests/src/com/android/settings/deviceinfo/storage/StorageItemPreferenceControllerTest.java index e8057a62b14..3ad37838a8e 100644 --- a/tests/robotests/src/com/android/settings/deviceinfo/storage/StorageItemPreferenceControllerTest.java +++ b/tests/robotests/src/com/android/settings/deviceinfo/storage/StorageItemPreferenceControllerTest.java @@ -28,7 +28,6 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; - import android.app.Fragment; import android.content.Context; import android.content.Intent; @@ -36,6 +35,7 @@ import android.graphics.drawable.Drawable; import android.os.UserHandle; import android.os.storage.VolumeInfo; import android.support.v7.preference.PreferenceScreen; +import android.util.SparseArray; import android.view.LayoutInflater; import android.view.View; import android.widget.LinearLayout; @@ -275,22 +275,22 @@ public class StorageItemPreferenceControllerTest { result.videoAppsSize = MEGABYTE_IN_BYTES * 160; result.musicAppsSize = MEGABYTE_IN_BYTES * 40; result.otherAppsSize = MEGABYTE_IN_BYTES * 90; - result.systemSize = MEGABYTE_IN_BYTES * 100; // This value is ignored and overridden now. result.externalStats = new StorageStatsSource.ExternalStorageStats( MEGABYTE_IN_BYTES * 500, // total MEGABYTE_IN_BYTES * 100, // audio MEGABYTE_IN_BYTES * 150, // video - MEGABYTE_IN_BYTES * 200); // image + MEGABYTE_IN_BYTES * 200, 0); // image - mController.onLoadFinished(result); + SparseArray results = new SparseArray<>(); + results.put(0, result); + mController.onLoadFinished(results, 0); assertThat(audio.getSummary().toString()).isEqualTo("0.14GB"); assertThat(image.getSummary().toString()).isEqualTo("0.35GB"); assertThat(games.getSummary().toString()).isEqualTo("0.08GB"); assertThat(movies.getSummary().toString()).isEqualTo("0.16GB"); assertThat(apps.getSummary().toString()).isEqualTo("0.09GB"); - assertThat(system.getSummary().toString()).isEqualTo("0.10GB"); assertThat(files.getSummary().toString()).isEqualTo("0.05GB"); } @@ -488,4 +488,4 @@ public class StorageItemPreferenceControllerTest { verify(screen).addPreference(files); } -} \ No newline at end of file +} diff --git a/tests/robotests/src/com/android/settings/deviceinfo/storage/UserProfileControllerTest.java b/tests/robotests/src/com/android/settings/deviceinfo/storage/UserProfileControllerTest.java index 8b7110d8d02..2199824eac6 100644 --- a/tests/robotests/src/com/android/settings/deviceinfo/storage/UserProfileControllerTest.java +++ b/tests/robotests/src/com/android/settings/deviceinfo/storage/UserProfileControllerTest.java @@ -114,7 +114,7 @@ public class UserProfileControllerTest { 99 * MEGABYTE_IN_BYTES, 33 * MEGABYTE_IN_BYTES, 33 * MEGABYTE_IN_BYTES, - 33 * MEGABYTE_IN_BYTES); + 33 * MEGABYTE_IN_BYTES, 0); result.put(10, userResult); mController.handleResult(result); @@ -141,4 +141,4 @@ public class UserProfileControllerTest { Preference preference = argumentCaptor.getValue(); assertThat(preference.getIcon()).isEqualTo(drawable); } -} \ No newline at end of file +} diff --git a/tests/unit/src/com/android/settings/deviceinfo/storage/StorageAsyncLoaderTest.java b/tests/unit/src/com/android/settings/deviceinfo/storage/StorageAsyncLoaderTest.java index 546ea4b799e..79a4595e010 100644 --- a/tests/unit/src/com/android/settings/deviceinfo/storage/StorageAsyncLoaderTest.java +++ b/tests/unit/src/com/android/settings/deviceinfo/storage/StorageAsyncLoaderTest.java @@ -133,9 +133,9 @@ public class StorageAsyncLoaderTest { info.id = SECONDARY_USER_ID; mUsers.add(info); when(mSource.getExternalStorageStats(anyString(), eq(UserHandle.SYSTEM))) - .thenReturn(new StorageStatsSource.ExternalStorageStats(9, 2, 3, 4)); + .thenReturn(new StorageStatsSource.ExternalStorageStats(9, 2, 3, 4, 0)); when(mSource.getExternalStorageStats(anyString(), eq(new UserHandle(SECONDARY_USER_ID)))) - .thenReturn(new StorageStatsSource.ExternalStorageStats(10, 3, 3, 4)); + .thenReturn(new StorageStatsSource.ExternalStorageStats(10, 3, 3, 4, 0)); SparseArray result = mLoader.loadInBackground(); @@ -144,19 +144,6 @@ public class StorageAsyncLoaderTest { assertThat(result.get(SECONDARY_USER_ID).externalStats.totalBytes).isEqualTo(10L); } - @Test - public void testSystemAppsBaseSizeIsAddedToSystem() throws Exception { - ApplicationInfo systemApp = - addPackage(PACKAGE_NAME_1, 100, 1, 10, ApplicationInfo.CATEGORY_UNDEFINED); - systemApp.flags = ApplicationInfo.FLAG_SYSTEM; - - SparseArray result = mLoader.loadInBackground(); - - assertThat(result.size()).isEqualTo(1); - assertThat(result.get(PRIMARY_USER_ID).otherAppsSize).isEqualTo(10L); - assertThat(result.get(PRIMARY_USER_ID).systemSize).isEqualTo(1L); - } - @Test public void testUpdatedSystemAppCodeSizeIsCounted() throws Exception { ApplicationInfo systemApp =