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

Commit b04d4f17 authored by Mark Renouf's avatar Mark Renouf
Browse files

Null and thread safety fixes 

Most of these are related to error handling and cancellation. When
shutting down or failing, cancellation signals, binder and view
references are cleared. Callbacks that have been queued must check
for null before invoking. These calls may come from either the
ScrollCaptureCallback (app side) or ScrollCaptureConnection
(SystemUI side).

Bug: 370630810
Bug: 325469181
Bug: 365207886
Bug: 341544887
Bug: 278379299
Test: ScrollCaptureConnectionTest ScrollCaptureControllerTest
Test: ScrollCaptureClientTest
Flag: NONE - null safety fixes, no change to normal behavior
Change-Id: I2ff8390e96b66dd5cbc35e57c2c6a3c05065fa5e
parent c77bf043

core/java/android/view/ScrollCaptureConnection.java

+34 −10
Original line number Diff line number Diff line
@@ -20,8 +20,9 @@ import static android.os.Trace.TRACE_TAG_GRAPHICS; 


import static java.util.Objects.requireNonNull;



import android.annotation.BinderThread;

import android.annotation.AnyThread;

import android.annotation.NonNull;

import android.annotation.Nullable;

import android.annotation.UiThread;

import android.graphics.Point;

import android.graphics.Rect;
@@ -64,9 +65,13 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub imple 
    private final Executor mUiThread;

    private final CloseGuard mCloseGuard = new CloseGuard();



    @Nullable

    private ScrollCaptureCallback mLocal;

    @Nullable

    private IScrollCaptureCallbacks mRemote;

    @Nullable

    private ScrollCaptureSession mSession;

    @Nullable

    private CancellationSignal mCancellation;



    private volatile boolean mActive;
@@ -92,7 +97,7 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub imple 
        mPositionInWindow = new Point(selectedTarget.getPositionInWindow());

    }



    @BinderThread

    @AnyThread

    @Override

    public ICancellationSignal startCapture(@NonNull Surface surface,

            @NonNull IScrollCaptureCallbacks remote) throws RemoteException {
@@ -115,7 +120,11 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub imple 
        Runnable listener =

                SafeCallback.create(mCancellation, mUiThread, this::onStartCaptureCompleted);

        // -> UiThread

        mUiThread.execute(() -> mLocal.onScrollCaptureStart(mSession, mCancellation, listener));

        mUiThread.execute(() -> {

            if (mLocal != null && mCancellation != null) {

                mLocal.onScrollCaptureStart(mSession, mCancellation, listener);

            }

        });

        return cancellation;

    }
@@ -123,7 +132,11 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub imple 
    private void onStartCaptureCompleted() {

        mActive = true;

        try {

            if (mRemote != null) {

                mRemote.onCaptureStarted();

            } else {

                close();

            }

        } catch (RemoteException e) {

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

            close();
@@ -132,7 +145,7 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub imple 
        Trace.asyncTraceForTrackEnd(TRACE_TAG_GRAPHICS, TRACE_TRACK, mTraceId);

    }



    @BinderThread

    @AnyThread

    @Override

    public ICancellationSignal requestImage(Rect requestRect) throws RemoteException {

        Trace.asyncTraceForTrackBegin(TRACE_TAG_GRAPHICS, TRACE_TRACK, REQUEST_IMAGE, mTraceId);
@@ -145,7 +158,7 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub imple 
                SafeCallback.create(mCancellation, mUiThread, this::onImageRequestCompleted);

        // -> UiThread

        mUiThread.execute(() -> {

            if (mLocal != null) {

            if (mLocal != null && mSession != null && mCancellation != null) {

                mLocal.onScrollCaptureImageRequest(

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

            }
@@ -157,7 +170,11 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub imple 
    @UiThread

    void onImageRequestCompleted(Rect capturedArea) {

        try {

            if (mRemote != null) {

                mRemote.onImageRequestCompleted(0, capturedArea);

            } else {

                close();

            }

        } catch (RemoteException e) {

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

            close();
@@ -167,7 +184,7 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub imple 
        Trace.asyncTraceForTrackEnd(TRACE_TAG_GRAPHICS, TRACE_TRACK, mTraceId);

    }



    @BinderThread

    @AnyThread

    @Override

    public ICancellationSignal endCapture() throws RemoteException {

        Trace.asyncTraceForTrackBegin(TRACE_TAG_GRAPHICS, TRACE_TRACK, END_CAPTURE, mTraceId);
@@ -212,7 +229,7 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub imple 


    }



    @BinderThread

    @AnyThread

    @Override

    public synchronized void close() {

        Trace.instantForTrack(TRACE_TAG_GRAPHICS, TRACE_TRACK, "close");
@@ -220,7 +237,11 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub imple 
            Log.w(TAG, "close(): capture session still active! Ending now.");

            cancelPendingAction();

            final ScrollCaptureCallback callback = mLocal;

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

            mUiThread.execute(() -> {

                if (callback != null) {

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

                }

            });

            mActive = false;

        }

        if (mRemote != null) {
@@ -297,10 +318,13 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub imple 
        protected final void maybeAccept(Consumer<T> consumer) {

            T value = mValue.getAndSet(null);

            if (mSignal.isCanceled()) {

                Log.w(TAG, "callback ignored, operation already cancelled");

                return;

            }

            if (value != null) {

                mExecutor.execute(() -> consumer.accept(value));

            } else {

                Log.w(TAG, "callback ignored, value already delivered");

            }

        }

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

+62 −8
Original line number Diff line number Diff line
@@ -16,8 +16,6 @@ 


package android.view;



import static androidx.test.InstrumentationRegistry.getTargetContext;



import static org.junit.Assert.assertFalse;

import static org.junit.Assert.assertTrue;

import static org.junit.Assert.fail;
@@ -32,7 +30,6 @@ 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;
@@ -54,7 +51,6 @@ import java.util.concurrent.Executor; 
/**

 * Tests of {@link ScrollCaptureConnection}.

 */

@SuppressWarnings("UnnecessaryLocalVariable")

@Presubmit

@SmallTest

@RunWith(AndroidJUnit4.class)
@@ -68,9 +64,8 @@ public class ScrollCaptureConnectionTest { 


    private ScrollCaptureTarget mTarget;

    private ScrollCaptureConnection mConnection;

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

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



    private Handler mHandler;



    @Mock

    private Surface mSurface;
@@ -85,7 +80,6 @@ public class ScrollCaptureConnectionTest { 
    @Before

    public void setUp() {

        MockitoAnnotations.initMocks(this);

        mHandler = new Handler(getTargetContext().getMainLooper());

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

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

        when(mRemote.asBinder()).thenReturn(mConnectionBinder);
@@ -269,8 +263,68 @@ public class ScrollCaptureConnectionTest { 
        assertFalse(mConnection.isConnected());

    }



    @Test(expected = RemoteException.class)

    public void testRequestImage_beforeStarted() throws RemoteException {

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

    }





    @Test(expected = RemoteException.class)

    public void testRequestImage_beforeStartCompleted() throws RemoteException {

        mFakeUiThread.setImmediate(false);

        mConnection.startCapture(mSurface, mRemote);

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

        mFakeUiThread.runAll();

    }



    @Test

    public void testCompleteStart_afterClosing() throws RemoteException {

        mConnection.startCapture(mSurface, mRemote);

        mConnection.close();

        mFakeUiThread.setImmediate(false);

        mCallback.completeStartRequest();

        mFakeUiThread.runAll();

    }



    @Test

    public void testLateCallbacks() throws RemoteException {

        mConnection.startCapture(mSurface, mRemote);

        mCallback.completeStartRequest();

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

        mConnection.endCapture();

        mFakeUiThread.setImmediate(false);

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

        mCallback.completeEndRequest();

        mFakeUiThread.runAll();

    }



    @Test

    public void testDelayedClose() throws RemoteException {

        mConnection.startCapture(mSurface, mRemote);

        mCallback.completeStartRequest();

        mFakeUiThread.setImmediate(false);

        mConnection.endCapture();

        mFakeUiThread.runAll();

        mConnection.close();

        mCallback.completeEndRequest();

        mFakeUiThread.runAll();

    }



    @Test

    public void testRequestImage_delayedCancellation() throws Exception {

        mConnection.startCapture(mSurface, mRemote);

        mCallback.completeStartRequest();



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

        mFakeUiThread.setImmediate(false);



        signal.cancel();

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

    }





    static class FakeExecutor implements Executor {

        private Queue<Runnable> mQueue = new ArrayDeque<>();

        private final Queue<Runnable> mQueue = new ArrayDeque<>();

        private boolean mImmediate;



        @Override

packages/SystemUI/multivalentTests/src/com/android/systemui/screenshot/scroll/ScrollCaptureControllerTest.java

+15 −0
Original line number Diff line number Diff line
@@ -204,6 +204,21 @@ public class ScrollCaptureControllerTest extends SysuiTestCase { 
        assertEquals("bottom", 200, screenshot.getBottom());

    }



    @Test

    public void testCancellation() {

        ScrollCaptureController controller = new TestScenario()

                .withPageHeight(100)

                .withMaxPages(2.5f)

                .withTileHeight(10)

                .withAvailableRange(-10, Integer.MAX_VALUE)

                .createController(mContext);



        ScrollCaptureController.LongScreenshot screenshot =

                getUnchecked(controller.run(EMPTY_RESPONSE));



        assertEquals("top", -10, screenshot.getTop());

        assertEquals("bottom", 240, screenshot.getBottom());

    }

    /**

     * Build and configure a stubbed controller for each test case.

     */

packages/SystemUI/src/com/android/systemui/screenshot/scroll/ScrollCaptureController.java

+18 −5
Original line number Diff line number Diff line
@@ -24,6 +24,8 @@ import android.provider.Settings; 
import android.util.Log;

import android.view.ScrollCaptureResponse;



import androidx.annotation.NonNull;

import androidx.annotation.Nullable;

import androidx.concurrent.futures.CallbackToFutureAdapter;

import androidx.concurrent.futures.CallbackToFutureAdapter.Completer;
@@ -68,11 +70,15 @@ public class ScrollCaptureController { 
    private final UiEventLogger mEventLogger;

    private final ScrollCaptureClient mClient;



    @Nullable

    private Completer<LongScreenshot> mCaptureCompleter;



    @Nullable

    private ListenableFuture<Session> mSessionFuture;

    private Session mSession;

    @Nullable

    private ListenableFuture<CaptureResult> mTileFuture;

    @Nullable

    private ListenableFuture<Void> mEndFuture;

    private String mWindowOwner;

    private volatile boolean mCancelled;
@@ -148,8 +154,9 @@ public class ScrollCaptureController { 
    }



    @Inject

    ScrollCaptureController(Context context, @Background Executor bgExecutor,

            ScrollCaptureClient client, ImageTileSet imageTileSet, UiEventLogger logger) {

    ScrollCaptureController(@NonNull Context context, @Background Executor bgExecutor,

            @NonNull ScrollCaptureClient client, @NonNull ImageTileSet imageTileSet,

            @NonNull UiEventLogger logger) {

        mContext = context;

        mBgExecutor = bgExecutor;

        mClient = client;
@@ -214,7 +221,9 @@ public class ScrollCaptureController { 
        } catch (InterruptedException | ExecutionException e) {

            // Failure to start, propagate to caller

            Log.e(TAG, "session start failed!");

            if (mCaptureCompleter != null) {

                mCaptureCompleter.setException(e);

            }

            mEventLogger.log(ScreenshotEvent.SCREENSHOT_LONG_SCREENSHOT_FAILURE, 0, mWindowOwner);

        }

    }
@@ -235,8 +244,10 @@ public class ScrollCaptureController { 
                Log.e(TAG, "requestTile cancelled");

            } catch (InterruptedException | ExecutionException e) {

                Log.e(TAG, "requestTile failed!", e);

                if (mCaptureCompleter != null) {

                    mCaptureCompleter.setException(e);

                }

            }

        }, mBgExecutor);

    }
@@ -350,7 +361,9 @@ public class ScrollCaptureController { 
            }

            // Provide result to caller and complete the top-level future

            // Caller is responsible for releasing this resource (ImageReader/HardwareBuffers)

            if (mCaptureCompleter != null) {

                mCaptureCompleter.set(new LongScreenshot(mSession, mImageTileSet));

            }

        }, mContext.getMainExecutor());

    }

}