From 2ea0d3979e66da459a1ac51f385628716de63af2 Mon Sep 17 00:00:00 2001 From: Griff Hazen Date: Tue, 17 Jun 2014 20:06:45 -0700 Subject: [PATCH] DO NOT MERGE Reduce chance of notification listener dropped messages. The existing code for notification manager/listeners uses a oneway binder api to deliver messages. One problem with this is that notification objects can sometimes get fairly large, and can bump into the oneway binder transaction buffer if many happen at once. To reduce this issue, flip the service into a oneway delivery of a status bar notification holder, whose wrapped content is then immediately fetched upon receipt of the one-way message. This moves the meat of the fetch to be over a two-way interface without changing the properties of which object is actually sent (a tickle solution with lookup key would have changed this) Further research: attempt to chunk notification objects themselves. They can sometimes transfer hundreds of KB over a binder transaction. Bug: 15426276 Change-Id: Ib1a1f4ff848c16f80bcf2ae4dfd2b87a9091f0b2 --- Android.mk | 1 + .../notification/INotificationListener.aidl | 5 ++- .../IStatusBarNotificationHolder.aidl | 24 +++++++++++ .../NotificationListenerService.java | 19 ++++++++- .../NotificationManagerService.java | 41 ++++++++++++++----- 5 files changed, 76 insertions(+), 14 deletions(-) create mode 100644 core/java/android/service/notification/IStatusBarNotificationHolder.aidl diff --git a/Android.mk b/Android.mk index 494ba1bb37805..8fdcea2f8dff2 100644 --- a/Android.mk +++ b/Android.mk @@ -166,6 +166,7 @@ LOCAL_SRC_FILES += \ core/java/android/os/IUserManager.aidl \ core/java/android/os/IVibratorService.aidl \ core/java/android/service/notification/INotificationListener.aidl \ + core/java/android/service/notification/IStatusBarNotificationHolder.aidl \ core/java/android/print/ILayoutResultCallback.aidl \ core/java/android/print/IPrinterDiscoveryObserver.aidl \ core/java/android/print/IPrintDocumentAdapter.aidl \ diff --git a/core/java/android/service/notification/INotificationListener.aidl b/core/java/android/service/notification/INotificationListener.aidl index d4b29d8e3a4d4..57ab9b9d48eb9 100644 --- a/core/java/android/service/notification/INotificationListener.aidl +++ b/core/java/android/service/notification/INotificationListener.aidl @@ -16,12 +16,13 @@ package android.service.notification; +import android.service.notification.IStatusBarNotificationHolder; import android.service.notification.StatusBarNotification; /** @hide */ oneway interface INotificationListener { void onListenerConnected(in String[] notificationKeys); - void onNotificationPosted(in StatusBarNotification notification); - void onNotificationRemoved(in StatusBarNotification notification); + void onNotificationPosted(in IStatusBarNotificationHolder notificationHolder); + void onNotificationRemoved(in IStatusBarNotificationHolder notificationHolder); } \ No newline at end of file diff --git a/core/java/android/service/notification/IStatusBarNotificationHolder.aidl b/core/java/android/service/notification/IStatusBarNotificationHolder.aidl new file mode 100644 index 0000000000000..fd6b59ed44495 --- /dev/null +++ b/core/java/android/service/notification/IStatusBarNotificationHolder.aidl @@ -0,0 +1,24 @@ +/** + * Copyright (c) 2014, 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 android.service.notification; + +import android.service.notification.StatusBarNotification; + +/** @hide */ +interface IStatusBarNotificationHolder { + StatusBarNotification get(); +} diff --git a/core/java/android/service/notification/NotificationListenerService.java b/core/java/android/service/notification/NotificationListenerService.java index eb2de2c5a52f2..98d36e0890e20 100644 --- a/core/java/android/service/notification/NotificationListenerService.java +++ b/core/java/android/service/notification/NotificationListenerService.java @@ -22,6 +22,7 @@ import android.app.Service; import android.content.Context; import android.content.Intent; import android.os.IBinder; +import android.os.RemoteException; import android.os.ServiceManager; import android.util.Log; @@ -213,7 +214,14 @@ public abstract class NotificationListenerService extends Service { private class INotificationListenerWrapper extends INotificationListener.Stub { @Override - public void onNotificationPosted(StatusBarNotification sbn) { + public void onNotificationPosted(IStatusBarNotificationHolder sbnHolder) { + StatusBarNotification sbn; + try { + sbn = sbnHolder.get(); + } catch (RemoteException e) { + Log.w(TAG, "onNotificationPosted: Error receiving StatusBarNotification", e); + return; + } try { NotificationListenerService.this.onNotificationPosted(sbn); } catch (Throwable t) { @@ -221,7 +229,14 @@ public abstract class NotificationListenerService extends Service { } } @Override - public void onNotificationRemoved(StatusBarNotification sbn) { + public void onNotificationRemoved(IStatusBarNotificationHolder sbnHolder) { + StatusBarNotification sbn; + try { + sbn = sbnHolder.get(); + } catch (RemoteException e) { + Log.w(TAG, "onNotificationRemoved: Error receiving StatusBarNotification", e); + return; + } try { NotificationListenerService.this.onNotificationRemoved(sbn); } catch (Throwable t) { diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index f8ff038f4c234..f53445af560b7 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -59,6 +59,7 @@ import android.os.UserHandle; import android.os.Vibrator; import android.provider.Settings; import android.service.notification.INotificationListener; +import android.service.notification.IStatusBarNotificationHolder; import android.service.notification.NotificationListenerService; import android.service.notification.StatusBarNotification; import android.telephony.TelephonyManager; @@ -241,21 +242,21 @@ public class NotificationManagerService extends SystemService { return (nid == UserHandle.USER_ALL || nid == this.userid); } - public void notifyPostedIfUserMatch(StatusBarNotification sbn) { - if (!enabledAndUserMatches(sbn)) { + public void notifyPostedIfUserMatch(StatusBarNotificationHolder sbnHolder) { + if (!enabledAndUserMatches(sbnHolder.get())) { return; } try { - listener.onNotificationPosted(sbn); + listener.onNotificationPosted(sbnHolder); } catch (RemoteException ex) { Log.e(TAG, "unable to notify listener (posted): " + listener, ex); } } - public void notifyRemovedIfUserMatch(StatusBarNotification sbn) { - if (!enabledAndUserMatches(sbn)) return; + public void notifyRemovedIfUserMatch(StatusBarNotificationHolder sbnHolder) { + if (!enabledAndUserMatches(sbnHolder.get())) return; try { - listener.onNotificationRemoved(sbn); + listener.onNotificationRemoved(sbnHolder); } catch (RemoteException ex) { Log.e(TAG, "unable to notify listener (removed): " + listener, ex); } @@ -711,12 +712,13 @@ public class NotificationManagerService extends SystemService { */ void notifyPostedLocked(NotificationRecord n) { // make a copy in case changes are made to the underlying Notification object - final StatusBarNotification sbn = n.sbn.clone(); + final StatusBarNotificationHolder sbnHolder = new StatusBarNotificationHolder( + n.sbn.clone()); for (final NotificationListenerInfo info : mListeners) { mHandler.post(new Runnable() { @Override public void run() { - info.notifyPostedIfUserMatch(sbn); + info.notifyPostedIfUserMatch(sbnHolder); }}); } } @@ -727,13 +729,14 @@ public class NotificationManagerService extends SystemService { void notifyRemovedLocked(NotificationRecord n) { // make a copy in case changes are made to the underlying Notification object // NOTE: this copy is lightweight: it doesn't include heavyweight parts of the notification - final StatusBarNotification sbn_light = n.sbn.cloneLight(); + final StatusBarNotificationHolder sbnLightHolder = new StatusBarNotificationHolder( + n.sbn.cloneLight()); for (final NotificationListenerInfo info : mListeners) { mHandler.post(new Runnable() { @Override public void run() { - info.notifyRemovedIfUserMatch(sbn_light); + info.notifyRemovedIfUserMatch(sbnLightHolder); }}); } } @@ -2531,4 +2534,22 @@ public class NotificationManagerService extends SystemService { updateLightsLocked(); } } + + /** + * Wrapper for a StatusBarNotification object that allows transfer across a oneway + * binder without sending large amounts of data over a oneway transaction. + */ + private static final class StatusBarNotificationHolder + extends IStatusBarNotificationHolder.Stub { + private final StatusBarNotification mValue; + + public StatusBarNotificationHolder(StatusBarNotification value) { + mValue = value; + } + + @Override + public StatusBarNotification get() { + return mValue; + } + } }