From af67563cb06453b59eede4d5455d11b1fb9eca96 Mon Sep 17 00:00:00 2001 From: Julia Reynolds Date: Sat, 6 Jun 2020 20:04:21 -0400 Subject: [PATCH] Stop overwriting files An earlier refactoring to write a test led to the same file being used for incoming notifications, which meant the older notifications were lost. Also fixes some mostly innocuous errors seen in the log when looking for the root cause of this problem -USER_ALL notifications weren't being written -AtomicFile should be closed -Files cleaned up because device has remained on beyond timeout should be removed from tracking list -Don't try to recreate a folder that exists Test: atest, and monitor created files and notis in history over an hour or two Fixes: 158300639 Change-Id: I3eb0e26863d1ab816a67cf509147674b90e2b6f4 --- .../NotificationHistoryDatabase.java | 58 +++++++++++-------- .../NotificationManagerService.java | 2 +- .../NotificationHistoryDatabaseTest.java | 7 +-- 3 files changed, 37 insertions(+), 30 deletions(-) diff --git a/services/core/java/com/android/server/notification/NotificationHistoryDatabase.java b/services/core/java/com/android/server/notification/NotificationHistoryDatabase.java index e846daf7a2e66..0ec4b39d89608 100644 --- a/services/core/java/com/android/server/notification/NotificationHistoryDatabase.java +++ b/services/core/java/com/android/server/notification/NotificationHistoryDatabase.java @@ -108,7 +108,7 @@ public class NotificationHistoryDatabase { public void init() { synchronized (mLock) { try { - if (!mHistoryDir.mkdir()) { + if (!mHistoryDir.exists() && !mHistoryDir.mkdir()) { throw new IllegalStateException("could not create history directory"); } mVersionFile.createNewFile(); @@ -197,7 +197,7 @@ public class NotificationHistoryDatabase { readLocked( file, notifications, new NotificationHistoryFilter.Builder().build()); } catch (Exception e) { - Slog.e(TAG, "error reading " + file.getBaseFile().getName(), e); + Slog.e(TAG, "error reading " + file.getBaseFile().getAbsolutePath(), e); } } @@ -223,7 +223,7 @@ public class NotificationHistoryDatabase { break; } } catch (Exception e) { - Slog.e(TAG, "error reading " + file.getBaseFile().getName(), e); + Slog.e(TAG, "error reading " + file.getBaseFile().getAbsolutePath(), e); } } @@ -279,7 +279,7 @@ public class NotificationHistoryDatabase { } file.delete(); // TODO: delete all relevant bitmaps, once they exist - mHistoryFiles.removeLast(); + mHistoryFiles.remove(file); } private void scheduleDeletion(File file, long creationTime, int retentionDays) { @@ -317,11 +317,17 @@ public class NotificationHistoryDatabase { private static void readLocked(AtomicFile file, NotificationHistory notificationsOut, NotificationHistoryFilter filter) throws IOException { - try (FileInputStream in = file.openRead()) { + FileInputStream in = null; + try { + in = file.openRead(); NotificationHistoryProtoHelper.read(in, notificationsOut, filter); } catch (FileNotFoundException e) { - Slog.e(TAG, "Cannot file " + file.getBaseFile().getName(), e); + Slog.e(TAG, "Cannot open " + file.getBaseFile().getAbsolutePath(), e); throw e; + } finally { + if (in != null) { + in.close(); + } } } @@ -334,9 +340,15 @@ public class NotificationHistoryDatabase { } if (ACTION_HISTORY_DELETION.equals(action)) { try { - final String filePath = intent.getStringExtra(EXTRA_KEY); - AtomicFile fileToDelete = new AtomicFile(new File(filePath)); - fileToDelete.delete(); + synchronized (mLock) { + final String filePath = intent.getStringExtra(EXTRA_KEY); + AtomicFile fileToDelete = new AtomicFile(new File(filePath)); + if (DEBUG) { + Slog.d(TAG, "Removed " + fileToDelete.getBaseFile().getName()); + } + fileToDelete.delete(); + mHistoryFiles.remove(fileToDelete); + } } catch (Exception e) { Slog.e(TAG, "Failed to delete notification history file", e); } @@ -345,27 +357,23 @@ public class NotificationHistoryDatabase { }; final class WriteBufferRunnable implements Runnable { - long currentTime = 0; - AtomicFile latestNotificationsFile; @Override public void run() { - if (DEBUG) Slog.d(TAG, "WriteBufferRunnable"); + long time = System.currentTimeMillis(); + run(time, new AtomicFile(new File(mHistoryDir, String.valueOf(time)))); + } + + void run(long time, AtomicFile file) { synchronized (mLock) { - if (currentTime == 0) { - currentTime = System.currentTimeMillis(); - } - if (latestNotificationsFile == null) { - latestNotificationsFile = new AtomicFile( - new File(mHistoryDir, String.valueOf(currentTime))); - } + if (DEBUG) Slog.d(TAG, "WriteBufferRunnable " + + file.getBaseFile().getAbsolutePath()); try { - writeLocked(latestNotificationsFile, mBuffer); - mHistoryFiles.addFirst(latestNotificationsFile); + writeLocked(file, mBuffer); + mHistoryFiles.addFirst(file); mBuffer = new NotificationHistory(); - scheduleDeletion(latestNotificationsFile.getBaseFile(), currentTime, - HISTORY_RETENTION_DAYS); + scheduleDeletion(file.getBaseFile(), time, HISTORY_RETENTION_DAYS); } catch (IOException e) { Slog.e(TAG, "Failed to write buffer to disk. not flushing buffer", e); } @@ -382,7 +390,7 @@ public class NotificationHistoryDatabase { @Override public void run() { - if (DEBUG) Slog.d(TAG, "RemovePackageRunnable"); + if (DEBUG) Slog.d(TAG, "RemovePackageRunnable " + mPkg); synchronized (mLock) { // Remove packageName entries from pending history mBuffer.removeNotificationsFromWrite(mPkg); @@ -398,7 +406,7 @@ public class NotificationHistoryDatabase { writeLocked(af, notifications); } catch (Exception e) { Slog.e(TAG, "Cannot clean up file on pkg removal " - + af.getBaseFile().getName(), e); + + af.getBaseFile().getAbsolutePath(), e); } } } diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index a95dc30352006..5585e98167832 100755 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -2672,7 +2672,7 @@ public class NotificationManagerService extends SystemService { mHistoryManager.addNotification(new HistoricalNotification.Builder() .setPackage(r.getSbn().getPackageName()) .setUid(r.getSbn().getUid()) - .setUserId(r.getUserId()) + .setUserId(r.getSbn().getNormalizedUserId()) .setChannelId(r.getChannel().getId()) .setChannelName(r.getChannel().getName().toString()) .setPostedTimeMs(System.currentTimeMillis()) diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationHistoryDatabaseTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationHistoryDatabaseTest.java index 80b474ff71281..6991c18ceb82d 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationHistoryDatabaseTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationHistoryDatabaseTest.java @@ -360,13 +360,12 @@ public class NotificationHistoryDatabaseTest extends UiServiceTestCase { mDataBase.new WriteBufferRunnable(); mDataBase.mBuffer = nh; - wbr.currentTime = 5; - wbr.latestNotificationsFile = mock(AtomicFile.class); + AtomicFile af = mock(AtomicFile.class); File file = mock(File.class); when(file.getName()).thenReturn("5"); - when(wbr.latestNotificationsFile.getBaseFile()).thenReturn(file); + when(af.getBaseFile()).thenReturn(file); - wbr.run(); + wbr.run(5, af); assertThat(mDataBase.mHistoryFiles.size()).isEqualTo(1); assertThat(mDataBase.mBuffer).isNotEqualTo(nh);