Merge "Avoid creating multiple death recipients for same observer" into qt-dev (a478c5f9) · Commits · e / os / android_frameworks_base

core/java/com/android/internal/os/BinderDeathDispatcher.java

0 → 100644

+147 −0

Original line number	Diff line number	Diff line
		/*
		* Copyright (C) 2019 The Android Open Source Project
		*
		* Licensed under the Apache License, Version 2.0 (the "License");
		* you may not use this file except in compliance with the License.
		* You may obtain a copy of the License at
		*
		* http://www.apache.org/licenses/LICENSE-2.0
		*
		* Unless required by applicable law or agreed to in writing, software
		* distributed under the License is distributed on an "AS IS" BASIS,
		* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
		* See the License for the specific language governing permissions and
		* limitations under the License.
		*/

		package com.android.internal.os;

		import android.annotation.NonNull;
		import android.annotation.Nullable;
		import android.os.IBinder;
		import android.os.IBinder.DeathRecipient;
		import android.os.IInterface;
		import android.os.RemoteException;
		import android.util.ArrayMap;
		import android.util.ArraySet;

		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
		* number of JNI strong references.
		*
		* test with: atest FrameworksCoreTests:BinderDeathDispatcherTest
		*/
		public class BinderDeathDispatcher<T extends IInterface> {
		private static final String TAG = "BinderDeathDispatcher";

		private final Object mLock = new Object();

		@GuardedBy("mLock")
		private final ArrayMap<IBinder, RecipientsInfo> mTargets = new ArrayMap<>();

		@VisibleForTesting
		class RecipientsInfo implements DeathRecipient {
		final IBinder mTarget;

		/**
		* Recipient list. If it's null, {@link #mTarget} has already died, but in that case
		* this RecipientsInfo instance is removed from {@link #mTargets}.
		*/
		@GuardedBy("mLock")
		@Nullable
		ArraySet<DeathRecipient> mRecipients = new ArraySet<>();

		private RecipientsInfo(IBinder target) {
		mTarget = target;
		}

		@Override
		public void binderDied() {
		final ArraySet<DeathRecipient> copy;
		synchronized (mLock) {
		copy = mRecipients;
		mRecipients = null;

		// Also remove from the targets.
		mTargets.remove(mTarget);
		}
		if (copy == null) {
		return;
		}
		// Let's call it without holding the lock.
		final int size = copy.size();
		for (int i = 0; i < size; i++) {
		copy.valueAt(i).binderDied();
		}
		}
		}

		/**
		* Add a {@code recipient} to the death recipient list on {@code target}.
		*
		* @return # of recipients in the recipient list, including {@code recipient}. Or, -1
		* if {@code target} is already dead, in which case recipient's
		* {@link DeathRecipient#binderDied} won't be called.
		*/
		public int linkToDeath(@NonNull T target, @NonNull DeathRecipient recipient) {
		final IBinder ib = target.asBinder();
		synchronized (mLock) {
		RecipientsInfo info = mTargets.get(ib);
		if (info == null) {
		info = new RecipientsInfo(ib);

		// First recipient; need to link to death.
		try {
		ib.linkToDeath(info, 0);
		} catch (RemoteException e) {
		return -1; // Already dead.
		}
		mTargets.put(ib, info);
		}
		info.mRecipients.add(recipient);
		return info.mRecipients.size();
		}
		}

		public void unlinkToDeath(@NonNull T target, @NonNull DeathRecipient recipient) {
		final IBinder ib = target.asBinder();

		synchronized (mLock) {
		final RecipientsInfo info = mTargets.get(ib);
		if (info == null) {
		return;
		}
		if (info.mRecipients.remove(recipient) && info.mRecipients.size() == 0) {
		info.mTarget.unlinkToDeath(info, 0);
		mTargets.remove(info.mTarget);
		}
		}
		}

		public void dump(PrintWriter pw, String indent) {
		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()) {
		n += info.mRecipients.size();
		}
		pw.println(n);
		}
		}

		@VisibleForTesting
		public ArrayMap<IBinder, RecipientsInfo> getTargetsForTest() {
		return mTargets;
		}
		}

core/java/com/android/internal/util/ArrayUtils.java

+4 −0

Original line number	Diff line number	Diff line
		@@ -756,4 +756,8 @@ public class ArrayUtils {
		return String.valueOf(value);
		}
		}

		public static @Nullable <T> T firstOrNull(T[] items) {
		return items.length > 0 ? items[0] : null;
		}
		}

core/tests/coretests/src/com/android/internal/os/BinderDeathDispatcherTest.java

0 → 100644

+265 −0

Original line number	Diff line number	Diff line
		/*
		* Copyright (C) 2019 The Android Open Source Project
		*
		* Licensed under the Apache License, Version 2.0 (the "License");
		* you may not use this file except in compliance with the License.
		* You may obtain a copy of the License at
		*
		* http://www.apache.org/licenses/LICENSE-2.0
		*
		* Unless required by applicable law or agreed to in writing, software
		* distributed under the License is distributed on an "AS IS" BASIS,
		* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
		* See the License for the specific language governing permissions and
		* limitations under the License.
		*/
		package com.android.internal.os;

		import static com.google.common.truth.Truth.assertThat;

		import static org.mockito.Mockito.mock;
		import static org.mockito.Mockito.reset;
		import static org.mockito.Mockito.times;
		import static org.mockito.Mockito.verify;

		import android.os.DeadObjectException;
		import android.os.IBinder;
		import android.os.IBinder.DeathRecipient;
		import android.os.IInterface;
		import android.os.Parcel;
		import android.os.RemoteException;
		import android.os.ResultReceiver;
		import android.os.ShellCallback;

		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 {
		private static class MyTarget implements IInterface, IBinder {
		public boolean isAlive = true;
		public DeathRecipient mRecipient;

		@Override
		public String getInterfaceDescriptor() throws RemoteException {
		return null;
		}

		@Override
		public boolean pingBinder() {
		return false;
		}

		@Override
		public boolean isBinderAlive() {
		return isAlive;
		}

		@Override
		public IInterface queryLocalInterface(String descriptor) {
		return null;
		}

		@Override
		public void dump(FileDescriptor fd, String[] args) throws RemoteException {

		}

		@Override
		public void dumpAsync(FileDescriptor fd, String[] args) throws RemoteException {

		}

		@Override
		public void shellCommand(FileDescriptor in, FileDescriptor out, FileDescriptor err,
		String[] args, ShellCallback shellCallback, ResultReceiver resultReceiver)
		throws RemoteException {

		}

		@Override
		public boolean transact(int code, Parcel data, Parcel reply, int flags)
		throws RemoteException {
		return false;
		}

		@Override
		public void linkToDeath(DeathRecipient recipient, int flags) throws RemoteException {
		// In any situation, a single binder object should only have at most one death
		// recipient.
		assertThat(mRecipient).isNull();

		if (!isAlive) {
		throw new DeadObjectException();
		}

		mRecipient = recipient;
		}

		@Override
		public boolean unlinkToDeath(DeathRecipient recipient, int flags) {
		if (!isAlive) {
		return false;
		}
		assertThat(mRecipient).isSameAs(recipient);
		mRecipient = null;
		return true;
		}

		@Override
		public IBinder asBinder() {
		return this;
		}

		public void die() {
		isAlive = false;
		if (mRecipient != null) {
		mRecipient.binderDied();
		}
		mRecipient = null;
		}

		public boolean hasDeathRecipient() {
		return mRecipient != null;
		}
		}

		@Test
		public void testRegisterAndUnregister() {
		BinderDeathDispatcher<MyTarget> d = new BinderDeathDispatcher<>();

		MyTarget t1 = new MyTarget();
		MyTarget t2 = new MyTarget();
		MyTarget t3 = new MyTarget();

		DeathRecipient r1 = mock(DeathRecipient.class);
		DeathRecipient r2 = mock(DeathRecipient.class);
		DeathRecipient r3 = mock(DeathRecipient.class);
		DeathRecipient r4 = mock(DeathRecipient.class);
		DeathRecipient r5 = mock(DeathRecipient.class);

		// Start hooking up.

		// Link 3 recipients to t1 -- only one real recipient will be set.
		assertThat(d.linkToDeath(t1, r1)).isEqualTo(1);
		assertThat(d.getTargetsForTest().size()).isEqualTo(1);

		assertThat(d.linkToDeath(t1, r2)).isEqualTo(2);
		assertThat(d.linkToDeath(t1, r3)).isEqualTo(3);
		assertThat(d.getTargetsForTest().size()).isEqualTo(1);

		// Unlink two -- the real recipient is still set.
		d.unlinkToDeath(t1, r1);
		d.unlinkToDeath(t1, r2);

		assertThat(t1.hasDeathRecipient()).isTrue();
		assertThat(d.getTargetsForTest().size()).isEqualTo(1);

		// Unlink the last one. The real recipient is also unlinked.
		d.unlinkToDeath(t1, r3);
		assertThat(t1.hasDeathRecipient()).isFalse();
		assertThat(d.getTargetsForTest().size()).isEqualTo(0);

		// Set recipients to t1, t2 and t3. t3 has two.
		assertThat(d.linkToDeath(t1, r1)).isEqualTo(1);
		assertThat(d.linkToDeath(t2, r1)).isEqualTo(1);
		assertThat(d.linkToDeath(t3, r1)).isEqualTo(1);
		assertThat(d.linkToDeath(t3, r2)).isEqualTo(2);


		// They should all have a real recipient.
		assertThat(t1.hasDeathRecipient()).isTrue();
		assertThat(t2.hasDeathRecipient()).isTrue();
		assertThat(t3.hasDeathRecipient()).isTrue();

		assertThat(d.getTargetsForTest().size()).isEqualTo(3);

		// Unlink r1 from t3. t3 still has r2, so it should still have a real recipient.
		d.unlinkToDeath(t3, r1);
		assertThat(t1.hasDeathRecipient()).isTrue();
		assertThat(t2.hasDeathRecipient()).isTrue();
		assertThat(t3.hasDeathRecipient()).isTrue();
		assertThat(d.getTargetsForTest().size()).isEqualTo(3);

		// Unlink r2 from t3. Now t3 has no real recipient.
		d.unlinkToDeath(t3, r2);
		assertThat(t3.hasDeathRecipient()).isFalse();
		assertThat(d.getTargetsForTest().size()).isEqualTo(2);
		}

		@Test
		public void testRegisterAndKill() {
		BinderDeathDispatcher<MyTarget> d = new BinderDeathDispatcher<>();

		MyTarget t1 = new MyTarget();
		MyTarget t2 = new MyTarget();
		MyTarget t3 = new MyTarget();

		DeathRecipient r1 = mock(DeathRecipient.class);
		DeathRecipient r2 = mock(DeathRecipient.class);
		DeathRecipient r3 = mock(DeathRecipient.class);
		DeathRecipient r4 = mock(DeathRecipient.class);
		DeathRecipient r5 = mock(DeathRecipient.class);

		// Hook them up.

		d.linkToDeath(t1, r1);
		d.linkToDeath(t1, r2);
		d.linkToDeath(t1, r3);

		// r4 is linked then unlinked. It shouldn't be notified.
		d.linkToDeath(t1, r4);
		d.unlinkToDeath(t1, r4);

		d.linkToDeath(t2, r1);

		d.linkToDeath(t3, r3);
		d.linkToDeath(t3, r5);

		assertThat(d.getTargetsForTest().size()).isEqualTo(3);

		// 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();

		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();

		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();

		assertThat(d.getTargetsForTest().size()).isEqualTo(0);

		// Try to register to a dead object -> should return -1.
		assertThat(d.linkToDeath(t1, r1)).isEqualTo(-1);

		assertThat(d.getTargetsForTest().size()).isEqualTo(0);
		}
		}

services/core/java/com/android/server/content/ContentService.java

+47 −6

Original line number	Diff line number	Diff line
		@@ -22,6 +22,7 @@ import android.annotation.Nullable;
		import android.annotation.RequiresPermission;
		import android.app.ActivityManager;
		import android.app.ActivityManagerInternal;
		import android.app.AppGlobals;
		import android.app.AppOpsManager;
		import android.app.job.JobInfo;
		import android.content.BroadcastReceiver;
		@@ -56,6 +57,7 @@ import android.os.ShellCallback;
		import android.os.UserHandle;
		import android.text.TextUtils;
		import android.util.ArrayMap;
		import android.util.ArraySet;
		import android.util.Log;
		import android.util.Pair;
		import android.util.Slog;
		@@ -63,6 +65,7 @@ import android.util.SparseArray;
		import android.util.SparseIntArray;

		import com.android.internal.annotations.GuardedBy;
		import com.android.internal.os.BinderDeathDispatcher;
		import com.android.internal.util.ArrayUtils;
		import com.android.internal.util.DumpUtils;
		import com.android.internal.util.IndentingPrintWriter;
		@@ -83,6 +86,9 @@ public final class ContentService extends IContentService.Stub {
		static final String TAG = "ContentService";
		static final boolean DEBUG = false;

		/** Do a WTF if a single observer is registered more than this times. */
		private static final int TOO_MANY_OBSERVERS_THRESHOLD = 1000;

		public static class Lifecycle extends SystemService {
		private ContentService mService;

		@@ -135,6 +141,12 @@ public final class ContentService extends IContentService.Stub {
		private SyncManager mSyncManager = null;
		private final Object mSyncManagerLock = new Object();

		private static final BinderDeathDispatcher<IContentObserver> sObserverDeathDispatcher =
		new BinderDeathDispatcher<>();

		@GuardedBy("sObserverLeakDetectedUid")
		private static final ArraySet<Integer> sObserverLeakDetectedUid = new ArraySet<>(0);

		/**
		* Map from userId to providerPackageName to [clientPackageName, uri] to
		* value. This structure is carefully optimized to keep invalidation logic
		@@ -236,6 +248,13 @@ public final class ContentService extends IContentService.Stub {
		pw.println();
		pw.print(" Total number of nodes: "); pw.println(counts[0]);
		pw.print(" Total number of observers: "); pw.println(counts[1]);

		sObserverDeathDispatcher.dump(pw, " ");
		}
		synchronized (sObserverLeakDetectedUid) {
		pw.println();
		pw.print("Observer leaking UIDs: ");
		pw.println(sObserverLeakDetectedUid.toString());
		}

		synchronized (mCache) {
		@@ -1345,20 +1364,42 @@ public final class ContentService extends IContentService.Stub {
		private final Object observersLock;

		public ObserverEntry(IContentObserver o, boolean n, Object observersLock,
		int _uid, int _pid, int _userHandle) {
		int _uid, int _pid, int _userHandle, Uri uri) {
		this.observersLock = observersLock;
		observer = o;
		uid = _uid;
		pid = _pid;
		userHandle = _userHandle;
		notifyForDescendants = n;
		try {
		observer.asBinder().linkToDeath(this, 0);
		} catch (RemoteException e) {

		final int entries = sObserverDeathDispatcher.linkToDeath(observer, this);
		if (entries == -1) {
		binderDied();
		} else if (entries == TOO_MANY_OBSERVERS_THRESHOLD) {
		boolean alreadyDetected;

		synchronized (sObserverLeakDetectedUid) {
		alreadyDetected = sObserverLeakDetectedUid.contains(uid);
		if (!alreadyDetected) {
		sObserverLeakDetectedUid.add(uid);
		}
		}
		if (!alreadyDetected) {
		String caller = null;
		try {
		caller = ArrayUtils.firstOrNull(AppGlobals.getPackageManager()
		.getPackagesForUid(uid));
		} catch (RemoteException ignore) {
		}
		Slog.wtf(TAG, "Observer registered too many times. Leak? cpid=" + pid
		+ " cuid=" + uid
		+ " cpkg=" + caller
		+ " url=" + uri);
		}
		}

		}

		@Override
		public void binderDied() {
		synchronized (observersLock) {
		@@ -1454,7 +1495,7 @@ public final class ContentService extends IContentService.Stub {
		// If this is the leaf node add the observer
		if (index == countUriSegments(uri)) {
		mObservers.add(new ObserverEntry(observer, notifyForDescendants, observersLock,
		uid, pid, userHandle));
		uid, pid, userHandle, uri));
		return;
		}

		@@ -1498,7 +1539,7 @@ public final class ContentService extends IContentService.Stub {
		if (entry.observer.asBinder() == observerBinder) {
		mObservers.remove(i);
		// We no longer need to listen for death notifications. Remove it.
		observerBinder.unlinkToDeath(entry, 0);
		sObserverDeathDispatcher.unlinkToDeath(observer, entry);
		break;
		}
		}

services/tests/servicestests/src/com/android/server/content/ObserverNodeTest.java

+1 −1

Original line number	Diff line number	Diff line
		@@ -30,7 +30,7 @@ import com.android.server.content.ContentService.ObserverCall;
		import com.android.server.content.ContentService.ObserverNode;

		/**
		* bit FrameworksServicesTests:com.android.server.content.ObserverNodeTest
		* atest FrameworksServicesTests:com.android.server.content.ObserverNodeTest
		*/
		@SmallTest
		public class ObserverNodeTest extends AndroidTestCase {