Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 3d60f471 authored by Mark Renouf's avatar Mark Renouf
Browse files

Ensure cleanup when interrupting scroll capture 

A batch of related cleanups and fixes:

 * Connects cancellation signals between the layers of scroll capture.

 * Handles cancellation on dismissal, ensuring clean shutdown of session.

 * Ensures any pending operation is cancelled on the client side and the
   connection is closed.

 * App side of connection handles a crash of SystemUI, ensure close()

 * ScrollCaptureController:
   Moves onCaptureResult to a background thread; it calls into binder.

 * Fixes accuracy and category of some log messages about dismissal
   gesture.

Test: atest ScrollCaptureConnectionTest ScrollCaptureControllerTest
Change-Id: I20014a4cb43e28fcb97f6980fb47af8c06deaca2
parent 54fef7af

core/java/android/view/ScrollCaptureConnection.java

+38 −18
Original line number Diff line number Diff line
@@ -24,9 +24,9 @@ import android.annotation.UiThread; 
import android.graphics.Point;

import android.graphics.Rect;

import android.os.CancellationSignal;

import android.os.IBinder;

import android.os.ICancellationSignal;

import android.os.RemoteException;

import android.os.Trace;

import android.util.CloseGuard;

import android.util.Log;
@@ -44,7 +44,8 @@ import java.util.function.Consumer; 
 *

 * @hide

 */

public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub {

public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub implements

        IBinder.DeathRecipient {



    private static final String TAG = "ScrollCaptureConnection";
@@ -54,15 +55,13 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub { 
    private final Executor mUiThread;

    private final CloseGuard mCloseGuard = new CloseGuard();





    private ScrollCaptureCallback mLocal;

    private IScrollCaptureCallbacks mRemote;



    private ScrollCaptureSession mSession;



    private CancellationSignal mCancellation;



    private volatile boolean mActive;

    private volatile boolean mConnected;



    /**

     * Constructs a ScrollCaptureConnection.
@@ -87,13 +86,14 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub { 
    @Override

    public ICancellationSignal startCapture(@NonNull Surface surface,

            @NonNull IScrollCaptureCallbacks remote) throws RemoteException {



        mCloseGuard.open("close");



        if (!surface.isValid()) {

            throw new RemoteException(new IllegalArgumentException("surface must be valid"));

        }

        mRemote = requireNonNull(remote, "<callbacks> must non-null");

        mRemote.asBinder().linkToDeath(this, 0);

        mConnected = true;



        ICancellationSignal cancellation = CancellationSignal.createTransport();

        mCancellation = CancellationSignal.fromTransport(cancellation);
@@ -115,14 +115,14 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub { 
            Log.w(TAG, "Shutting down due to error: ", e);

            close();

        }

        mCancellation = null;

    }



    @BinderThread

    @Override

    public ICancellationSignal requestImage(Rect requestRect) throws RemoteException {

        Trace.beginSection("requestImage");

        checkActive();



        cancelPendingAction();

        ICancellationSignal cancellation = CancellationSignal.createTransport();

        mCancellation = CancellationSignal.fromTransport(cancellation);
@@ -131,7 +131,6 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub { 
        // -> UiThread

        mUiThread.execute(() -> mLocal.onScrollCaptureImageRequest(

                mSession, mCancellation, new Rect(requestRect), listener));

        Trace.endSection();

        return cancellation;

    }
@@ -142,6 +141,8 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub { 
        } catch (RemoteException e) {

            Log.w(TAG, "Shutting down due to error: ", e);

            close();

        } finally {

            mCancellation = null;

        }

    }
@@ -149,7 +150,7 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub { 
    @Override

    public ICancellationSignal endCapture() throws RemoteException {

        checkActive();



        cancelPendingAction();

        ICancellationSignal cancellation = CancellationSignal.createTransport();

        mCancellation = CancellationSignal.fromTransport(cancellation);
@@ -170,26 +171,32 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub { 
        } catch (RemoteException e) {

            Log.w(TAG, "Caught exception confirming capture end!", e);

        } finally {

            mCancellation = null;

            close();

        }

    }



    @Override

    public void binderDied() {

        Log.e(TAG, "Controlling process just died.");

        close();

    }



    @BinderThread

    @Override

    public void close() {

        if (mActive) {

            if (mCancellation != null) {

                Log.w(TAG, "close(): cancelling pending operation.");

                mCancellation.cancel();

                mCancellation = null;

            }

            Log.w(TAG, "close(): capture session still active! Ending now.");

            // -> UiThread

            cancelPendingAction();

            final ScrollCaptureCallback callback = mLocal;

            mUiThread.execute(() -> callback.onScrollCaptureEnd(() -> { /* ignore */ }));

            mActive = false;

        }

        if (mRemote != null) {

            mRemote.asBinder().unlinkToDeath(this, 0);

        }

        mActive = false;

        mConnected = false;

        mSession = null;

        mRemote = null;

        mLocal = null;
@@ -197,6 +204,19 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub { 
        Reference.reachabilityFence(this);

    }



    private void cancelPendingAction() {

        if (mCancellation != null) {

            Log.w(TAG, "cancelling pending operation.");

            mCancellation.cancel();

            mCancellation = null;

        }

    }



    @VisibleForTesting

    public boolean isConnected() {

        return mConnected;

    }



    @VisibleForTesting

    public boolean isActive() {

        return mActive;
@@ -236,7 +256,7 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub { 


        protected SafeCallback(CancellationSignal signal, Executor executor, T value) {

            mSignal = signal;

            mValue = new AtomicReference<T>(value);

            mValue = new AtomicReference<>(value);

            mExecutor = executor;

        }
@@ -257,7 +277,7 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub { 


        static <T> Consumer<T> create(CancellationSignal signal, Executor executor,

                Consumer<T> target) {

            return new ConsumerCallback<T>(signal, executor, target);

            return new ConsumerCallback<>(signal, executor, target);

        }

    }

core/tests/coretests/src/android/view/ScrollCaptureConnectionTest.java

+41 −13
Original line number Diff line number Diff line
@@ -21,16 +21,19 @@ import static androidx.test.InstrumentationRegistry.getTargetContext; 
import static org.junit.Assert.assertFalse;

import static org.junit.Assert.assertTrue;

import static org.junit.Assert.fail;

import static org.mockito.ArgumentMatchers.any;

import static org.mockito.ArgumentMatchers.anyInt;

import static org.mockito.ArgumentMatchers.eq;

import static org.mockito.Mockito.reset;

import static org.mockito.Mockito.never;

import static org.mockito.Mockito.times;

import static org.mockito.Mockito.verify;

import static org.mockito.Mockito.verifyNoMoreInteractions;

import static org.mockito.Mockito.when;



import android.graphics.Point;

import android.graphics.Rect;

import android.os.Binder;

import android.os.Handler;

import android.os.IBinder;

import android.os.ICancellationSignal;

import android.os.RemoteException;

import android.platform.test.annotations.Presubmit;
@@ -60,6 +63,7 @@ public class ScrollCaptureConnectionTest { 


    private ScrollCaptureTarget mTarget;

    private ScrollCaptureConnection mConnection;

    private IBinder mConnectionBinder = new Binder("ScrollCaptureConnection Test");



    private Handler mHandler;
@@ -76,6 +80,7 @@ public class ScrollCaptureConnectionTest { 
        mHandler = new Handler(getTargetContext().getMainLooper());

        when(mSurface.isValid()).thenReturn(true);

        when(mView.getScrollCaptureHint()).thenReturn(View.SCROLL_CAPTURE_HINT_INCLUDE);

        when(mRemote.asBinder()).thenReturn(mConnectionBinder);



        mTarget = new ScrollCaptureTarget(mView, mLocalVisibleRect, mPositionInWindow, mCallback);

        mTarget.setScrollBounds(mScrollBounds);
@@ -107,12 +112,13 @@ public class ScrollCaptureConnectionTest { 
    @Test

    public void testStartCapture() throws Exception {

        mConnection.startCapture(mSurface, mRemote);

        assertTrue(mConnection.isConnected());

        assertFalse(mConnection.isActive());



        mCallback.completeStartRequest();

        assertTrue(mConnection.isActive());



        verify(mRemote, times(1)).onCaptureStarted();

        verifyNoMoreInteractions(mRemote);

    }



    @Test
@@ -123,7 +129,7 @@ public class ScrollCaptureConnectionTest { 
        mCallback.completeStartRequest();

        assertFalse(mConnection.isActive());



        verifyNoMoreInteractions(mRemote);

        verify(mRemote, never()).onCaptureStarted();

    }



    /** @see ScrollCaptureConnection#requestImage(Rect) */
@@ -131,42 +137,63 @@ public class ScrollCaptureConnectionTest { 
    public void testRequestImage() throws Exception {

        mConnection.startCapture(mSurface, mRemote);

        mCallback.completeStartRequest();

        reset(mRemote);



        mConnection.requestImage(new Rect(1, 2, 3, 4));

        mCallback.completeImageRequest(new Rect(1, 2, 3, 4));



        verify(mRemote, times(1))

                .onImageRequestCompleted(eq(0), eq(new Rect(1, 2, 3, 4)));

        verifyNoMoreInteractions(mRemote);

    }



    @Test

    public void testRequestImage_cancellation() throws Exception {

        mConnection.startCapture(mSurface, mRemote);

        mCallback.completeStartRequest();

        reset(mRemote);



        ICancellationSignal signal = mConnection.requestImage(new Rect(1, 2, 3, 4));

        signal.cancel();

        mCallback.completeImageRequest(new Rect(1, 2, 3, 4));



        verifyNoMoreInteractions(mRemote);

        verify(mRemote, never()).onImageRequestCompleted(anyInt(), any(Rect.class));

    }



    /** @see ScrollCaptureConnection#endCapture() */

    @Test

    public void testEndCapture() throws Exception {

        mConnection.startCapture(mSurface, mRemote);

        assertTrue(mConnection.isConnected());



        mCallback.completeStartRequest();

        reset(mRemote);

        assertTrue(mConnection.isActive());



        mConnection.endCapture();

        mCallback.completeEndRequest();

        assertFalse(mConnection.isActive());



        // And the reply is sent

        verify(mRemote, times(1)).onCaptureEnded();

        verifyNoMoreInteractions(mRemote);



        assertFalse(mConnection.isConnected());

    }



    /** @see ScrollCaptureConnection#endCapture() */

    @Test

    public void testClose_withPendingOperation() throws Exception {

        mConnection.startCapture(mSurface, mRemote);

        mCallback.completeStartRequest();



        mConnection.requestImage(new Rect(1, 2, 3, 4));

        assertFalse(mCallback.getLastCancellationSignal().isCanceled());



        mConnection.close();



        // And the reply is sent

        assertTrue(mCallback.onScrollCaptureEndCalled());

        assertTrue(mCallback.getLastCancellationSignal().isCanceled());

        assertFalse(mConnection.isActive());

        assertFalse(mConnection.isConnected());

        verify(mRemote, never()).onCaptureEnded();

        verify(mRemote, never()).onImageRequestCompleted(anyInt(), any(Rect.class));

    }



    /** @see ScrollCaptureConnection#endCapture() */
@@ -174,20 +201,19 @@ public class ScrollCaptureConnectionTest { 
    public void testEndCapture_cancellation() throws Exception {

        mConnection.startCapture(mSurface, mRemote);

        mCallback.completeStartRequest();

        reset(mRemote);



        ICancellationSignal signal = mConnection.endCapture();

        signal.cancel();

        mCallback.completeEndRequest();



        verifyNoMoreInteractions(mRemote);

        verify(mRemote, never()).onCaptureEnded();

    }



    @Test

    public void testClose() {

        mConnection.close();

        assertFalse(mConnection.isActive());

        verifyNoMoreInteractions(mRemote);

        assertFalse(mConnection.isConnected());

    }



    @Test
@@ -196,9 +222,11 @@ public class ScrollCaptureConnectionTest { 


        mCallback.completeStartRequest();

        assertTrue(mConnection.isActive());

        assertTrue(mConnection.isConnected());



        mConnection.close();

        mCallback.completeEndRequest();

        assertFalse(mConnection.isActive());

        assertFalse(mConnection.isConnected());

    }

}

core/tests/coretests/src/android/view/TestScrollCaptureCallback.java

+22 −3
Original line number Diff line number Diff line
@@ -31,10 +31,13 @@ class TestScrollCaptureCallback implements ScrollCaptureCallback { 
    private Consumer<Rect> mImageOnComplete;

    private Runnable mOnEndReady;

    private volatile int mModCount;

    private boolean mOnScrollCaptureEndCalled;

    private CancellationSignal mLastCancellationSignal;



    @Override

    public void onScrollCaptureSearch(@NonNull CancellationSignal signal,

            @NonNull Consumer<Rect> onReady) {

        mLastCancellationSignal = signal;

        mSearchConsumer = onReady;

        mModCount++;

    }
@@ -42,6 +45,7 @@ class TestScrollCaptureCallback implements ScrollCaptureCallback { 
    @Override

    public void onScrollCaptureStart(@NonNull ScrollCaptureSession session,

            @NonNull CancellationSignal signal, @NonNull Runnable onReady) {

        mLastCancellationSignal = signal;

        mStartOnReady = onReady;

        mModCount++;

    }
@@ -50,6 +54,7 @@ class TestScrollCaptureCallback implements ScrollCaptureCallback { 
    public void onScrollCaptureImageRequest(@NonNull ScrollCaptureSession session,

            @NonNull CancellationSignal signal, @NonNull Rect captureArea,

            @NonNull Consumer<Rect> onComplete) {

        mLastCancellationSignal = signal;

        mImageOnComplete = onComplete;

        mModCount++;

    }
@@ -57,8 +62,12 @@ class TestScrollCaptureCallback implements ScrollCaptureCallback { 
    @Override

    public void onScrollCaptureEnd(@NonNull Runnable onReady) {

        mOnEndReady = onReady;

        mOnScrollCaptureEndCalled = true;

    }



    public boolean onScrollCaptureEndCalled() {

        return mOnScrollCaptureEndCalled;

    }

    void completeSearchRequest(Rect scrollBounds) {

        assertNotNull("Did not receive search request", mSearchConsumer);

        mSearchConsumer.accept(scrollBounds);
@@ -71,16 +80,26 @@ class TestScrollCaptureCallback implements ScrollCaptureCallback { 


    void completeStartRequest() {

        assertNotNull("Did not receive start request", mStartOnReady);

        if (mLastCancellationSignal != null && !mLastCancellationSignal.isCanceled()) {

            mStartOnReady.run();

        }

    }



    void completeImageRequest(Rect captured) {

        assertNotNull("Did not receive image request", mImageOnComplete);

        if (mLastCancellationSignal != null && !mLastCancellationSignal.isCanceled()) {

            mImageOnComplete.accept(captured);

        }

    }



    void completeEndRequest() {

        assertNotNull("Did not receive end request", mOnEndReady);

        if (mLastCancellationSignal != null && !mLastCancellationSignal.isCanceled()) {

            mOnEndReady.run();

        }

    }



    public CancellationSignal getLastCancellationSignal() {

        return mLastCancellationSignal;

    }

}

packages/SystemUI/src/com/android/systemui/screenshot/ScreenshotController.java

+74 −56
Original line number Diff line number Diff line
@@ -422,11 +422,6 @@ public class ScreenshotController { 
        } else {

            mScreenshotView.animateDismissal();

        }



        if (mLastScrollCaptureResponse != null) {

            mLastScrollCaptureResponse.close();

            mLastScrollCaptureResponse = null;

        }

    }



    boolean isPendingSharedTransition() {
@@ -456,6 +451,9 @@ public class ScreenshotController { 
        mScreenshotView.init(mUiEventLogger, new ScreenshotView.ScreenshotViewCallback() {

            @Override

            public void onUserInteraction() {

                if (DEBUG_INPUT) {

                    Log.d(TAG, "onUserInteraction");

                }

                resetTimeout();

            }
@@ -670,19 +668,32 @@ public class ScreenshotController { 
                mScreenshotView.prepareScrollingTransition(response, mScreenBitmap, newScreenshot,

                        mScreenshotTakenInPortrait);

                // delay starting scroll capture to make sure the scrim is up before the app moves

                mScreenshotView.post(() -> {

                mScreenshotView.post(() -> runBatchScrollCapture(response));

            });

        } catch (CancellationException e) {

            // Ignore

        } catch (InterruptedException | ExecutionException e) {

            Log.e(TAG, "requestScrollCapture failed", e);

        }

    }

    ListenableFuture<ScrollCaptureController.LongScreenshot> mLongScreenshotFuture;



    private void runBatchScrollCapture(ScrollCaptureResponse response) {

        // Clear the reference to prevent close() in dismissScreenshot

        mLastScrollCaptureResponse = null;

                    final ListenableFuture<ScrollCaptureController.LongScreenshot> future =

                            mScrollCaptureController.run(response);

                    future.addListener(() -> {

                        ScrollCaptureController.LongScreenshot longScreenshot;



        if (mLongScreenshotFuture != null) {

            mLongScreenshotFuture.cancel(true);

        }

        mLongScreenshotFuture = mScrollCaptureController.run(response);

        mLongScreenshotFuture.addListener(() -> {

            ScrollCaptureController.LongScreenshot longScreenshot;

            try {

                            longScreenshot = future.get();

                        } catch (CancellationException

                                | InterruptedException

                                | ExecutionException e) {

                longScreenshot = mLongScreenshotFuture.get();

            } catch (CancellationException e) {

                Log.e(TAG, "Long screenshot cancelled");

                return;

            } catch (InterruptedException | ExecutionException e) {

                Log.e(TAG, "Exception", e);

                mScreenshotView.restoreNonScrollingUi();

                return;
@@ -715,13 +726,6 @@ public class ScreenshotController { 
                Log.e(TAG, "Error overriding screenshot app transition", e);

            }

        }, mMainExecutor);

                });

            });

        } catch (CancellationException e) {

            // Ignore

        } catch (InterruptedException | ExecutionException e) {

            Log.e(TAG, "requestScrollCapture failed", e);

        }

    }



    private void withWindowAttached(Runnable action) {
@@ -824,16 +828,27 @@ public class ScreenshotController { 


    /** Reset screenshot view and then call onCompleteRunnable */

    private void finishDismiss() {

        if (DEBUG_UI) {

        if (DEBUG_DISMISS) {

            Log.d(TAG, "finishDismiss");

        }

        cancelTimeout();

        removeWindow();

        mScreenshotView.reset();

        if (mLastScrollCaptureRequest != null) {

            mLastScrollCaptureRequest.cancel(true);

            mLastScrollCaptureRequest = null;

        }

        if (mLastScrollCaptureResponse != null) {

            mLastScrollCaptureResponse.close();

            mLastScrollCaptureResponse = null;

        }

        if (mLongScreenshotFuture != null) {

            mLongScreenshotFuture.cancel(true);

        }

        if (mCurrentRequestCallback != null) {

            mCurrentRequestCallback.onFinish();

            mCurrentRequestCallback = null;

        }

        mScreenshotView.reset();

        removeWindow();

        cancelTimeout();

    }



    /**
@@ -861,6 +876,9 @@ public class ScreenshotController { 
    }



    private void cancelTimeout() {

        if (DEBUG_DISMISS) {

            Log.d(TAG, "cancel timeout");

        }

        mScreenshotHandler.removeMessages(MESSAGE_CORNER_TIMEOUT);

    }
@@ -876,7 +894,7 @@ public class ScreenshotController { 
        mScreenshotHandler.sendMessageDelayed(

                mScreenshotHandler.obtainMessage(MESSAGE_CORNER_TIMEOUT),

                timeoutMs);

        if (DEBUG_UI) {

        if (DEBUG_DISMISS) {

            Log.d(TAG, "dismiss timeout: " + timeoutMs + " ms");

        }

packages/SystemUI/src/com/android/systemui/screenshot/ScreenshotView.java

+6 −6
Original line number Diff line number Diff line
@@ -1117,9 +1117,11 @@ public class ScreenshotView extends FrameLayout implements 
                mPreviousX = mStartX;

                return true;

            } else if (event.getActionMasked() == MotionEvent.ACTION_UP) {

                if (isPastDismissThreshold()

                        && (mDismissAnimation == null || !mDismissAnimation.isRunning())) {

                    if (DEBUG_INPUT) {

                if (mDismissAnimation != null && mDismissAnimation.isRunning()) {

                    return true;

                }

                if (isPastDismissThreshold()) {

                    if (DEBUG_DISMISS) {

                        Log.d(TAG, "dismiss triggered via swipe gesture");

                    }

                    mUiEventLogger.log(ScreenshotEvent.SCREENSHOT_SWIPE_DISMISSED);
@@ -1129,10 +1131,8 @@ public class ScreenshotView extends FrameLayout implements 
                    if (DEBUG_DISMISS) {

                        Log.d(TAG, "swipe gesture abandoned");

                    }

                    if ((mDismissAnimation == null || !mDismissAnimation.isRunning())) {

                    createSwipeReturnAnimation().start();

                }

                }

                return true;

            }

            return gestureResult;