Closes NotificationRankingUpdate fd after write
Adds close and unmap buffer to NotificationRankingUpdate writeToParcel.
This should be safe to do (and good to prevent memory leaks) because
there's a per-process counter on the SharedMemory fd, and the
NotificationListeners who will receive this update have the pointer, so
the memory shouldn't actually be cleared.
Test: atest NotificationRankingUpdateTest, atest SystemUIMicrobenchmark:android.platform.test.scenario.notification.PostAndClearNotificationMicrobenchmark
Bug: 308308726
Change-Id: I70a5d31d0969cc82ef5462865ad035093c757017
diff --git a/core/java/android/service/notification/NotificationRankingUpdate.java b/core/java/android/service/notification/NotificationRankingUpdate.java
index 2a4cbaf..46ea158 100644
--- a/core/java/android/service/notification/NotificationRankingUpdate.java
+++ b/core/java/android/service/notification/NotificationRankingUpdate.java
@@ -15,7 +15,6 @@
*/
package android.service.notification;
-import android.annotation.Nullable;
import android.annotation.SuppressLint;
import android.app.Notification;
import android.os.Bundle;
@@ -26,6 +25,7 @@
import android.system.OsConstants;
import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;
import java.nio.ByteBuffer;
@@ -75,10 +75,6 @@
}
// We only need read-only access to the shared memory region.
buffer = mRankingMapFd.mapReadOnly();
- if (buffer == null) {
- mRankingMap = null;
- return;
- }
byte[] payload = new byte[buffer.remaining()];
buffer.get(payload);
mapParcel.unmarshall(payload, 0, payload.length);
@@ -98,7 +94,7 @@
} finally {
mapParcel.recycle();
if (buffer != null && mRankingMapFd != null) {
- mRankingMapFd.unmap(buffer);
+ SharedMemory.unmap(buffer);
mRankingMapFd.close();
}
}
@@ -210,6 +206,7 @@
new NotificationListenerService.Ranking[0]
)
);
+ ByteBuffer buffer = null;
try {
// Parcels the ranking map and measures its size.
@@ -217,13 +214,10 @@
int mapSize = mapParcel.dataSize();
// Creates a new SharedMemory object with enough space to hold the ranking map.
- SharedMemory mRankingMapFd = SharedMemory.create(mSharedMemoryName, mapSize);
- if (mRankingMapFd == null) {
- return;
- }
+ mRankingMapFd = SharedMemory.create(mSharedMemoryName, mapSize);
// Gets a read/write buffer mapping the entire shared memory region.
- final ByteBuffer buffer = mRankingMapFd.mapReadWrite();
+ buffer = mRankingMapFd.mapReadWrite();
// Puts the ranking map into the shared memory region buffer.
buffer.put(mapParcel.marshall(), 0, mapSize);
// Protects the region from being written to, by setting it to be read-only.
@@ -238,6 +232,12 @@
throw new RuntimeException(e);
} finally {
mapParcel.recycle();
+ // To prevent memory leaks, we can close the ranking map fd here.
+ // Because a reference to this still exists
+ if (buffer != null && mRankingMapFd != null) {
+ SharedMemory.unmap(buffer);
+ mRankingMapFd.close();
+ }
}
} else {
out.writeParcelable(mRankingMap, flags);
@@ -247,7 +247,7 @@
/**
* @hide
*/
- public static final @android.annotation.NonNull Parcelable.Creator<NotificationRankingUpdate> CREATOR
+ public static final @NonNull Parcelable.Creator<NotificationRankingUpdate> CREATOR
= new Parcelable.Creator<NotificationRankingUpdate>() {
public NotificationRankingUpdate createFromParcel(Parcel parcel) {
return new NotificationRankingUpdate(parcel);
diff --git a/core/tests/coretests/src/android/service/notification/NotificationRankingUpdateTest.java b/core/tests/coretests/src/android/service/notification/NotificationRankingUpdateTest.java
index 0855268..1bdb006 100644
--- a/core/tests/coretests/src/android/service/notification/NotificationRankingUpdateTest.java
+++ b/core/tests/coretests/src/android/service/notification/NotificationRankingUpdateTest.java
@@ -472,6 +472,9 @@
NotificationRankingUpdate nru = generateUpdate(getContext());
Parcel parcel = Parcel.obtain();
nru.writeToParcel(parcel, 0);
+ if (Flags.rankingUpdateAshmem()) {
+ assertTrue(nru.isFdNotNullAndClosed());
+ }
parcel.setDataPosition(0);
NotificationRankingUpdate nru1 = NotificationRankingUpdate.CREATOR.createFromParcel(parcel);
// The rankingUpdate file descriptor is only non-null in the new path.
