Notification listener and ranker callbacks on binder threads.
The callbacks for the notification listener and notification
ranker were delivered on binder threads which is problematic
becuase: 1) permission checks and app ops checks would fail
unless the app developer knows to clear binder calling id and
restore it after that; 2) developers need to synchronize their
implementation as they get callbacks on different threads (
arguably callbacks should not be concurrent); 3) this doesn't
follow the pattern in the platform;
Also the code delivering callbacks was catching Throwable which
we shouldn't do in general and also masks bugs in the listener
or ranker implementation. Now that the callbacks are offloaded
to the main listener/ranker thread system code should not be
guarding against Throwable to handle exceptions propagated
over binder calls.
bug:26704777
Change-Id: I171fb41bbe25e6105dd05e4166193dbcec594f82
diff --git a/core/java/android/service/notification/NotificationAssistantService.java b/core/java/android/service/notification/NotificationAssistantService.java
index b5387f1..41af837 100644
--- a/core/java/android/service/notification/NotificationAssistantService.java
+++ b/core/java/android/service/notification/NotificationAssistantService.java
@@ -18,18 +18,17 @@
import android.annotation.SdkConstant;
import android.annotation.SystemApi;
-import android.app.INotificationManager;
-import android.app.Notification;
import android.content.ComponentName;
import android.content.Context;
import android.content.Intent;
import android.net.Uri;
+import android.os.Handler;
import android.os.IBinder;
-import android.os.Parcel;
-import android.os.Parcelable;
+import android.os.Looper;
+import android.os.Message;
import android.os.RemoteException;
-import android.os.ServiceManager;
import android.util.Log;
+import com.android.internal.os.SomeArgs;
/**
* A service that helps the user manage notifications by modifying the
@@ -125,8 +124,24 @@
}
}
+ private Handler mHandler;
+
+ /** @hide */
@Override
- public IBinder onBind(Intent intent) {
+ public void registerAsSystemService(Context context, ComponentName componentName,
+ int currentUser) throws RemoteException {
+ super.registerAsSystemService(context, componentName, currentUser);
+ mHandler = new MyHandler(getContext().getMainLooper());
+ }
+
+ @Override
+ protected void attachBaseContext(Context base) {
+ super.attachBaseContext(base);
+ mHandler = new MyHandler(getContext().getMainLooper());
+ }
+
+ @Override
+ public final IBinder onBind(Intent intent) {
if (mWrapper == null) {
mWrapper = new NotificationAssistantWrapper();
}
@@ -198,8 +213,7 @@
* @param key the notification key
* @param adjustment the new importance with an explanation
*/
- public final void adjustImportance(String key, Adjustment adjustment)
- {
+ public final void adjustImportance(String key, Adjustment adjustment) {
if (!isBound()) return;
try {
getNotificationInterface().setImportanceFromAssistant(mWrapper, key,
@@ -212,7 +226,7 @@
private class NotificationAssistantWrapper extends NotificationListenerWrapper {
@Override
public void onNotificationEnqueued(IStatusBarNotificationHolder sbnHolder,
- int importance, boolean user) throws RemoteException {
+ int importance, boolean user) {
StatusBarNotification sbn;
try {
sbn = sbnHolder.get();
@@ -221,54 +235,114 @@
return;
}
- try {
- Adjustment adjustment =
- NotificationAssistantService.this.onNotificationEnqueued(sbn, importance, user);
- if (adjustment != null) {
- adjustImportance(sbn.getKey(), adjustment);
- }
- } catch (Throwable t) {
- Log.w(TAG, "Error running onNotificationEnqueued", t);
- }
+ SomeArgs args = SomeArgs.obtain();
+ args.arg1 = sbn;
+ args.argi1 = importance;
+ args.argi2 = user ? 1 : 0;
+ mHandler.obtainMessage(MyHandler.MSG_ON_NOTIFICATION_ENQUEUED,
+ args).sendToTarget();
}
@Override
- public void onNotificationVisibilityChanged(String key, long time, boolean visible)
- throws RemoteException {
- try {
- NotificationAssistantService.this.onNotificationVisibilityChanged(key, time,
- visible);
- } catch (Throwable t) {
- Log.w(TAG, "Error running onNotificationVisibilityChanged", t);
- }
+ public void onNotificationVisibilityChanged(String key, long time, boolean visible) {
+ SomeArgs args = SomeArgs.obtain();
+ args.arg1 = key;
+ args.arg2 = time;
+ args.argi1 = visible ? 1 : 0;
+ mHandler.obtainMessage(MyHandler.MSG_ON_NOTIFICATION_VISIBILITY_CHANGED,
+ args).sendToTarget();
}
@Override
- public void onNotificationClick(String key, long time) throws RemoteException {
- try {
- NotificationAssistantService.this.onNotificationClick(key, time);
- } catch (Throwable t) {
- Log.w(TAG, "Error running onNotificationClick", t);
- }
+ public void onNotificationClick(String key, long time) {
+ SomeArgs args = SomeArgs.obtain();
+ args.arg1 = key;
+ args.arg2 = time;
+ mHandler.obtainMessage(MyHandler.MSG_ON_NOTIFICATION_CLICK,
+ args).sendToTarget();
}
@Override
- public void onNotificationActionClick(String key, long time, int actionIndex)
- throws RemoteException {
- try {
- NotificationAssistantService.this.onNotificationActionClick(key, time, actionIndex);
- } catch (Throwable t) {
- Log.w(TAG, "Error running onNotificationActionClick", t);
- }
+ public void onNotificationActionClick(String key, long time, int actionIndex) {
+ SomeArgs args = SomeArgs.obtain();
+ args.arg1 = key;
+ args.arg2 = time;
+ args.argi1 = actionIndex;
+ mHandler.obtainMessage(MyHandler.MSG_ON_NOTIFICATION_ACTION_CLICK,
+ args).sendToTarget();
}
@Override
- public void onNotificationRemovedReason(String key, long time, int reason)
- throws RemoteException {
- try {
- NotificationAssistantService.this.onNotificationRemoved(key, time, reason);
- } catch (Throwable t) {
- Log.w(TAG, "Error running onNotificationRemoved", t);
+ public void onNotificationRemovedReason(String key, long time, int reason) {
+ SomeArgs args = SomeArgs.obtain();
+ args.arg1 = key;
+ args.arg2 = time;
+ args.argi1 = reason;
+ mHandler.obtainMessage(MyHandler.MSG_ON_NOTIFICATION_REMOVED_REASON,
+ args).sendToTarget();
+ }
+ }
+
+ private final class MyHandler extends Handler {
+ public static final int MSG_ON_NOTIFICATION_ENQUEUED = 1;
+ public static final int MSG_ON_NOTIFICATION_VISIBILITY_CHANGED = 2;
+ public static final int MSG_ON_NOTIFICATION_CLICK = 3;
+ public static final int MSG_ON_NOTIFICATION_ACTION_CLICK = 4;
+ public static final int MSG_ON_NOTIFICATION_REMOVED_REASON = 5;
+
+ public MyHandler(Looper looper) {
+ super(looper, null, false);
+ }
+
+ @Override
+ public void handleMessage(Message msg) {
+ switch (msg.what) {
+ case MSG_ON_NOTIFICATION_ENQUEUED: {
+ SomeArgs args = (SomeArgs) msg.obj;
+ StatusBarNotification sbn = (StatusBarNotification) args.arg1;
+ final int importance = args.argi1;
+ final boolean user = args.argi2 == 1;
+ args.recycle();
+ Adjustment adjustment = onNotificationEnqueued(sbn, importance, user);
+ if (adjustment != null) {
+ adjustImportance(sbn.getKey(), adjustment);
+ }
+ } break;
+
+ case MSG_ON_NOTIFICATION_VISIBILITY_CHANGED: {
+ SomeArgs args = (SomeArgs) msg.obj;
+ final String key = (String) args.arg1;
+ final long time = (long) args.arg2;
+ final boolean visible = args.argi1 == 1;
+ args.recycle();
+ onNotificationVisibilityChanged(key, time, visible);
+ } break;
+
+ case MSG_ON_NOTIFICATION_CLICK: {
+ SomeArgs args = (SomeArgs) msg.obj;
+ final String key = (String) args.arg1;
+ final long time = (long) args.arg2;
+ args.recycle();
+ onNotificationClick(key, time);
+ } break;
+
+ case MSG_ON_NOTIFICATION_ACTION_CLICK: {
+ SomeArgs args = (SomeArgs) msg.obj;
+ final String key = (String) args.arg1;
+ final long time = (long) args.arg2;
+ final int actionIndex = args.argi1;
+ args.recycle();
+ onNotificationActionClick(key, time, actionIndex);
+ } break;
+
+ case MSG_ON_NOTIFICATION_REMOVED_REASON: {
+ SomeArgs args = (SomeArgs) msg.obj;
+ final String key = (String) args.arg1;
+ final long time = (long) args.arg2;
+ final int reason = args.argi1;
+ args.recycle();
+ onNotificationRemoved(key, time, reason);
+ } break;
}
}
}
diff --git a/core/java/android/service/notification/NotificationListenerService.java b/core/java/android/service/notification/NotificationListenerService.java
index b4332e1..73a890f 100644
--- a/core/java/android/service/notification/NotificationListenerService.java
+++ b/core/java/android/service/notification/NotificationListenerService.java
@@ -16,6 +16,10 @@
package android.service.notification;
+import android.os.Handler;
+import android.os.Looper;
+import android.os.Message;
+
import android.annotation.IntDef;
import android.annotation.SystemApi;
import android.annotation.SdkConstant;
@@ -43,7 +47,8 @@
import android.util.ArraySet;
import android.util.Log;
import android.widget.RemoteViews;
-
+import com.android.internal.annotations.GuardedBy;
+import com.android.internal.os.SomeArgs;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.util.ArrayList;
@@ -163,8 +168,14 @@
@SystemApi
public static final int TRIM_LIGHT = 1;
+ private final Object mLock = new Object();
+
+ private Handler mHandler;
+
/** @hide */
protected NotificationListenerWrapper mWrapper = null;
+
+ @GuardedBy("mLock")
private RankingMap mRankingMap;
private INotificationManager mNoMan;
@@ -195,6 +206,12 @@
public static final String CATEGORY_VR_NOTIFICATIONS =
"android.intent.category.vr.notifications";
+ @Override
+ protected void attachBaseContext(Context base) {
+ super.attachBaseContext(base);
+ mHandler = new MyHandler(getMainLooper());
+ }
+
/**
* Implement this method to learn about new notifications as they are posted by apps.
*
@@ -642,7 +659,9 @@
* @return A {@link RankingMap} object providing access to ranking information
*/
public RankingMap getCurrentRanking() {
- return mRankingMap;
+ synchronized (mLock) {
+ return mRankingMap;
+ }
}
@Override
@@ -684,6 +703,7 @@
INotificationManager noMan = getNotificationInterface();
noMan.registerListener(mWrapper, componentName, currentUser);
mCurrentUser = currentUser;
+ mHandler = new MyHandler(context.getMainLooper());
}
/**
@@ -710,7 +730,7 @@
* <P>The service should wait for the {@link #onListenerConnected()} event
* before performing any operations.
*/
- public static final void requestRebind(ComponentName componentName)
+ public static void requestRebind(ComponentName componentName)
throws RemoteException {
INotificationManager noMan = INotificationManager.Stub.asInterface(
ServiceManager.getService(Context.NOTIFICATION_SERVICE));
@@ -782,7 +802,6 @@
}
try {
- Notification notification = sbn.getNotification();
// convert icon metadata to legacy format for older clients
createLegacyIconExtras(sbn.getNotification());
maybePopulateRemoteViews(sbn.getNotification());
@@ -794,20 +813,23 @@
}
// protect subclass from concurrent modifications of (@link mNotificationKeys}.
- synchronized (mWrapper) {
- applyUpdate(update);
- try {
- if (sbn != null) {
- NotificationListenerService.this.onNotificationPosted(sbn, mRankingMap);
- } else {
- // still pass along the ranking map, it may contain other information
- NotificationListenerService.this.onNotificationRankingUpdate(mRankingMap);
- }
- } catch (Throwable t) {
- Log.w(TAG, "Error running onNotificationPosted", t);
+ synchronized (mLock) {
+ applyUpdateLocked(update);
+ if (sbn != null) {
+ SomeArgs args = SomeArgs.obtain();
+ args.arg1 = sbn;
+ args.arg2 = mRankingMap;
+ mHandler.obtainMessage(MyHandler.MSG_ON_NOTIFICATION_POSTED,
+ args).sendToTarget();
+ } else {
+ // still pass along the ranking map, it may contain other information
+ mHandler.obtainMessage(MyHandler.MSG_ON_NOTIFICATION_RANKING_UPDATE,
+ mRankingMap).sendToTarget();
}
}
+
}
+
@Override
public void onNotificationRemoved(IStatusBarNotificationHolder sbnHolder,
NotificationRankingUpdate update) {
@@ -819,56 +841,48 @@
return;
}
// protect subclass from concurrent modifications of (@link mNotificationKeys}.
- synchronized (mWrapper) {
- applyUpdate(update);
- try {
- NotificationListenerService.this.onNotificationRemoved(sbn, mRankingMap);
- } catch (Throwable t) {
- Log.w(TAG, "Error running onNotificationRemoved", t);
- }
+ synchronized (mLock) {
+ applyUpdateLocked(update);
+ SomeArgs args = SomeArgs.obtain();
+ args.arg1 = sbn;
+ args.arg2 = mRankingMap;
+ mHandler.obtainMessage(MyHandler.MSG_ON_NOTIFICATION_REMOVED,
+ args).sendToTarget();
}
+
}
+
@Override
public void onListenerConnected(NotificationRankingUpdate update) {
// protect subclass from concurrent modifications of (@link mNotificationKeys}.
- synchronized (mWrapper) {
- applyUpdate(update);
- try {
- NotificationListenerService.this.onListenerConnected();
- } catch (Throwable t) {
- Log.w(TAG, "Error running onListenerConnected", t);
- }
+ synchronized (mLock) {
+ applyUpdateLocked(update);
}
+ mHandler.obtainMessage(MyHandler.MSG_ON_LISTENER_CONNECTED).sendToTarget();
}
+
@Override
public void onNotificationRankingUpdate(NotificationRankingUpdate update)
throws RemoteException {
// protect subclass from concurrent modifications of (@link mNotificationKeys}.
- synchronized (mWrapper) {
- applyUpdate(update);
- try {
- NotificationListenerService.this.onNotificationRankingUpdate(mRankingMap);
- } catch (Throwable t) {
- Log.w(TAG, "Error running onNotificationRankingUpdate", t);
- }
+ synchronized (mLock) {
+ applyUpdateLocked(update);
+ mHandler.obtainMessage(MyHandler.MSG_ON_NOTIFICATION_RANKING_UPDATE,
+ mRankingMap).sendToTarget();
}
+
}
+
@Override
public void onListenerHintsChanged(int hints) throws RemoteException {
- try {
- NotificationListenerService.this.onListenerHintsChanged(hints);
- } catch (Throwable t) {
- Log.w(TAG, "Error running onListenerHintsChanged", t);
- }
+ mHandler.obtainMessage(MyHandler.MSG_ON_LISTENER_HINTS_CHANGED,
+ hints, 0).sendToTarget();
}
@Override
public void onInterruptionFilterChanged(int interruptionFilter) throws RemoteException {
- try {
- NotificationListenerService.this.onInterruptionFilterChanged(interruptionFilter);
- } catch (Throwable t) {
- Log.w(TAG, "Error running onInterruptionFilterChanged", t);
- }
+ mHandler.obtainMessage(MyHandler.MSG_ON_INTERRUPTION_FILTER_CHANGED,
+ interruptionFilter, 0).sendToTarget();
}
@Override
@@ -901,11 +915,12 @@
}
}
- private void applyUpdate(NotificationRankingUpdate update) {
+ private void applyUpdateLocked(NotificationRankingUpdate update) {
mRankingMap = new RankingMap(update);
}
- private Context getContext() {
+ /** @hide */
+ protected Context getContext() {
if (mSystemContext != null) {
return mSystemContext;
}
@@ -1289,4 +1304,57 @@
}
};
}
+
+ private final class MyHandler extends Handler {
+ public static final int MSG_ON_NOTIFICATION_POSTED = 1;
+ public static final int MSG_ON_NOTIFICATION_REMOVED = 2;
+ public static final int MSG_ON_LISTENER_CONNECTED = 3;
+ public static final int MSG_ON_NOTIFICATION_RANKING_UPDATE = 4;
+ public static final int MSG_ON_LISTENER_HINTS_CHANGED = 5;
+ public static final int MSG_ON_INTERRUPTION_FILTER_CHANGED = 6;
+
+ public MyHandler(Looper looper) {
+ super(looper, null, false);
+ }
+
+ @Override
+ public void handleMessage(Message msg) {
+ switch (msg.what) {
+ case MSG_ON_NOTIFICATION_POSTED: {
+ SomeArgs args = (SomeArgs) msg.obj;
+ StatusBarNotification sbn = (StatusBarNotification) args.arg1;
+ RankingMap rankingMap = (RankingMap) args.arg2;
+ args.recycle();
+ onNotificationPosted(sbn, rankingMap);
+ } break;
+
+ case MSG_ON_NOTIFICATION_REMOVED: {
+ SomeArgs args = (SomeArgs) msg.obj;
+ StatusBarNotification sbn = (StatusBarNotification) args.arg1;
+ RankingMap rankingMap = (RankingMap) args.arg2;
+ args.recycle();
+ onNotificationRemoved(sbn, rankingMap);
+ } break;
+
+ case MSG_ON_LISTENER_CONNECTED: {
+ onListenerConnected();
+ } break;
+
+ case MSG_ON_NOTIFICATION_RANKING_UPDATE: {
+ RankingMap rankingMap = (RankingMap) msg.obj;
+ onNotificationRankingUpdate(rankingMap);
+ } break;
+
+ case MSG_ON_LISTENER_HINTS_CHANGED: {
+ final int hints = msg.arg1;
+ onListenerHintsChanged(hints);
+ } break;
+
+ case MSG_ON_INTERRUPTION_FILTER_CHANGED: {
+ final int interruptionFilter = msg.arg1;
+ onInterruptionFilterChanged(interruptionFilter);
+ } break;
+ }
+ }
+ }
}