From ac9cef964f1634541e1b29ec6d71270101e8982d Mon Sep 17 00:00:00 2001
From: Mark Renouf <mrenouf@google.com>
Date: Wed, 18 May 2022 19:17:47 +0000
Subject: [PATCH] Avoid NPE on ScrollCaptureConnection callbacks

State is checked at the front half of some calls, but is
dispatched onto the UI thread. If the connection is closed
in this time, the queued call will NPE when the field it
uses is cleared.

This adds a simple null check in two places, and test for both.

Bug: 232375183
Test: atest ScrollCaptureConnectionTest
Change-Id: Ida7fbf0a21c206db8d774dc247b6ab5257dacabe
---
 .../android/view/ScrollCaptureConnection.java | 15 ++++-
 .../view/ScrollCaptureConnectionTest.java     | 65 ++++++++++++++++++-
 2 files changed, 76 insertions(+), 4 deletions(-)

diff --git a/core/java/android/view/ScrollCaptureConnection.java b/core/java/android/view/ScrollCaptureConnection.java
index 0f27989642a91..d70de74ee75f4 100644
--- a/core/java/android/view/ScrollCaptureConnection.java
+++ b/core/java/android/view/ScrollCaptureConnection.java
@@ -144,8 +144,13 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub imple
         Consumer<Rect> listener =
                 SafeCallback.create(mCancellation, mUiThread, this::onImageRequestCompleted);
         // -> UiThread
-        mUiThread.execute(() -> mLocal.onScrollCaptureImageRequest(
-                mSession, mCancellation, new Rect(requestRect), listener));
+        mUiThread.execute(() -> {
+            if (mLocal != null) {
+                mLocal.onScrollCaptureImageRequest(
+                        mSession, mCancellation, new Rect(requestRect), listener);
+            }
+        });
+
         return cancellation;
     }
 
@@ -174,7 +179,11 @@ public class ScrollCaptureConnection extends IScrollCaptureConnection.Stub imple
         Runnable listener =
                 SafeCallback.create(mCancellation, mUiThread, this::onEndCaptureCompleted);
         // -> UiThread
-        mUiThread.execute(() -> mLocal.onScrollCaptureEnd(listener));
+        mUiThread.execute(() -> {
+            if (mLocal != null) {
+                mLocal.onScrollCaptureEnd(listener);
+            }
+        });
         return cancellation;
     }
 
diff --git a/core/tests/coretests/src/android/view/ScrollCaptureConnectionTest.java b/core/tests/coretests/src/android/view/ScrollCaptureConnectionTest.java
index 469be4c2b008e..5f258949857af 100644
--- a/core/tests/coretests/src/android/view/ScrollCaptureConnectionTest.java
+++ b/core/tests/coretests/src/android/view/ScrollCaptureConnectionTest.java
@@ -47,6 +47,10 @@ import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 
+import java.util.ArrayDeque;
+import java.util.Queue;
+import java.util.concurrent.Executor;
+
 /**
  * Tests of {@link ScrollCaptureConnection}.
  */
@@ -56,6 +60,7 @@ import org.mockito.MockitoAnnotations;
 @RunWith(AndroidJUnit4.class)
 public class ScrollCaptureConnectionTest {
 
+
     private final Point mPositionInWindow = new Point(1, 2);
     private final Rect mLocalVisibleRect = new Rect(2, 3, 4, 5);
     private final Rect mScrollBounds = new Rect(3, 4, 5, 6);
@@ -73,6 +78,9 @@ public class ScrollCaptureConnectionTest {
     private IScrollCaptureCallbacks mRemote;
     @Mock
     private View mView;
+    private FakeExecutor mFakeUiThread;
+
+    private final Executor mImmediateExecutor = Runnable::run;
 
     @Before
     public void setUp() {
@@ -84,7 +92,9 @@ public class ScrollCaptureConnectionTest {
 
         mTarget = new ScrollCaptureTarget(mView, mLocalVisibleRect, mPositionInWindow, mCallback);
         mTarget.setScrollBounds(mScrollBounds);
-        mConnection = new ScrollCaptureConnection(Runnable::run, mTarget);
+        mFakeUiThread = new FakeExecutor();
+        mFakeUiThread.setImmediate(true);
+        mConnection = new ScrollCaptureConnection(mFakeUiThread, mTarget);
     }
 
     /** Test creating a client with valid info */
@@ -145,6 +155,21 @@ public class ScrollCaptureConnectionTest {
                 .onImageRequestCompleted(eq(0), eq(new Rect(1, 2, 3, 4)));
     }
 
+    /** Test closing while callbacks are in-flight: b/232375183 */
+    @Test
+    public void testRequestImage_afterClose() throws Exception {
+        mConnection = new ScrollCaptureConnection(mFakeUiThread, mTarget);
+        mConnection.startCapture(mSurface, mRemote);
+        mCallback.completeStartRequest();
+
+        mFakeUiThread.setImmediate(false);
+        mConnection.requestImage(new Rect(1, 2, 3, 4));
+
+        // Now close connection, before the UI thread executes
+        mConnection.close();
+        mFakeUiThread.runAll();
+    }
+
     @Test
     public void testRequestImage_cancellation() throws Exception {
         mConnection.startCapture(mSurface, mRemote);
@@ -176,6 +201,20 @@ public class ScrollCaptureConnectionTest {
         assertFalse(mConnection.isConnected());
     }
 
+    /** Test closing while callbacks are in-flight: b/232375183 */
+    @Test
+    public void testEndCapture_afterClose() throws Exception {
+        mConnection.startCapture(mSurface, mRemote);
+        mCallback.completeStartRequest();
+
+        mFakeUiThread.setImmediate(false);
+        mConnection.endCapture();
+
+        // Now close connection, before the UI thread executes
+        mConnection.close();
+        mFakeUiThread.runAll();
+    }
+
     /** @see ScrollCaptureConnection#endCapture() */
     @Test
     public void testClose_withPendingOperation() throws Exception {
@@ -229,4 +268,28 @@ public class ScrollCaptureConnectionTest {
         assertFalse(mConnection.isActive());
         assertFalse(mConnection.isConnected());
     }
+
+    static class FakeExecutor implements Executor {
+        private Queue<Runnable> mQueue = new ArrayDeque<>();
+        private boolean mImmediate;
+
+        @Override
+        public void execute(Runnable command) {
+            if (mImmediate) {
+                command.run();
+            } else {
+                mQueue.add(command);
+            }
+        }
+
+        void setImmediate(boolean immediate) {
+            mImmediate = immediate;
+        }
+
+        public void runAll() {
+            while (!mQueue.isEmpty()) {
+                mQueue.remove().run();
+            }
+        }
+    }
 }