From f25a518355daf3a594fa186992557b5c5f0a9893 Mon Sep 17 00:00:00 2001 From: Suprabh Shukla Date: Mon, 3 May 2021 23:19:23 -0700 Subject: [PATCH] Use BinderDeathDispatcher for alarm listeners This ensures only one JNI strong reference for the death recipient when the same binder is used for multiple linkToDeath calls. Test: atest CtsAlarmManagerTestCases atest FrameworksCoreTests:BinderDeathDispatcherTest atest FrameworksMockingServicesTests:com.android.server.alarm Fixes: 162961981 Bug: 182971683 Change-Id: Ieea2d8f4e495a4c4687378db9d0b8c6949a0406a --- .../server/alarm/AlarmManagerService.java | 14 ++++- .../internal/os/BinderDeathDispatcher.java | 14 +++-- .../java/com/android/internal/os/TEST_MAPPING | 10 +++ .../os/BinderDeathDispatcherTest.java | 62 +++++++++++++------ .../server/content/ContentService.java | 8 ++- 5 files changed, 77 insertions(+), 31 deletions(-) diff --git a/apex/jobscheduler/service/java/com/android/server/alarm/AlarmManagerService.java b/apex/jobscheduler/service/java/com/android/server/alarm/AlarmManagerService.java index 64686a1682f0d..308866f542cc9 100644 --- a/apex/jobscheduler/service/java/com/android/server/alarm/AlarmManagerService.java +++ b/apex/jobscheduler/service/java/com/android/server/alarm/AlarmManagerService.java @@ -118,6 +118,7 @@ import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.app.IAppOpsCallback; import com.android.internal.app.IAppOpsService; +import com.android.internal.os.BinderDeathDispatcher; import com.android.internal.util.DumpUtils; import com.android.internal.util.FrameworkStatsLog; import com.android.internal.util.LocalLog; @@ -202,6 +203,8 @@ public class AlarmManagerService extends SystemService { .addFlags(Intent.FLAG_RECEIVER_REPLACE_PENDING | Intent.FLAG_RECEIVER_INCLUDE_BACKGROUND); + private static final BinderDeathDispatcher sListenerDeathDispatcher = + new BinderDeathDispatcher<>(); final LocalLog mLog = new LocalLog(TAG); AppOpsManager mAppOps; @@ -1809,9 +1812,8 @@ public class AlarmManagerService extends SystemService { } if (directReceiver != null) { - try { - directReceiver.asBinder().linkToDeath(mListenerDeathRecipient, 0); - } catch (RemoteException e) { + if (sListenerDeathDispatcher.linkToDeath(directReceiver, mListenerDeathRecipient) + <= 0) { Slog.w(TAG, "Dropping unreachable alarm listener " + listenerTag); return; } @@ -2824,6 +2826,12 @@ public class AlarmManagerService extends SystemService { pw.println(); } + pw.println("Listener death dispatcher state:"); + pw.increaseIndent(); + sListenerDeathDispatcher.dump(pw); + pw.println(); + pw.decreaseIndent(); + if (mLog.dump(pw, "Recent problems:")) { pw.println(); } diff --git a/core/java/com/android/internal/os/BinderDeathDispatcher.java b/core/java/com/android/internal/os/BinderDeathDispatcher.java index 0c93f7f160e41..8ca6241e63c66 100644 --- a/core/java/com/android/internal/os/BinderDeathDispatcher.java +++ b/core/java/com/android/internal/os/BinderDeathDispatcher.java @@ -24,12 +24,11 @@ import android.os.IInterface; import android.os.RemoteException; import android.util.ArrayMap; import android.util.ArraySet; +import android.util.IndentingPrintWriter; import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; -import java.io.PrintWriter; - /** * Multiplexes multiple binder death recipients on the same binder objects, so that at the native * level, we only need to keep track of one death recipient reference. This will help reduce the @@ -63,6 +62,10 @@ public class BinderDeathDispatcher { @Override public void binderDied() { + } + + @Override + public void binderDied(IBinder who) { final ArraySet copy; synchronized (mLock) { copy = mRecipients; @@ -77,7 +80,7 @@ public class BinderDeathDispatcher { // Let's call it without holding the lock. final int size = copy.size(); for (int i = 0; i < size; i++) { - copy.valueAt(i).binderDied(); + copy.valueAt(i).binderDied(who); } } } @@ -124,13 +127,12 @@ public class BinderDeathDispatcher { } } - public void dump(PrintWriter pw, String indent) { + /** Dump stats useful for debugging. Can be used from dump() methods of client services. */ + public void dump(IndentingPrintWriter pw) { synchronized (mLock) { - pw.print(indent); pw.print("# of watched binders: "); pw.println(mTargets.size()); - pw.print(indent); pw.print("# of death recipients: "); int n = 0; for (RecipientsInfo info : mTargets.values()) { diff --git a/core/java/com/android/internal/os/TEST_MAPPING b/core/java/com/android/internal/os/TEST_MAPPING index 14cdb0890b25f..2b22f08652e73 100644 --- a/core/java/com/android/internal/os/TEST_MAPPING +++ b/core/java/com/android/internal/os/TEST_MAPPING @@ -12,6 +12,16 @@ { "exclude-annotation": "com.android.internal.os.SkipPresubmit" } ] }, + { + "file_patterns": [ + "BinderDeathDispatcher\\.java" + ], + "name": "FrameworksCoreTests", + "options": [ + { "include-filter": "com.android.internal.os.BinderDeathDispatcherTest" }, + { "exclude-annotation": "com.android.internal.os.SkipPresubmit" } + ] + }, { "file_patterns": ["Battery[^/]*\\.java"], "name": "FrameworksServicesTests", diff --git a/core/tests/coretests/src/com/android/internal/os/BinderDeathDispatcherTest.java b/core/tests/coretests/src/com/android/internal/os/BinderDeathDispatcherTest.java index 942045c8bf359..83103333f68b4 100644 --- a/core/tests/coretests/src/com/android/internal/os/BinderDeathDispatcherTest.java +++ b/core/tests/coretests/src/com/android/internal/os/BinderDeathDispatcherTest.java @@ -17,6 +17,7 @@ package com.android.internal.os; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; @@ -31,14 +32,14 @@ import android.os.RemoteException; import android.os.ResultReceiver; import android.os.ShellCallback; +import androidx.test.filters.SmallTest; +import androidx.test.runner.AndroidJUnit4; + import org.junit.Test; import org.junit.runner.RunWith; import java.io.FileDescriptor; -import androidx.test.filters.SmallTest; -import androidx.test.runner.AndroidJUnit4; - @SmallTest @RunWith(AndroidJUnit4.class) public class BinderDeathDispatcherTest { @@ -120,7 +121,7 @@ public class BinderDeathDispatcherTest { public void die() { isAlive = false; if (mRecipient != null) { - mRecipient.binderDied(); + mRecipient.binderDied(this); } mRecipient = null; } @@ -227,33 +228,33 @@ public class BinderDeathDispatcherTest { // Kill the targets. t1.die(); - verify(r1, times(1)).binderDied(); - verify(r2, times(1)).binderDied(); - verify(r3, times(1)).binderDied(); - verify(r4, times(0)).binderDied(); - verify(r5, times(0)).binderDied(); + verify(r1, times(1)).binderDied(t1); + verify(r2, times(1)).binderDied(t1); + verify(r3, times(1)).binderDied(t1); + verify(r4, times(0)).binderDied(any()); + verify(r5, times(0)).binderDied(any()); assertThat(d.getTargetsForTest().size()).isEqualTo(2); reset(r1, r2, r3, r4, r5); t2.die(); - verify(r1, times(1)).binderDied(); - verify(r2, times(0)).binderDied(); - verify(r3, times(0)).binderDied(); - verify(r4, times(0)).binderDied(); - verify(r5, times(0)).binderDied(); + verify(r1, times(1)).binderDied(t2); + verify(r2, times(0)).binderDied(any()); + verify(r3, times(0)).binderDied(any()); + verify(r4, times(0)).binderDied(any()); + verify(r5, times(0)).binderDied(any()); assertThat(d.getTargetsForTest().size()).isEqualTo(1); reset(r1, r2, r3, r4, r5); t3.die(); - verify(r1, times(0)).binderDied(); - verify(r2, times(0)).binderDied(); - verify(r3, times(1)).binderDied(); - verify(r4, times(0)).binderDied(); - verify(r5, times(1)).binderDied(); + verify(r1, times(0)).binderDied(any()); + verify(r2, times(0)).binderDied(any()); + verify(r3, times(1)).binderDied(t3); + verify(r4, times(0)).binderDied(any()); + verify(r5, times(1)).binderDied(t3); assertThat(d.getTargetsForTest().size()).isEqualTo(0); @@ -262,4 +263,27 @@ public class BinderDeathDispatcherTest { assertThat(d.getTargetsForTest().size()).isEqualTo(0); } + + @Test + public void duplicateRegistrations() { + BinderDeathDispatcher d = new BinderDeathDispatcher<>(); + + MyTarget t1 = new MyTarget(); + + DeathRecipient r1 = mock(DeathRecipient.class); + DeathRecipient r2 = mock(DeathRecipient.class); + + for (int i = 0; i < 5; i++) { + assertThat(d.linkToDeath(t1, r1)).isEqualTo(1); + } + assertThat(d.linkToDeath(t1, r2)).isEqualTo(2); + + t1.die(); + verify(r1, times(1)).binderDied(t1); + verify(r2, times(1)).binderDied(t1); + + d.unlinkToDeath(t1, r1); + d.unlinkToDeath(t1, r2); + assertThat(d.getTargetsForTest()).isEmpty(); + } } diff --git a/services/core/java/com/android/server/content/ContentService.java b/services/core/java/com/android/server/content/ContentService.java index 17f39ddac4fd8..c3c42bae55b0b 100644 --- a/services/core/java/com/android/server/content/ContentService.java +++ b/services/core/java/com/android/server/content/ContentService.java @@ -261,10 +261,12 @@ public final class ContentService extends IContentService.Stub { pw.print(pidCounts.get(pid)); pw.println(" observers"); } pw.println(); - pw.print(" Total number of nodes: "); pw.println(counts[0]); - pw.print(" Total number of observers: "); pw.println(counts[1]); + pw.increaseIndent(); + pw.print("Total number of nodes: "); pw.println(counts[0]); + pw.print("Total number of observers: "); pw.println(counts[1]); - sObserverDeathDispatcher.dump(pw, " "); + sObserverDeathDispatcher.dump(pw); + pw.decreaseIndent(); } synchronized (sObserverLeakDetectedUid) { pw.println();