From 2e89e8d893acfe571ad6f5555baccb1b5e55abb7 Mon Sep 17 00:00:00 2001 From: Chris Wren Date: Thu, 17 May 2018 18:55:42 -0400 Subject: [PATCH] clone the visibility objects for the handler thread The main thread was recycling the objects before the hander could pack up the binder call. Change-Id: I4289bdcc5b940a0a8209fdd5d3df47972de0fa4b Fixes: 72953296 Test: atest com.android.notification.functional.NotificationInteractionTests#testNotificationShadeMetrics --- .../internal/statusbar/NotificationVisibility.java | 2 +- .../systemui/statusbar/NotificationLogger.java | 31 ++++++++++++++++--- .../systemui/statusbar/NotificationLoggerTest.java | 35 ++++++++++++++++++---- 3 files changed, 57 insertions(+), 11 deletions(-) diff --git a/core/java/com/android/internal/statusbar/NotificationVisibility.java b/core/java/com/android/internal/statusbar/NotificationVisibility.java index 7fe440cf0b89..ea0344d1dadf 100644 --- a/core/java/com/android/internal/statusbar/NotificationVisibility.java +++ b/core/java/com/android/internal/statusbar/NotificationVisibility.java @@ -51,7 +51,7 @@ public class NotificationVisibility implements Parcelable { @Override public String toString() { return "NotificationVisibility(id=" + id - + "key=" + key + + " key=" + key + " rank=" + rank + " count=" + count + (visible?" visible":"") diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLogger.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLogger.java index 01ec46151b38..8e8e7180a8ab 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLogger.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLogger.java @@ -176,10 +176,9 @@ public class NotificationLogger { if (newlyVisible.isEmpty() && noLongerVisible.isEmpty()) { return; } - NotificationVisibility[] newlyVisibleAr = - newlyVisible.toArray(new NotificationVisibility[newlyVisible.size()]); - NotificationVisibility[] noLongerVisibleAr = - noLongerVisible.toArray(new NotificationVisibility[noLongerVisible.size()]); + final NotificationVisibility[] newlyVisibleAr = cloneVisibilitiesAsArr(newlyVisible); + final NotificationVisibility[] noLongerVisibleAr = cloneVisibilitiesAsArr(noLongerVisible); + mUiOffloadThread.submit(() -> { try { mBarService.onNotificationVisibilityChanged(newlyVisibleAr, noLongerVisibleAr); @@ -202,6 +201,8 @@ public class NotificationLogger { Log.d(TAG, "failed setNotificationsShown: ", e); } } + recycleAllVisibilityObjects(newlyVisibleAr); + recycleAllVisibilityObjects(noLongerVisibleAr); }); } @@ -213,6 +214,28 @@ public class NotificationLogger { array.clear(); } + private void recycleAllVisibilityObjects(NotificationVisibility[] array) { + final int N = array.length; + for (int i = 0 ; i < N; i++) { + if (array[i] != null) { + array[i].recycle(); + } + } + } + + private NotificationVisibility[] cloneVisibilitiesAsArr(Collection c) { + + final NotificationVisibility[] array = new NotificationVisibility[c.size()]; + int i = 0; + for(NotificationVisibility nv: c) { + if (nv != null) { + array[i] = nv.clone(); + } + i++; + } + return array; + } + @VisibleForTesting public Runnable getVisibilityReporter() { return mVisibilityReporter; diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationLoggerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationLoggerTest.java index bbb03d78bd03..42bf290c70fd 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationLoggerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationLoggerTest.java @@ -16,7 +16,9 @@ package com.android.systemui.statusbar; +import static org.junit.Assert.assertArrayEquals; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -25,6 +27,7 @@ import static org.mockito.Mockito.when; import android.app.Notification; import android.os.Handler; import android.os.Looper; +import android.os.RemoteException; import android.os.UserHandle; import android.service.notification.StatusBarNotification; import android.support.test.filters.SmallTest; @@ -43,6 +46,9 @@ import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.MockitoAnnotations; +import org.mockito.stubbing.Answer; + +import java.util.concurrent.ConcurrentLinkedQueue; @SmallTest @RunWith(AndroidTestingRunner.class) @@ -64,9 +70,10 @@ public class NotificationLoggerTest extends SysuiTestCase { private NotificationData.Entry mEntry; private StatusBarNotification mSbn; private TestableNotificationLogger mLogger; + private ConcurrentLinkedQueue mErrorQueue = new ConcurrentLinkedQueue<>(); @Before - public void setUp() { + public void setUp() throws RemoteException { MockitoAnnotations.initMocks(this); mDependency.injectTestDependency(NotificationEntryManager.class, mEntryManager); mDependency.injectTestDependency(NotificationListener.class, mListener); @@ -84,17 +91,33 @@ public class NotificationLoggerTest extends SysuiTestCase { @Test public void testOnChildLocationsChangedReportsVisibilityChanged() throws Exception { + NotificationVisibility[] newlyVisibleKeys = { + NotificationVisibility.obtain(mEntry.key, 0, 1, true) + }; + NotificationVisibility[] noLongerVisibleKeys = {}; + doAnswer((Answer) invocation -> { + try { + assertArrayEquals(newlyVisibleKeys, + (NotificationVisibility[]) invocation.getArguments()[0]); + assertArrayEquals(noLongerVisibleKeys, + (NotificationVisibility[]) invocation.getArguments()[1]); + } catch (AssertionError error) { + mErrorQueue.offer(error); + } + return null; + } + ).when(mBarService).onNotificationVisibilityChanged(any(NotificationVisibility[].class), + any(NotificationVisibility[].class)); + when(mListContainer.isInVisibleLocation(any())).thenReturn(true); when(mNotificationData.getActiveNotifications()).thenReturn(Lists.newArrayList(mEntry)); mLogger.getChildLocationsChangedListenerForTest().onChildLocationsChanged(); TestableLooper.get(this).processAllMessages(); waitForUiOffloadThread(); - NotificationVisibility[] newlyVisibleKeys = { - NotificationVisibility.obtain(mEntry.key, 0, 1, true) - }; - NotificationVisibility[] noLongerVisibleKeys = {}; - verify(mBarService).onNotificationVisibilityChanged(newlyVisibleKeys, noLongerVisibleKeys); + if(!mErrorQueue.isEmpty()) { + throw mErrorQueue.poll(); + } // |mEntry| won't change visibility, so it shouldn't be reported again: Mockito.reset(mBarService); -- 2.11.0