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

Commit ec86f4b9 authored by Matt Casey's avatar Matt Casey
Browse files

Fix Race in ScreenshotController 

(Fixing merge for ag/26554944)

TakeScreenshotExecutor was creating the ScreenshotController by passing the
displayId. However, if the display was disconnected before ScreenshotController
was fully initialized, it ended up crashing (exactly when creating the display
context).

Now, ScreenshotController is not created unless we have a not-null display in
TakescreenshotExecutor.

Test: atest TakeScreenshotExecutorTest
Flag: None
Fixes: 329147174
Change-Id: I0c7c9ec4e7d3dfc8896bd325bd250d372a6c0891
parent f57b481f

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

+22 −28
Original line number Diff line number Diff line
@@ -47,7 +47,6 @@ import android.content.res.Configuration; 
import android.graphics.Bitmap;

import android.graphics.Insets;

import android.graphics.Rect;

import android.hardware.display.DisplayManager;

import android.net.Uri;

import android.os.Process;

import android.os.UserHandle;
@@ -208,8 +207,7 @@ public class ScreenshotController { 
    @Nullable

    private final ScreenshotSoundController mScreenshotSoundController;

    private final PhoneWindow mWindow;

    private final DisplayManager mDisplayManager;

    private final int mDisplayId;

    private final Display mDisplay;

    private final ScrollCaptureExecutor mScrollCaptureExecutor;

    private final ScreenshotNotificationSmartActionsProvider

            mScreenshotNotificationSmartActionsProvider;
@@ -249,7 +247,6 @@ public class ScreenshotController { 
    @AssistedInject

    ScreenshotController(

            Context context,

            DisplayManager displayManager,

            WindowManager windowManager,

            FeatureFlags flags,

            ScreenshotViewProxy.Factory viewProxyFactory,
@@ -271,12 +268,13 @@ public class ScreenshotController { 
            AssistContentRequester assistContentRequester,

            MessageContainerController messageContainerController,

            Provider<ScreenshotSoundController> screenshotSoundController,

            @Assisted int displayId,

            @Assisted Display display,

            @Assisted boolean showUIOnExternalDisplay

    ) {

        mScreenshotSmartActions = screenshotSmartActions;

        mActionsProviderFactory = actionsProviderFactory;

        mNotificationsController = screenshotNotificationsControllerFactory.create(displayId);

        mNotificationsController = screenshotNotificationsControllerFactory.create(

                display.getDisplayId());

        mUiEventLogger = uiEventLogger;

        mImageExporter = imageExporter;

        mImageCapture = imageCapture;
@@ -290,11 +288,9 @@ public class ScreenshotController { 
        mScreenshotHandler = timeoutHandler;

        mScreenshotHandler.setDefaultTimeoutMillis(SCREENSHOT_CORNER_DEFAULT_TIMEOUT_MILLIS);





        mDisplayId = displayId;

        mDisplayManager = displayManager;

        mDisplay = display;

        mWindowManager = windowManager;

        final Context displayContext = context.createDisplayContext(getDisplay());

        final Context displayContext = context.createDisplayContext(display);

        mContext = (WindowContext) displayContext.createWindowContext(TYPE_SCREENSHOT, null);

        mFlags = flags;

        mActionIntentExecutor = actionIntentExecutor;
@@ -302,7 +298,7 @@ public class ScreenshotController { 
        mMessageContainerController = messageContainerController;

        mAssistContentRequester = assistContentRequester;



        mViewProxy = viewProxyFactory.getProxy(mContext, mDisplayId);

        mViewProxy = viewProxyFactory.getProxy(mContext, mDisplay.getDisplayId());



        mScreenshotHandler.setOnTimeoutRunnable(() -> {

            if (DEBUG_UI) {
@@ -328,7 +324,7 @@ public class ScreenshotController { 
                });



        // Sound is only reproduced from the controller of the default display.

        if (displayId == Display.DEFAULT_DISPLAY) {

        if (mDisplay.getDisplayId() == Display.DEFAULT_DISPLAY) {

            mScreenshotSoundController = screenshotSoundController.get();

        } else {

            mScreenshotSoundController = null;
@@ -356,7 +352,7 @@ public class ScreenshotController { 
        if (screenshot.getType() == WindowManager.TAKE_SCREENSHOT_FULLSCREEN

                && screenshot.getBitmap() == null) {

            Rect bounds = getFullScreenRect();

            screenshot.setBitmap(mImageCapture.captureDisplay(mDisplayId, bounds));

            screenshot.setBitmap(mImageCapture.captureDisplay(mDisplay.getDisplayId(), bounds));

            screenshot.setScreenBounds(bounds);

        }
@@ -459,7 +455,7 @@ public class ScreenshotController { 
    }



    private boolean shouldShowUi() {

        return mDisplayId == Display.DEFAULT_DISPLAY || mShowUIOnExternalDisplay;

        return mDisplay.getDisplayId() == Display.DEFAULT_DISPLAY || mShowUIOnExternalDisplay;

    }



    void prepareViewForNewScreenshot(@NonNull ScreenshotData screenshot, String oldPackageName) {
@@ -618,7 +614,7 @@ public class ScreenshotController { 


    private void requestScrollCapture(UserHandle owner) {

        mScrollCaptureExecutor.requestScrollCapture(

                mDisplayId,

                mDisplay.getDisplayId(),

                mWindow.getDecorView().getWindowToken(),

                (response) -> {

                    mUiEventLogger.log(ScreenshotEvent.SCREENSHOT_LONG_SCREENSHOT_IMPRESSION,
@@ -641,7 +637,8 @@ public class ScreenshotController { 
        }

        mUiEventLogger.log(ScreenshotEvent.SCREENSHOT_LONG_SCREENSHOT_REQUESTED, 0,

                response.getPackageName());

        Bitmap newScreenshot = mImageCapture.captureDisplay(mDisplayId, getFullScreenRect());

        Bitmap newScreenshot = mImageCapture.captureDisplay(mDisplay.getDisplayId(),

                getFullScreenRect());

        if (newScreenshot == null) {

            Log.e(TAG, "Failed to capture current screenshot for scroll transition!");

            return;
@@ -819,7 +816,8 @@ public class ScreenshotController { 
    private void saveScreenshotInBackground(

            ScreenshotData screenshot, UUID requestId, Consumer<Uri> finisher) {

        ListenableFuture<ImageExporter.Result> future = mImageExporter.export(mBgExecutor,

                requestId, screenshot.getBitmap(), screenshot.getUserOrDefault(), mDisplayId);

                requestId, screenshot.getBitmap(), screenshot.getUserOrDefault(),

                mDisplay.getDisplayId());

        future.addListener(() -> {

            try {

                ImageExporter.Result result = future.get();
@@ -861,7 +859,7 @@ public class ScreenshotController { 
        data.mActionsReadyListener = actionsReadyListener;

        data.mQuickShareActionsReadyListener = quickShareActionsReadyListener;

        data.owner = owner;

        data.displayId = mDisplayId;

        data.displayId = mDisplay.getDisplayId();



        if (mSaveInBgTask != null) {

            // just log success/failure for the pre-existing screenshot
@@ -986,13 +984,9 @@ public class ScreenshotController { 
        }

    }



    private Display getDisplay() {

        return mDisplayManager.getDisplay(mDisplayId);

    }



    private Rect getFullScreenRect() {

        DisplayMetrics displayMetrics = new DisplayMetrics();

        getDisplay().getRealMetrics(displayMetrics);

        mDisplay.getRealMetrics(displayMetrics);

        return new Rect(0, 0, displayMetrics.widthPixels, displayMetrics.heightPixels);

    }
@@ -1026,12 +1020,12 @@ public class ScreenshotController { 
    @AssistedFactory

    public interface Factory {

        /**

         * Creates an instance of the controller for that specific displayId.

         * Creates an instance of the controller for that specific display.

         *

         * @param displayId:               display to capture

         * @param showUIOnExternalDisplay: Whether the UI should be shown if this is an external

         * @param display                 display to capture

         * @param showUIOnExternalDisplay Whether the UI should be shown if this is an external

         *                                display.

         */

        ScreenshotController create(int displayId, boolean showUIOnExternalDisplay);

        ScreenshotController create(Display display, boolean showUIOnExternalDisplay);

    }

}

packages/SystemUI/src/com/android/systemui/screenshot/TakeScreenshotExecutor.kt

+13 −10
Original line number Diff line number Diff line
@@ -68,11 +68,13 @@ constructor( 
        onSaved: (Uri?) -> Unit,

        requestCallback: RequestCallback

    ) {

        val displayIds = getDisplaysToScreenshot(screenshotRequest.type)

        val displays = getDisplaysToScreenshot(screenshotRequest.type)

        val resultCallbackWrapper = MultiResultCallbackWrapper(requestCallback)

        displayIds.forEach { displayId: Int ->

        displays.forEach { display ->

            val displayId = display.displayId

            Log.d(TAG, "Executing screenshot for display $displayId")

            dispatchToController(

                display = display,

                rawScreenshotData = ScreenshotData.fromRequest(screenshotRequest, displayId),

                onSaved =

                    if (displayId == Display.DEFAULT_DISPLAY) {
@@ -85,6 +87,7 @@ constructor( 


    /** All logging should be triggered only by this method. */

    private suspend fun dispatchToController(

        display: Display,

        rawScreenshotData: ScreenshotData,

        onSaved: (Uri?) -> Unit,

        callback: RequestCallback
@@ -104,8 +107,7 @@ constructor( 
        logScreenshotRequested(screenshotData)

        Log.d(TAG, "Screenshot request: $screenshotData")

        try {

            getScreenshotController(screenshotData.displayId)

                .handleScreenshot(screenshotData, onSaved, callback)

            getScreenshotController(display).handleScreenshot(screenshotData, onSaved, callback)

        } catch (e: IllegalStateException) {

            Log.e(TAG, "Error while ScreenshotController was handling ScreenshotData!", e)

            onFailedScreenshotRequest(screenshotData, callback)
@@ -135,12 +137,13 @@ constructor( 
        callback.reportError()

    }



    private suspend fun getDisplaysToScreenshot(requestType: Int): List<Int> {

    private suspend fun getDisplaysToScreenshot(requestType: Int): List<Display> {

        val allDisplays = displays.first()

        return if (requestType == TAKE_SCREENSHOT_PROVIDED_IMAGE) {

            // If this is a provided image, let's show the UI on the default display only.

            listOf(Display.DEFAULT_DISPLAY)

            allDisplays.filter { it.displayId == Display.DEFAULT_DISPLAY }

        } else {

            displays.first().filter { it.type in ALLOWED_DISPLAY_TYPES }.map { it.displayId }

            allDisplays.filter { it.type in ALLOWED_DISPLAY_TYPES }

        }

    }
@@ -170,9 +173,9 @@ constructor( 
        screenshotControllers.clear()

    }



    private fun getScreenshotController(id: Int): ScreenshotController {

        return screenshotControllers.computeIfAbsent(id) {

            screenshotControllerFactory.create(id, /* showUIOnExternalDisplay= */ false)

    private fun getScreenshotController(display: Display): ScreenshotController {

        return screenshotControllers.computeIfAbsent(display.displayId) {

            screenshotControllerFactory.create(display, /* showUIOnExternalDisplay= */ false)

        }

    }

packages/SystemUI/tests/src/com/android/systemui/screenshot/TakeScreenshotExecutorTest.kt

+13 −8
Original line number Diff line number Diff line
@@ -69,8 +69,9 @@ class TakeScreenshotExecutorTest : SysuiTestCase() { 


    @Before

    fun setUp() {

        whenever(controllerFactory.create(eq(0), any())).thenReturn(controller0)

        whenever(controllerFactory.create(eq(1), any())).thenReturn(controller1)

        whenever(controllerFactory.create(any(), any())).thenAnswer {

            if (it.getArgument<Display>(0).displayId == 0) controller0 else controller1

        }

        whenever(notificationControllerFactory.create(eq(0))).thenReturn(notificationsController0)

        whenever(notificationControllerFactory.create(eq(1))).thenReturn(notificationsController1)

    }
@@ -78,12 +79,14 @@ class TakeScreenshotExecutorTest : SysuiTestCase() { 
    @Test

    fun executeScreenshots_severalDisplays_callsControllerForEachOne() =

        testScope.runTest {

            setDisplays(display(TYPE_INTERNAL, id = 0), display(TYPE_EXTERNAL, id = 1))

            val internalDisplay = display(TYPE_INTERNAL, id = 0)

            val externalDisplay = display(TYPE_EXTERNAL, id = 1)

            setDisplays(internalDisplay, externalDisplay)

            val onSaved = { _: Uri? -> }

            screenshotExecutor.executeScreenshots(createScreenshotRequest(), onSaved, callback)



            verify(controllerFactory).create(eq(0), any())

            verify(controllerFactory).create(eq(1), any())

            verify(controllerFactory).create(eq(internalDisplay), any())

            verify(controllerFactory).create(eq(externalDisplay), any())



            val capturer = ArgumentCaptor<ScreenshotData>()
@@ -107,7 +110,9 @@ class TakeScreenshotExecutorTest : SysuiTestCase() { 
    @Test

    fun executeScreenshots_providedImageType_callsOnlyDefaultDisplayController() =

        testScope.runTest {

            setDisplays(display(TYPE_INTERNAL, id = 0), display(TYPE_EXTERNAL, id = 1))

            val internalDisplay = display(TYPE_INTERNAL, id = 0)

            val externalDisplay = display(TYPE_EXTERNAL, id = 1)

            setDisplays(internalDisplay, externalDisplay)

            val onSaved = { _: Uri? -> }

            screenshotExecutor.executeScreenshots(

                createScreenshotRequest(TAKE_SCREENSHOT_PROVIDED_IMAGE),
@@ -115,8 +120,8 @@ class TakeScreenshotExecutorTest : SysuiTestCase() { 
                callback

            )



            verify(controllerFactory).create(eq(0), any())

            verify(controllerFactory, never()).create(eq(1), any())

            verify(controllerFactory).create(eq(internalDisplay), any())

            verify(controllerFactory, never()).create(eq(externalDisplay), any())



            val capturer = ArgumentCaptor<ScreenshotData>()