Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix unbounded number of uncached shortcuts

Browse Source 
There is a consistency issue in which some cached conversations in
People Service do not get cleaned up because they are missing a event
timestamp. For those cases, we use the creation timestamp to order by
recency and still clean up those conversations.

This change will enforce that at most `MAX_CACHED_RECENT_SHORTCUTS` are
cached by People Service for uncustomized conversations as expected.

Test: Manually with app that creates cached shortcuts and unit tests
Bug: 233926836
Change-Id: Ia017917ea56f2e233fdb837e574a8bd7c0d69e3b
This commit is contained in:
Jeff Nainaparampil
2022-08-09 16:07:13 +00:00
parent 4f28845c84
commit 59b2f1743d
5 changed files with 250 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
core/proto/android/server/peopleservice.proto
View File

@@ -62,7 +62,10 @@ message ConversationInfoProto {
// The timestamp of the last event in millis.
optional int64 last_event_timestamp = 9;
// Next tag: 10
// The timestamp this conversation was created in millis.
optional int64 creation_timestamp = 10;
// Next tag: 11
}
// On disk data of events.

49
services/people/java/com/android/server/people/data/ConversationInfo.java
View File

@@ -26,6 +26,7 @@ import android.content.pm.ShortcutInfo;
import android.content.pm.ShortcutInfo.ShortcutFlags;
import android.net.Uri;
import android.text.TextUtils;
import android.util.Log;
import android.util.Slog;
import android.util.proto.ProtoInputStream;
import android.util.proto.ProtoOutputStream;
@@ -37,6 +38,7 @@ import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.DataInputStream;
import java.io.DataOutputStream;
import java.io.EOFException;
import java.io.IOException;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
@@ -50,6 +52,11 @@ import java.util.Objects;
* Represents a conversation that is provided by the app based on {@link ShortcutInfo}.
*/
public class ConversationInfo {
private static final boolean DEBUG = false;
// Schema version for the backup payload. Must be incremented whenever fields are added in
// backup payload.
private static final int VERSION = 1;
private static final String TAG = ConversationInfo.class.getSimpleName();
@@ -100,6 +107,8 @@ public class ConversationInfo {
private long mLastEventTimestamp;
private long mCreationTimestamp;
@ShortcutFlags
private int mShortcutFlags;
@@ -116,6 +125,7 @@ public class ConversationInfo {
mNotificationChannelId = builder.mNotificationChannelId;
mParentNotificationChannelId = builder.mParentNotificationChannelId;
mLastEventTimestamp = builder.mLastEventTimestamp;
mCreationTimestamp = builder.mCreationTimestamp;
mShortcutFlags = builder.mShortcutFlags;
mConversationFlags = builder.mConversationFlags;
mCurrStatuses = builder.mCurrStatuses;
@@ -170,6 +180,13 @@ public class ConversationInfo {
return mLastEventTimestamp;
}
/**
* Timestamp of the creation of the conversationInfo.
*/
long getCreationTimestamp() {
return mCreationTimestamp;
}
/** Whether the shortcut for this conversation is set long-lived by the app. */
public boolean isShortcutLongLived() {
return hasShortcutFlags(ShortcutInfo.FLAG_LONG_LIVED);
@@ -241,6 +258,7 @@ public class ConversationInfo {
&& Objects.equals(mNotificationChannelId, other.mNotificationChannelId)
&& Objects.equals(mParentNotificationChannelId, other.mParentNotificationChannelId)
&& Objects.equals(mLastEventTimestamp, other.mLastEventTimestamp)
&& mCreationTimestamp == other.mCreationTimestamp
&& mShortcutFlags == other.mShortcutFlags
&& mConversationFlags == other.mConversationFlags
&& Objects.equals(mCurrStatuses, other.mCurrStatuses);
@@ -250,7 +268,7 @@ public class ConversationInfo {
public int hashCode() {
return Objects.hash(mShortcutId, mLocusId, mContactUri, mContactPhoneNumber,
mNotificationChannelId, mParentNotificationChannelId, mLastEventTimestamp,
mShortcutFlags, mConversationFlags, mCurrStatuses);
mCreationTimestamp, mShortcutFlags, mConversationFlags, mCurrStatuses);
}
@Override
@@ -264,6 +282,7 @@ public class ConversationInfo {
sb.append(", notificationChannelId=").append(mNotificationChannelId);
sb.append(", parentNotificationChannelId=").append(mParentNotificationChannelId);
sb.append(", lastEventTimestamp=").append(mLastEventTimestamp);
sb.append(", creationTimestamp=").append(mCreationTimestamp);
sb.append(", statuses=").append(mCurrStatuses);
sb.append(", shortcutFlags=0x").append(Integer.toHexString(mShortcutFlags));
sb.append(" [");
@@ -329,6 +348,7 @@ public class ConversationInfo {
mParentNotificationChannelId);
}
protoOutputStream.write(ConversationInfoProto.LAST_EVENT_TIMESTAMP, mLastEventTimestamp);
protoOutputStream.write(ConversationInfoProto.CREATION_TIMESTAMP, mCreationTimestamp);
protoOutputStream.write(ConversationInfoProto.SHORTCUT_FLAGS, mShortcutFlags);
protoOutputStream.write(ConversationInfoProto.CONVERSATION_FLAGS, mConversationFlags);
if (mContactPhoneNumber != null) {
@@ -352,6 +372,8 @@ public class ConversationInfo {
out.writeUTF(mContactPhoneNumber != null ? mContactPhoneNumber : "");
out.writeUTF(mParentNotificationChannelId != null ? mParentNotificationChannelId : "");
out.writeLong(mLastEventTimestamp);
out.writeInt(VERSION);
out.writeLong(mCreationTimestamp);
// ConversationStatus is a transient object and not persisted
} catch (IOException e) {
Slog.e(TAG, "Failed to write fields to backup payload.", e);
@@ -399,6 +421,9 @@ public class ConversationInfo {
builder.setLastEventTimestamp(protoInputStream.readLong(
ConversationInfoProto.LAST_EVENT_TIMESTAMP));
break;
case (int) ConversationInfoProto.CREATION_TIMESTAMP:
builder.setCreationTimestamp(protoInputStream.readLong(
ConversationInfoProto.CREATION_TIMESTAMP));
case (int) ConversationInfoProto.SHORTCUT_FLAGS:
builder.setShortcutFlags(protoInputStream.readInt(
ConversationInfoProto.SHORTCUT_FLAGS));
@@ -448,6 +473,10 @@ public class ConversationInfo {
builder.setParentNotificationChannelId(parentNotificationChannelId);
}
builder.setLastEventTimestamp(in.readLong());
int payloadVersion = maybeReadVersion(in);
if (payloadVersion == 1) {
builder.setCreationTimestamp(in.readLong());
}
} catch (IOException e) {
Slog.e(TAG, "Failed to read conversation info fields from backup payload.", e);
return null;
@@ -455,6 +484,16 @@ public class ConversationInfo {
return builder.build();
}
private static int maybeReadVersion(DataInputStream in) throws IOException {
try {
return in.readInt();
} catch (EOFException eofException) {
// EOF is expected if using old backup payload protocol.
if (DEBUG) Log.d(TAG, "Eof reached for data stream, missing version number");
return 0;
}
}
/**
* Builder class for {@link ConversationInfo} objects.
*/
@@ -479,6 +518,8 @@ public class ConversationInfo {
private long mLastEventTimestamp;
private long mCreationTimestamp;
@ShortcutFlags
private int mShortcutFlags;
@@ -502,6 +543,7 @@ public class ConversationInfo {
mNotificationChannelId = conversationInfo.mNotificationChannelId;
mParentNotificationChannelId = conversationInfo.mParentNotificationChannelId;
mLastEventTimestamp = conversationInfo.mLastEventTimestamp;
mCreationTimestamp = conversationInfo.mCreationTimestamp;
mShortcutFlags = conversationInfo.mShortcutFlags;
mConversationFlags = conversationInfo.mConversationFlags;
mCurrStatuses = conversationInfo.mCurrStatuses;
@@ -542,6 +584,11 @@ public class ConversationInfo {
return this;
}
Builder setCreationTimestamp(long creationTimestamp) {
mCreationTimestamp = creationTimestamp;
return this;
}
Builder setShortcutFlags(@ShortcutFlags int shortcutFlags) {
mShortcutFlags = shortcutFlags;
return this;

29
services/people/java/com/android/server/people/data/DataManager.java
View File

@@ -816,10 +816,18 @@ public class DataManager {
}
private boolean isCachedRecentConversation(ConversationInfo conversationInfo) {
return isEligibleForCleanUp(conversationInfo)
&& conversationInfo.getLastEventTimestamp() > 0L;
}
/**
* Conversations that are cached and not customized are eligible for clean-up, even if they
* don't have an associated notification event with them.
*/
private boolean isEligibleForCleanUp(ConversationInfo conversationInfo) {
return conversationInfo.isShortcutCachedForNotification()
&& Objects.equals(conversationInfo.getNotificationChannelId(),
conversationInfo.getParentNotificationChannelId())
&& conversationInfo.getLastEventTimestamp() > 0L;
conversationInfo.getParentNotificationChannelId());
}
private boolean hasActiveNotifications(String packageName, @UserIdInt int userId,
@@ -842,14 +850,14 @@ public class DataManager {
}
// pair of <package name, conversation info>
List<Pair<String, ConversationInfo>> cachedConvos = new ArrayList<>();
userData.forAllPackages(packageData ->
userData.forAllPackages(packageData -> {
packageData.forAllConversations(conversationInfo -> {
if (isCachedRecentConversation(conversationInfo)) {
if (isEligibleForCleanUp(conversationInfo)) {
cachedConvos.add(
Pair.create(packageData.getPackageName(), conversationInfo));
}
})
);
});
});
if (cachedConvos.size() <= targetCachedCount) {
return;
}
@@ -858,7 +866,9 @@ public class DataManager {
PriorityQueue<Pair<String, ConversationInfo>> maxHeap = new PriorityQueue<>(
numToUncache + 1,
Comparator.comparingLong((Pair<String, ConversationInfo> pair) ->
pair.second.getLastEventTimestamp()).reversed());
Math.max(
pair.second.getLastEventTimestamp(),
pair.second.getCreationTimestamp())).reversed());
for (Pair<String, ConversationInfo> cached : cachedConvos) {
if (hasActiveNotifications(cached.first, userId, cached.second.getShortcutId())) {
continue;
@@ -893,7 +903,7 @@ public class DataManager {
}
ConversationInfo.Builder builder = oldConversationInfo != null
? new ConversationInfo.Builder(oldConversationInfo)
: new ConversationInfo.Builder();
: new ConversationInfo.Builder().setCreationTimestamp(System.currentTimeMillis());
builder.setShortcutId(shortcutInfo.getId());
builder.setLocusId(shortcutInfo.getLocusId());
@@ -1326,7 +1336,8 @@ public class DataManager {
}
}
private void updateConversationStoreThenNotifyListeners(ConversationStore cs,
@VisibleForTesting
void updateConversationStoreThenNotifyListeners(ConversationStore cs,
ConversationInfo modifiedConv,
String packageName, int userId) {
cs.addOrUpdate(modifiedConv);

106
services/tests/servicestests/src/com/android/server/people/data/ConversationInfoTest.java
View File

@@ -58,6 +58,7 @@ public final class ConversationInfoTest {
.setNotificationChannelId(NOTIFICATION_CHANNEL_ID)
.setParentNotificationChannelId(PARENT_NOTIFICATION_CHANNEL_ID)
.setLastEventTimestamp(100L)
.setCreationTimestamp(200L)
.setShortcutFlags(ShortcutInfo.FLAG_LONG_LIVED
| ShortcutInfo.FLAG_CACHED_NOTIFICATIONS)
.setImportant(true)
@@ -79,6 +80,7 @@ public final class ConversationInfoTest {
assertEquals(PARENT_NOTIFICATION_CHANNEL_ID,
conversationInfo.getParentNotificationChannelId());
assertEquals(100L, conversationInfo.getLastEventTimestamp());
assertEquals(200L, conversationInfo.getCreationTimestamp());
assertTrue(conversationInfo.isShortcutLongLived());
assertTrue(conversationInfo.isShortcutCachedForNotification());
assertTrue(conversationInfo.isImportant());
@@ -105,6 +107,7 @@ public final class ConversationInfoTest {
assertNull(conversationInfo.getNotificationChannelId());
assertNull(conversationInfo.getParentNotificationChannelId());
assertEquals(0L, conversationInfo.getLastEventTimestamp());
assertEquals(0L, conversationInfo.getCreationTimestamp());
assertFalse(conversationInfo.isShortcutLongLived());
assertFalse(conversationInfo.isShortcutCachedForNotification());
assertFalse(conversationInfo.isImportant());
@@ -131,6 +134,7 @@ public final class ConversationInfoTest {
.setNotificationChannelId(NOTIFICATION_CHANNEL_ID)
.setParentNotificationChannelId(PARENT_NOTIFICATION_CHANNEL_ID)
.setLastEventTimestamp(100L)
.setCreationTimestamp(200L)
.setShortcutFlags(ShortcutInfo.FLAG_LONG_LIVED)
.setImportant(true)
.setNotificationSilenced(true)
@@ -154,6 +158,7 @@ public final class ConversationInfoTest {
assertEquals(NOTIFICATION_CHANNEL_ID, destination.getNotificationChannelId());
assertEquals(PARENT_NOTIFICATION_CHANNEL_ID, destination.getParentNotificationChannelId());
assertEquals(100L, destination.getLastEventTimestamp());
assertEquals(200L, destination.getCreationTimestamp());
assertTrue(destination.isShortcutLongLived());
assertFalse(destination.isImportant());
assertTrue(destination.isNotificationSilenced());
@@ -164,4 +169,105 @@ public final class ConversationInfoTest {
assertThat(destination.getStatuses()).contains(cs);
assertThat(destination.getStatuses()).contains(cs2);
}
@Test
public void testBuildFromAnotherConversation_identicalConversation() {
ConversationStatus cs = new ConversationStatus.Builder("id", ACTIVITY_ANNIVERSARY).build();
ConversationStatus cs2 = new ConversationStatus.Builder("id2", ACTIVITY_GAME).build();
ConversationInfo source = new ConversationInfo.Builder()
.setShortcutId(SHORTCUT_ID)
.setLocusId(LOCUS_ID)
.setContactUri(CONTACT_URI)
.setContactPhoneNumber(PHONE_NUMBER)
.setNotificationChannelId(NOTIFICATION_CHANNEL_ID)
.setParentNotificationChannelId(PARENT_NOTIFICATION_CHANNEL_ID)
.setLastEventTimestamp(100L)
.setCreationTimestamp(200L)
.setShortcutFlags(ShortcutInfo.FLAG_LONG_LIVED)
.setImportant(true)
.setNotificationSilenced(true)
.setBubbled(true)
.setPersonImportant(true)
.setPersonBot(true)
.setContactStarred(true)
.addOrUpdateStatus(cs)
.addOrUpdateStatus(cs2)
.build();
ConversationInfo destination = new ConversationInfo.Builder(source).build();
assertEquals(SHORTCUT_ID, destination.getShortcutId());
assertEquals(LOCUS_ID, destination.getLocusId());
assertEquals(CONTACT_URI, destination.getContactUri());
assertEquals(PHONE_NUMBER, destination.getContactPhoneNumber());
assertEquals(NOTIFICATION_CHANNEL_ID, destination.getNotificationChannelId());
assertEquals(PARENT_NOTIFICATION_CHANNEL_ID, destination.getParentNotificationChannelId());
assertEquals(100L, destination.getLastEventTimestamp());
assertEquals(200L, destination.getCreationTimestamp());
assertTrue(destination.isShortcutLongLived());
assertTrue(destination.isImportant());
assertTrue(destination.isNotificationSilenced());
assertTrue(destination.isBubbled());
assertTrue(destination.isPersonImportant());
assertTrue(destination.isPersonBot());
assertTrue(destination.isContactStarred());
assertThat(destination.getStatuses()).contains(cs);
assertThat(destination.getStatuses()).contains(cs2);
// Also check equals() implementation
assertTrue(source.equals(destination));
assertTrue(destination.equals(source));
}
@Test
public void testBuildFromBackupPayload() {
ConversationStatus cs = new ConversationStatus.Builder("id", ACTIVITY_ANNIVERSARY).build();
ConversationStatus cs2 = new ConversationStatus.Builder("id2", ACTIVITY_GAME).build();
ConversationInfo conversationInfo = new ConversationInfo.Builder()
.setShortcutId(SHORTCUT_ID)
.setLocusId(LOCUS_ID)
.setContactUri(CONTACT_URI)
.setContactPhoneNumber(PHONE_NUMBER)
.setNotificationChannelId(NOTIFICATION_CHANNEL_ID)
.setParentNotificationChannelId(PARENT_NOTIFICATION_CHANNEL_ID)
.setLastEventTimestamp(100L)
.setCreationTimestamp(200L)
.setShortcutFlags(ShortcutInfo.FLAG_LONG_LIVED
| ShortcutInfo.FLAG_CACHED_NOTIFICATIONS)
.setImportant(true)
.setNotificationSilenced(true)
.setBubbled(true)
.setDemoted(true)
.setPersonImportant(true)
.setPersonBot(true)
.setContactStarred(true)
.addOrUpdateStatus(cs)
.addOrUpdateStatus(cs2)
.build();
ConversationInfo conversationInfoFromBackup =
ConversationInfo.readFromBackupPayload(conversationInfo.getBackupPayload());
assertEquals(SHORTCUT_ID, conversationInfoFromBackup.getShortcutId());
assertEquals(LOCUS_ID, conversationInfoFromBackup.getLocusId());
assertEquals(CONTACT_URI, conversationInfoFromBackup.getContactUri());
assertEquals(PHONE_NUMBER, conversationInfoFromBackup.getContactPhoneNumber());
assertEquals(
NOTIFICATION_CHANNEL_ID, conversationInfoFromBackup.getNotificationChannelId());
assertEquals(PARENT_NOTIFICATION_CHANNEL_ID,
conversationInfoFromBackup.getParentNotificationChannelId());
assertEquals(100L, conversationInfoFromBackup.getLastEventTimestamp());
assertEquals(200L, conversationInfoFromBackup.getCreationTimestamp());
assertTrue(conversationInfoFromBackup.isShortcutLongLived());
assertTrue(conversationInfoFromBackup.isShortcutCachedForNotification());
assertTrue(conversationInfoFromBackup.isImportant());
assertTrue(conversationInfoFromBackup.isNotificationSilenced());
assertTrue(conversationInfoFromBackup.isBubbled());
assertTrue(conversationInfoFromBackup.isDemoted());
assertTrue(conversationInfoFromBackup.isPersonImportant());
assertTrue(conversationInfoFromBackup.isPersonBot());
assertTrue(conversationInfoFromBackup.isContactStarred());
// ConversationStatus is a transient object and not persisted
}
}

73
services/tests/servicestests/src/com/android/server/people/data/DataManagerTest.java
View File

@@ -291,7 +291,8 @@ public final class DataManagerTest {
mShortcutChangeCallbackCaptor.capture());
mShortcutChangeCallback = mShortcutChangeCallbackCaptor.getValue();
verify(mContext, times(2)).registerReceiver(any(), any());
verify(mContext, times(1)).registerReceiver(any(), any());
verify(mContext, times(1)).registerReceiver(any(), any(), anyInt());
}
@After
@@ -1162,6 +1163,76 @@ public final class DataManagerTest {
}
}
@Test
public void testUncacheOldestCachedShortcut_missingNotificationEvents() {
mDataManager.onUserUnlocked(USER_ID_PRIMARY);
for (int i = 0; i < DataManager.MAX_CACHED_RECENT_SHORTCUTS + 1; i++) {
String shortcutId = TEST_SHORTCUT_ID + i;
ShortcutInfo shortcut = buildShortcutInfo(TEST_PKG_NAME, USER_ID_PRIMARY, shortcutId,
buildPerson());
shortcut.setCached(ShortcutInfo.FLAG_CACHED_NOTIFICATIONS);
mShortcutChangeCallback.onShortcutsAddedOrUpdated(
TEST_PKG_NAME,
Collections.singletonList(shortcut),
UserHandle.of(USER_ID_PRIMARY));
mLooper.dispatchAll();
}
// Only the shortcut #0 is uncached, all the others are not.
verify(mShortcutServiceInternal).uncacheShortcuts(
anyInt(), any(), eq(TEST_PKG_NAME),
eq(Collections.singletonList(TEST_SHORTCUT_ID + 0)), eq(USER_ID_PRIMARY),
eq(ShortcutInfo.FLAG_CACHED_NOTIFICATIONS));
for (int i = 1; i < DataManager.MAX_CACHED_RECENT_SHORTCUTS + 1; i++) {
verify(mShortcutServiceInternal, never()).uncacheShortcuts(
anyInt(), anyString(), anyString(),
eq(Collections.singletonList(TEST_SHORTCUT_ID + i)), anyInt(),
eq(ShortcutInfo.FLAG_CACHED_NOTIFICATIONS));
}
}
@Test
public void testUncacheOldestCachedShortcut_legacyConversation() {
mDataManager.onUserUnlocked(USER_ID_PRIMARY);
// Add an extra conversation with a legacy type (no creationTime)
ConversationStore conversationStore = mDataManager
.getUserDataForTesting(USER_ID_PRIMARY)
.getOrCreatePackageData(TEST_PKG_NAME)
.getConversationStore();
ConversationInfo.Builder builder = new ConversationInfo.Builder();
builder.setShortcutId(TEST_SHORTCUT_ID + 0);
builder.setShortcutFlags(ShortcutInfo.FLAG_CACHED_NOTIFICATIONS);
mDataManager.updateConversationStoreThenNotifyListeners(
conversationStore,
builder.build(),
TEST_PKG_NAME, USER_ID_PRIMARY);
for (int i = 1; i < DataManager.MAX_CACHED_RECENT_SHORTCUTS + 1; i++) {
String shortcutId = TEST_SHORTCUT_ID + i;
ShortcutInfo shortcut = buildShortcutInfo(TEST_PKG_NAME, USER_ID_PRIMARY, shortcutId,
buildPerson());
shortcut.setCached(ShortcutInfo.FLAG_CACHED_NOTIFICATIONS);
mShortcutChangeCallback.onShortcutsAddedOrUpdated(
TEST_PKG_NAME,
Collections.singletonList(shortcut),
UserHandle.of(USER_ID_PRIMARY));
mLooper.dispatchAll();
}
// Only the shortcut #0 is uncached, all the others are not.
verify(mShortcutServiceInternal).uncacheShortcuts(
anyInt(), any(), eq(TEST_PKG_NAME),
eq(Collections.singletonList(TEST_SHORTCUT_ID + 0)), eq(USER_ID_PRIMARY),
eq(ShortcutInfo.FLAG_CACHED_NOTIFICATIONS));
for (int i = 1; i < DataManager.MAX_CACHED_RECENT_SHORTCUTS + 1; i++) {
verify(mShortcutServiceInternal, never()).uncacheShortcuts(
anyInt(), anyString(), anyString(),
eq(Collections.singletonList(TEST_SHORTCUT_ID + i)), anyInt(),
eq(ShortcutInfo.FLAG_CACHED_NOTIFICATIONS));
}
}
@Test
public void testBackupAndRestoration()
throws IntentFilter.MalformedMimeTypeException {