From 99742734e3b67cc48941e37db982c43aa843596c Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Wed, 28 Feb 2018 18:06:10 -0800 Subject: [PATCH] Fix toast lifetime In NotificationManagerService#cancelToast we have been calling WindowManagerService#removeWindowToken with 'removeWindows'=true. This is allowing for Surface destruction without any sort of synchronization from the client. Before the call to removeWindowToken we are emitting a one-way hide call to the Toast client. As a solution to the lifetime issue we have the client callback to let us know it has processed the hide call (and thus stopped the ViewRoot). On the server side we also instate a timeout. This mirrors the app stop timeout. All codepaths I could find leading to this sort of situation where a client is still rendering in to a toast following the total duration expiring seem to indicate a hung client UI thread. Bug: 62536731 Bug: 70530552 Test: Manual. go/wm-smoke Change-Id: I89643b3c3a9fa42423b498c1bd3a422a7959aaaf --- .../android/app/INotificationManager.aidl | 2 + core/java/android/widget/Toast.java | 8 ++ .../NotificationManagerService.java | 73 +++++++++++++++++-- 3 files changed, 75 insertions(+), 8 deletions(-) diff --git a/core/java/android/app/INotificationManager.aidl b/core/java/android/app/INotificationManager.aidl index d378f227d921d..ddd065692ea2a 100644 --- a/core/java/android/app/INotificationManager.aidl +++ b/core/java/android/app/INotificationManager.aidl @@ -45,6 +45,8 @@ interface INotificationManager void clearData(String pkg, int uid, boolean fromApp); void enqueueToast(String pkg, ITransientNotification callback, int duration); void cancelToast(String pkg, ITransientNotification callback); + void finishToken(String pkg, ITransientNotification callback); + void enqueueNotificationWithTag(String pkg, String opPkg, String tag, int id, in Notification notification, int userId); void cancelNotificationWithTag(String pkg, String tag, int id, int userId); diff --git a/core/java/android/widget/Toast.java b/core/java/android/widget/Toast.java index edcf209bdf818..d74a60e483e57 100644 --- a/core/java/android/widget/Toast.java +++ b/core/java/android/widget/Toast.java @@ -531,6 +531,14 @@ public class Toast { mWM.removeViewImmediate(mView); } + + // Now that we've removed the view it's safe for the server to release + // the resources. + try { + getService().finishToken(mPackageName, this); + } catch (RemoteException e) { + } + mView = null; } } diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index ef354cdee0739..debfbc0ff4b87 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -229,11 +229,12 @@ public class NotificationManagerService extends SystemService { static final float DEFAULT_MAX_NOTIFICATION_ENQUEUE_RATE = 5f; // message codes - static final int MESSAGE_TIMEOUT = 2; + static final int MESSAGE_DURATION_REACHED = 2; static final int MESSAGE_SAVE_POLICY_FILE = 3; static final int MESSAGE_SEND_RANKING_UPDATE = 4; static final int MESSAGE_LISTENER_HINTS_CHANGED = 5; static final int MESSAGE_LISTENER_NOTIFICATION_FILTER_CHANGED = 6; + static final int MESSAGE_FINISH_TOKEN_TIMEOUT = 7; // ranking thread messages private static final int MESSAGE_RECONSIDER_RANKING = 1000; @@ -1889,6 +1890,25 @@ public class NotificationManagerService extends SystemService { } } + @Override + public void finishToken(String pkg, ITransientNotification callback) { + synchronized (mToastQueue) { + long callingId = Binder.clearCallingIdentity(); + try { + int index = indexOfToastLocked(pkg, callback); + if (index >= 0) { + ToastRecord record = mToastQueue.get(index); + finishTokenLocked(record.token); + } else { + Slog.w(TAG, "Toast already killed. pkg=" + pkg + + " callback=" + callback); + } + } finally { + Binder.restoreCallingIdentity(callingId); + } + } + } + @Override public void enqueueNotificationWithTag(String pkg, String opPkg, String tag, int id, Notification notification, int userId) throws RemoteException { @@ -4578,7 +4598,7 @@ public class NotificationManagerService extends SystemService { if (DBG) Slog.d(TAG, "Show pkg=" + record.pkg + " callback=" + record.callback); try { record.callback.show(record.token); - scheduleTimeoutLocked(record); + scheduleDurationReachedLocked(record); return; } catch (RemoteException e) { Slog.w(TAG, "Object died trying to show notification " + record.callback @@ -4611,7 +4631,15 @@ public class NotificationManagerService extends SystemService { } ToastRecord lastToast = mToastQueue.remove(index); - mWindowManagerInternal.removeWindowToken(lastToast.token, true, DEFAULT_DISPLAY); + + mWindowManagerInternal.removeWindowToken(lastToast.token, false /* removeWindows */, + DEFAULT_DISPLAY); + // We passed 'false' for 'removeWindows' so that the client has time to stop + // rendering (as hide above is a one-way message), otherwise we could crash + // a client which was actively using a surface made from the token. However + // we need to schedule a timeout to make sure the token is eventually killed + // one way or another. + scheduleKillTokenTimeout(lastToast.token); keepProcessAliveIfNeededLocked(record.pid); if (mToastQueue.size() > 0) { @@ -4622,16 +4650,26 @@ public class NotificationManagerService extends SystemService { } } + void finishTokenLocked(IBinder t) { + mHandler.removeCallbacksAndMessages(t); + // We pass 'true' for 'removeWindows' to let the WindowManager destroy any + // remaining surfaces as either the client has called finishToken indicating + // it has successfully removed the views, or the client has timed out + // at which point anything goes. + mWindowManagerInternal.removeWindowToken(t, true /* removeWindows */, + DEFAULT_DISPLAY); + } + @GuardedBy("mToastQueue") - private void scheduleTimeoutLocked(ToastRecord r) + private void scheduleDurationReachedLocked(ToastRecord r) { mHandler.removeCallbacksAndMessages(r); - Message m = Message.obtain(mHandler, MESSAGE_TIMEOUT, r); + Message m = Message.obtain(mHandler, MESSAGE_DURATION_REACHED, r); long delay = r.duration == Toast.LENGTH_LONG ? LONG_DELAY : SHORT_DELAY; mHandler.sendMessageDelayed(m, delay); } - private void handleTimeout(ToastRecord record) + private void handleDurationReached(ToastRecord record) { if (DBG) Slog.d(TAG, "Timeout pkg=" + record.pkg + " callback=" + record.callback); synchronized (mToastQueue) { @@ -4642,6 +4680,22 @@ public class NotificationManagerService extends SystemService { } } + @GuardedBy("mToastQueue") + private void scheduleKillTokenTimeout(IBinder token) + { + mHandler.removeCallbacksAndMessages(token); + Message m = Message.obtain(mHandler, MESSAGE_FINISH_TOKEN_TIMEOUT, token); + mHandler.sendMessageDelayed(m, 5); + } + + private void handleKillTokenTimeout(IBinder token) + { + if (DBG) Slog.d(TAG, "Kill Token Timeout token=" + token); + synchronized (mToastQueue) { + finishTokenLocked(token); + } + } + @GuardedBy("mToastQueue") int indexOfToastLocked(String pkg, ITransientNotification callback) { @@ -4839,8 +4893,11 @@ public class NotificationManagerService extends SystemService { { switch (msg.what) { - case MESSAGE_TIMEOUT: - handleTimeout((ToastRecord)msg.obj); + case MESSAGE_DURATION_REACHED: + handleDurationReached((ToastRecord)msg.obj); + break; + case MESSAGE_FINISH_TOKEN_TIMEOUT: + handleKillTokenTimeout((IBinder)msg.obj); break; case MESSAGE_SAVE_POLICY_FILE: handleSavePolicyFile();