From a4c79758fb53a6d25368c9fe706c666a7d6f5db3 Mon Sep 17 00:00:00 2001 From: Miranda Kephart Date: Fri, 24 Apr 2020 15:23:58 -0400 Subject: [PATCH] Keep screenshot process bound Currently, the screenshot process only remains bound to SysUI (by ScreenshotHelper) until the screenshot finishes saving. This makes it vulnerable to getting cached and frozen. Since the process now includes UI with touch focus for longer than that, this can cause ANRs in the screenshot process, if it is frozen while it's supposed to be accepting touch input. Retaining the binding for the entire lifecycle of the screenshot UI fixes this problem. Test: manual -- used the steps in b/153577093 to verify the freezing behavior and verified that it no longer occurred after the change Bug: 153418099 Fix: 153418099 Change-Id: Idca24a69274af3d633e1243b46b602613cb04d50 --- .../internal/util/ScreenshotHelper.java | 165 ++++++++++-------- .../systemui/screenshot/GlobalScreenshot.java | 38 ++-- .../screenshot/SaveImageInBackgroundTask.java | 13 ++ .../screenshot/TakeScreenshotService.java | 29 ++- 4 files changed, 153 insertions(+), 92 deletions(-) diff --git a/core/java/com/android/internal/util/ScreenshotHelper.java b/core/java/com/android/internal/util/ScreenshotHelper.java index adadc5e205492..49c9302eeb110 100644 --- a/core/java/com/android/internal/util/ScreenshotHelper.java +++ b/core/java/com/android/internal/util/ScreenshotHelper.java @@ -27,6 +27,9 @@ import java.util.function.Consumer; public class ScreenshotHelper { + public static final int SCREENSHOT_MSG_URI = 1; + public static final int SCREENSHOT_MSG_PROCESS_COMPLETE = 2; + /** * Describes a screenshot request (to make it easier to pass data through to the handler). */ @@ -135,6 +138,7 @@ public class ScreenshotHelper { private final int SCREENSHOT_TIMEOUT_MS = 10000; private final Object mScreenshotLock = new Object(); + private IBinder mScreenshotService = null; private ServiceConnection mScreenshotConnection = null; private final Context mContext; @@ -251,85 +255,104 @@ public class ScreenshotHelper { private void takeScreenshot(final int screenshotType, long timeoutMs, @NonNull Handler handler, ScreenshotRequest screenshotRequest, @Nullable Consumer completionConsumer) { synchronized (mScreenshotLock) { - if (mScreenshotConnection != null) { - return; - } - final ComponentName serviceComponent = ComponentName.unflattenFromString( - mContext.getResources().getString( - com.android.internal.R.string.config_screenshotServiceComponent)); - final Intent serviceIntent = new Intent(); - final Runnable mScreenshotTimeout = new Runnable() { + final Runnable mScreenshotTimeout = () -> { + synchronized (mScreenshotLock) { + if (mScreenshotConnection != null) { + mContext.unbindService(mScreenshotConnection); + mScreenshotConnection = null; + mScreenshotService = null; + notifyScreenshotError(); + } + } + if (completionConsumer != null) { + completionConsumer.accept(null); + } + }; + + Message msg = Message.obtain(null, screenshotType, screenshotRequest); + final ServiceConnection myConn = mScreenshotConnection; + Handler h = new Handler(handler.getLooper()) { @Override - public void run() { - synchronized (mScreenshotLock) { - if (mScreenshotConnection != null) { - mContext.unbindService(mScreenshotConnection); - mScreenshotConnection = null; - notifyScreenshotError(); + public void handleMessage(Message msg) { + switch (msg.what) { + case SCREENSHOT_MSG_URI: + if (completionConsumer != null) { + completionConsumer.accept((Uri) msg.obj); + } + handler.removeCallbacks(mScreenshotTimeout); + break; + case SCREENSHOT_MSG_PROCESS_COMPLETE: + synchronized (mScreenshotLock) { + if (mScreenshotConnection == myConn) { + mContext.unbindService(mScreenshotConnection); + mScreenshotConnection = null; + mScreenshotService = null; + } + } + break; + } + } + }; + msg.replyTo = new Messenger(h); + + if (mScreenshotConnection == null) { + final ComponentName serviceComponent = ComponentName.unflattenFromString( + mContext.getResources().getString( + com.android.internal.R.string.config_screenshotServiceComponent)); + final Intent serviceIntent = new Intent(); + + serviceIntent.setComponent(serviceComponent); + ServiceConnection conn = new ServiceConnection() { + @Override + public void onServiceConnected(ComponentName name, IBinder service) { + synchronized (mScreenshotLock) { + if (mScreenshotConnection != this) { + return; + } + mScreenshotService = service; + Messenger messenger = new Messenger(mScreenshotService); + + try { + messenger.send(msg); + } catch (RemoteException e) { + Log.e(TAG, "Couldn't take screenshot: " + e); + if (completionConsumer != null) { + completionConsumer.accept(null); + } + } } } + + @Override + public void onServiceDisconnected(ComponentName name) { + synchronized (mScreenshotLock) { + if (mScreenshotConnection != null) { + mContext.unbindService(mScreenshotConnection); + mScreenshotConnection = null; + mScreenshotService = null; + handler.removeCallbacks(mScreenshotTimeout); + notifyScreenshotError(); + } + } + } + }; + if (mContext.bindServiceAsUser(serviceIntent, conn, + Context.BIND_AUTO_CREATE | Context.BIND_FOREGROUND_SERVICE, + UserHandle.CURRENT)) { + mScreenshotConnection = conn; + handler.postDelayed(mScreenshotTimeout, timeoutMs); + } + } else { + Messenger messenger = new Messenger(mScreenshotService); + try { + messenger.send(msg); + } catch (RemoteException e) { + Log.e(TAG, "Couldn't take screenshot: " + e); if (completionConsumer != null) { completionConsumer.accept(null); } } - }; - - serviceIntent.setComponent(serviceComponent); - ServiceConnection conn = new ServiceConnection() { - @Override - public void onServiceConnected(ComponentName name, IBinder service) { - synchronized (mScreenshotLock) { - if (mScreenshotConnection != this) { - return; - } - Messenger messenger = new Messenger(service); - Message msg = Message.obtain(null, screenshotType, screenshotRequest); - final ServiceConnection myConn = this; - Handler h = new Handler(handler.getLooper()) { - @Override - public void handleMessage(Message msg) { - synchronized (mScreenshotLock) { - if (mScreenshotConnection == myConn) { - mContext.unbindService(mScreenshotConnection); - mScreenshotConnection = null; - handler.removeCallbacks(mScreenshotTimeout); - } - } - if (completionConsumer != null) { - completionConsumer.accept((Uri) msg.obj); - } - } - }; - msg.replyTo = new Messenger(h); - - try { - messenger.send(msg); - } catch (RemoteException e) { - Log.e(TAG, "Couldn't take screenshot: " + e); - if (completionConsumer != null) { - completionConsumer.accept(null); - } - } - } - } - - @Override - public void onServiceDisconnected(ComponentName name) { - synchronized (mScreenshotLock) { - if (mScreenshotConnection != null) { - mContext.unbindService(mScreenshotConnection); - mScreenshotConnection = null; - handler.removeCallbacks(mScreenshotTimeout); - notifyScreenshotError(); - } - } - } - }; - if (mContext.bindServiceAsUser(serviceIntent, conn, - Context.BIND_AUTO_CREATE | Context.BIND_FOREGROUND_SERVICE_WHILE_AWAKE, - UserHandle.CURRENT)) { - mScreenshotConnection = conn; handler.postDelayed(mScreenshotTimeout, timeoutMs); } } diff --git a/packages/SystemUI/src/com/android/systemui/screenshot/GlobalScreenshot.java b/packages/SystemUI/src/com/android/systemui/screenshot/GlobalScreenshot.java index 70454d4d63df1..290816b7e8af8 100644 --- a/packages/SystemUI/src/com/android/systemui/screenshot/GlobalScreenshot.java +++ b/packages/SystemUI/src/com/android/systemui/screenshot/GlobalScreenshot.java @@ -48,7 +48,6 @@ import android.graphics.Region; import android.graphics.drawable.Icon; import android.media.MediaActionSound; import android.net.Uri; -import android.os.AsyncTask; import android.os.Handler; import android.os.Looper; import android.os.Message; @@ -188,7 +187,9 @@ public class GlobalScreenshot implements ViewTreeObserver.OnComputeInternalInset private final ImageView mDismissImage; private Bitmap mScreenBitmap; + private SaveImageInBackgroundTask mSaveInBgTask; private Animator mScreenshotAnimation; + private Runnable mOnCompleteRunnable; private boolean mInDarkMode = false; private float mScreenshotOffsetXPx; @@ -197,8 +198,6 @@ public class GlobalScreenshot implements ViewTreeObserver.OnComputeInternalInset private float mDismissButtonSize; private float mCornerSizeX; - private AsyncTask mSaveInBgTask; - private MediaActionSound mCameraSound; // standard material ease @@ -211,6 +210,7 @@ public class GlobalScreenshot implements ViewTreeObserver.OnComputeInternalInset case MESSAGE_CORNER_TIMEOUT: mUiEventLogger.log(ScreenshotEvent.SCREENSHOT_INTERACTION_TIMEOUT); GlobalScreenshot.this.clearScreenshot("timeout"); + mOnCompleteRunnable.run(); break; default: break; @@ -252,6 +252,7 @@ public class GlobalScreenshot implements ViewTreeObserver.OnComputeInternalInset mDismissButton.setOnClickListener(view -> { mUiEventLogger.log(ScreenshotEvent.SCREENSHOT_EXPLICIT_DISMISSAL); clearScreenshot("dismiss_button"); + mOnCompleteRunnable.run(); }); mDismissImage = mDismissButton.findViewById(R.id.global_screenshot_dismiss_image); @@ -325,19 +326,19 @@ public class GlobalScreenshot implements ViewTreeObserver.OnComputeInternalInset data.finisher = finisher; data.mActionsReadyListener = actionsReadyListener; data.createDeleteAction = false; + if (mSaveInBgTask != null) { - mSaveInBgTask.cancel(false); + mSaveInBgTask.ignoreResult(); } - mSaveInBgTask = new SaveImageInBackgroundTask(mContext, data).execute(); + mSaveInBgTask = new SaveImageInBackgroundTask(mContext, data); + mSaveInBgTask.execute(); } /** * Takes a screenshot of the current display and shows an animation. */ private void takeScreenshot(Consumer finisher, Rect crop) { - clearScreenshot("new screenshot requested"); - int rot = mDisplay.getRotation(); int width = crop.width(); int height = crop.height(); @@ -349,10 +350,12 @@ public class GlobalScreenshot implements ViewTreeObserver.OnComputeInternalInset private void takeScreenshot(Bitmap screenshot, Consumer finisher, Rect screenRect) { mScreenBitmap = screenshot; + if (mScreenBitmap == null) { mNotificationsController.notifyScreenshotError( R.string.screenshot_failed_to_capture_text); finisher.accept(null); + mOnCompleteRunnable.run(); return; } @@ -366,11 +369,13 @@ public class GlobalScreenshot implements ViewTreeObserver.OnComputeInternalInset mScreenshotLayout.getViewTreeObserver().addOnComputeInternalInsetsListener(this); // Start the post-screenshot animation - startAnimation(finisher, screenRect.width(), screenRect.height(), - screenRect); + startAnimation(finisher, screenRect.width(), screenRect.height(), screenRect); } - void takeScreenshot(Consumer finisher) { + void takeScreenshot(Consumer finisher, Runnable onComplete) { + clearScreenshot("new screenshot requested"); + mOnCompleteRunnable = onComplete; + mDisplay.getRealMetrics(mDisplayMetrics); takeScreenshot( finisher, @@ -378,9 +383,11 @@ public class GlobalScreenshot implements ViewTreeObserver.OnComputeInternalInset } void handleImageAsScreenshot(Bitmap screenshot, Rect screenshotScreenBounds, - Insets visibleInsets, int taskId, Consumer finisher) { + Insets visibleInsets, int taskId, Consumer finisher, Runnable onComplete) { // TODO use taskId and visibleInsets clearScreenshot("new screenshot requested"); + mOnCompleteRunnable = onComplete; + takeScreenshot(screenshot, finisher, screenshotScreenBounds); } @@ -388,7 +395,10 @@ public class GlobalScreenshot implements ViewTreeObserver.OnComputeInternalInset * Displays a screenshot selector */ @SuppressLint("ClickableViewAccessibility") - void takeScreenshotPartial(final Consumer finisher) { + void takeScreenshotPartial(final Consumer finisher, Runnable onComplete) { + clearScreenshot("new screenshot requested"); + mOnCompleteRunnable = onComplete; + mWindowManager.addView(mScreenshotLayout, mWindowLayoutParams); mScreenshotSelectorView.setOnTouchListener(new View.OnTouchListener() { @Override @@ -660,6 +670,7 @@ public class GlobalScreenshot implements ViewTreeObserver.OnComputeInternalInset () -> { mUiEventLogger.log(ScreenshotEvent.SCREENSHOT_SMART_ACTION_TAPPED); clearScreenshot("chip tapped"); + mOnCompleteRunnable.run(); }); mActionsView.addView(actionChip); } @@ -671,6 +682,7 @@ public class GlobalScreenshot implements ViewTreeObserver.OnComputeInternalInset shareChip.setPendingIntent(imageData.shareAction.actionIntent, () -> { mUiEventLogger.log(ScreenshotEvent.SCREENSHOT_SHARE_TAPPED); clearScreenshot("chip tapped"); + mOnCompleteRunnable.run(); }); mActionsView.addView(shareChip); @@ -681,6 +693,7 @@ public class GlobalScreenshot implements ViewTreeObserver.OnComputeInternalInset editChip.setPendingIntent(imageData.editAction.actionIntent, () -> { mUiEventLogger.log(ScreenshotEvent.SCREENSHOT_EDIT_TAPPED); clearScreenshot("chip tapped"); + mOnCompleteRunnable.run(); }); mActionsView.addView(editChip); @@ -689,6 +702,7 @@ public class GlobalScreenshot implements ViewTreeObserver.OnComputeInternalInset imageData.editAction.actionIntent.send(); mUiEventLogger.log(ScreenshotEvent.SCREENSHOT_PREVIEW_TAPPED); clearScreenshot("screenshot preview tapped"); + mOnCompleteRunnable.run(); } catch (PendingIntent.CanceledException e) { Log.e(TAG, "Intent cancelled", e); } diff --git a/packages/SystemUI/src/com/android/systemui/screenshot/SaveImageInBackgroundTask.java b/packages/SystemUI/src/com/android/systemui/screenshot/SaveImageInBackgroundTask.java index 170174deaeb3c..23139382c6e07 100644 --- a/packages/SystemUI/src/com/android/systemui/screenshot/SaveImageInBackgroundTask.java +++ b/packages/SystemUI/src/com/android/systemui/screenshot/SaveImageInBackgroundTask.java @@ -230,6 +230,19 @@ class SaveImageInBackgroundTask extends AsyncTask { return null; } + /** + * If we get a new screenshot request while this one is saving, we want to continue saving in + * the background but not return anything. + */ + void ignoreResult() { + mParams.mActionsReadyListener = new GlobalScreenshot.ActionsReadyListener() { + @Override + void onActionsReady(GlobalScreenshot.SavedImageData imageData) { + // do nothing + } + }; + } + @Override protected void onCancelled(Void params) { // If we are cancelled while the task is running in the background, we may get null diff --git a/packages/SystemUI/src/com/android/systemui/screenshot/TakeScreenshotService.java b/packages/SystemUI/src/com/android/systemui/screenshot/TakeScreenshotService.java index f68cb745e4c8f..98030d45b05ee 100644 --- a/packages/SystemUI/src/com/android/systemui/screenshot/TakeScreenshotService.java +++ b/packages/SystemUI/src/com/android/systemui/screenshot/TakeScreenshotService.java @@ -16,6 +16,9 @@ package com.android.systemui.screenshot; +import static com.android.internal.util.ScreenshotHelper.SCREENSHOT_MSG_PROCESS_COMPLETE; +import static com.android.internal.util.ScreenshotHelper.SCREENSHOT_MSG_URI; + import android.app.Service; import android.content.Intent; import android.graphics.Bitmap; @@ -51,8 +54,15 @@ public class TakeScreenshotService extends Service { @Override public void handleMessage(Message msg) { final Messenger callback = msg.replyTo; - Consumer finisher = uri -> { - Message reply = Message.obtain(null, 1, uri); + Consumer uriConsumer = uri -> { + Message reply = Message.obtain(null, SCREENSHOT_MSG_URI, uri); + try { + callback.send(reply); + } catch (RemoteException e) { + } + }; + Runnable onComplete = () -> { + Message reply = Message.obtain(null, SCREENSHOT_MSG_PROCESS_COMPLETE); try { callback.send(reply); } catch (RemoteException e) { @@ -64,7 +74,8 @@ public class TakeScreenshotService extends Service { // animation and error notification. if (!mUserManager.isUserUnlocked()) { Log.w(TAG, "Skipping screenshot because storage is locked!"); - post(() -> finisher.accept(null)); + post(() -> uriConsumer.accept(null)); + post(onComplete); return; } @@ -79,19 +90,19 @@ public class TakeScreenshotService extends Service { switch (msg.what) { case WindowManager.TAKE_SCREENSHOT_FULLSCREEN: if (useCornerFlow) { - mScreenshot.takeScreenshot(finisher); + mScreenshot.takeScreenshot(uriConsumer, onComplete); } else { mScreenshotLegacy.takeScreenshot( - finisher, screenshotRequest.getHasStatusBar(), + uriConsumer, screenshotRequest.getHasStatusBar(), screenshotRequest.getHasNavBar()); } break; case WindowManager.TAKE_SCREENSHOT_SELECTED_REGION: if (useCornerFlow) { - mScreenshot.takeScreenshotPartial(finisher); + mScreenshot.takeScreenshotPartial(uriConsumer, onComplete); } else { mScreenshotLegacy.takeScreenshotPartial( - finisher, screenshotRequest.getHasStatusBar(), + uriConsumer, screenshotRequest.getHasStatusBar(), screenshotRequest.getHasNavBar()); } break; @@ -102,10 +113,10 @@ public class TakeScreenshotService extends Service { int taskId = screenshotRequest.getTaskId(); if (useCornerFlow) { mScreenshot.handleImageAsScreenshot( - screenshot, screenBounds, insets, taskId, finisher); + screenshot, screenBounds, insets, taskId, uriConsumer, onComplete); } else { mScreenshotLegacy.handleImageAsScreenshot( - screenshot, screenBounds, insets, taskId, finisher); + screenshot, screenBounds, insets, taskId, uriConsumer); } break; default: