From 5828b413b0720cdcb9d51b5ebb90cbf7d9eb7249 Mon Sep 17 00:00:00 2001 From: Riddle Hsu Date: Mon, 29 Apr 2019 19:22:38 +0800 Subject: [PATCH] Invoke setWindowStopped on the thread that created the view The windows belong to a activity will be set to stopped state when the activity is stopped. But the associated thread of view root could be different from activity, that results the inconsistent calling thread of surface callback, and maybe some potential races. Since the surface could be destroyed during traversal, also add a check for callback notifySurfaceDestroyed to prevent duplicated calls if the surface is released. Bug: 130553315 Test: atest ViewRootSurfaceCallbackTest Change-Id: Iea2cf8ff51118c0fa8c00a35c1d3e53c959ea754 --- core/java/android/view/ViewRootImpl.java | 3 +- .../android/view/WindowManagerGlobal.java | 20 +++- .../view/ViewRootSurfaceCallbackTest.java | 105 ++++++++++++++++++ 3 files changed, 126 insertions(+), 2 deletions(-) create mode 100644 core/tests/coretests/src/android/view/ViewRootSurfaceCallbackTest.java diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java index e8356752f8077..e9c856fde362b 100644 --- a/core/java/android/view/ViewRootImpl.java +++ b/core/java/android/view/ViewRootImpl.java @@ -1522,6 +1522,7 @@ public final class ViewRootImpl implements ViewParent, } void setWindowStopped(boolean stopped) { + checkThread(); if (mStopped != stopped) { mStopped = stopped; final ThreadedRenderer renderer = mAttachInfo.mThreadedRenderer; @@ -1543,7 +1544,7 @@ public final class ViewRootImpl implements ViewParent, } if (mStopped) { - if (mSurfaceHolder != null) { + if (mSurfaceHolder != null && mSurface.isValid()) { notifySurfaceDestroyed(); } destroySurface(); diff --git a/core/java/android/view/WindowManagerGlobal.java b/core/java/android/view/WindowManagerGlobal.java index 8a111cf866583..379acbecb6133 100644 --- a/core/java/android/view/WindowManagerGlobal.java +++ b/core/java/android/view/WindowManagerGlobal.java @@ -636,13 +636,21 @@ public final class WindowManagerGlobal { } public void setStoppedState(IBinder token, boolean stopped) { + ArrayList nonCurrentThreadRoots = null; synchronized (mLock) { int count = mViews.size(); for (int i = count - 1; i >= 0; i--) { if (token == null || mParams.get(i).token == token) { ViewRootImpl root = mRoots.get(i); // Client might remove the view by "stopped" event. - root.setWindowStopped(stopped); + if (root.mThread == Thread.currentThread()) { + root.setWindowStopped(stopped); + } else { + if (nonCurrentThreadRoots == null) { + nonCurrentThreadRoots = new ArrayList<>(); + } + nonCurrentThreadRoots.add(root); + } // Recursively forward stopped state to View's attached // to this Window rather than the root application token, // e.g. PopupWindow's. @@ -650,6 +658,16 @@ public final class WindowManagerGlobal { } } } + + // Update the stopped state synchronously to ensure the surface won't be used after server + // side has destroyed it. This operation should be outside the lock to avoid any potential + // paths from setWindowStopped to WindowManagerGlobal which may cause deadlocks. + if (nonCurrentThreadRoots != null) { + for (int i = nonCurrentThreadRoots.size() - 1; i >= 0; i--) { + ViewRootImpl root = nonCurrentThreadRoots.get(i); + root.mHandler.runWithScissors(() -> root.setWindowStopped(stopped), 0); + } + } } public void reportNewConfiguration(Configuration config) { diff --git a/core/tests/coretests/src/android/view/ViewRootSurfaceCallbackTest.java b/core/tests/coretests/src/android/view/ViewRootSurfaceCallbackTest.java new file mode 100644 index 0000000000000..94917cf540e89 --- /dev/null +++ b/core/tests/coretests/src/android/view/ViewRootSurfaceCallbackTest.java @@ -0,0 +1,105 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.view; + +import static android.os.Process.myTid; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; + +import android.app.Activity; +import android.app.AlertDialog; +import android.os.HandlerThread; + +import androidx.test.filters.SmallTest; +import androidx.test.platform.app.InstrumentationRegistry; +import androidx.test.rule.ActivityTestRule; + +import org.junit.Rule; +import org.junit.Test; + +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; + +@SmallTest +public class ViewRootSurfaceCallbackTest implements SurfaceHolder.Callback2 { + private static final String TAG = ViewRootSurfaceCallbackTest.class.getSimpleName(); + private static final long TIMEOUT_MS = TimeUnit.SECONDS.toMillis(3); + + @Rule + public ActivityTestRule mActivityRule = new ActivityTestRule<>(Activity.class); + + private final CompletableFuture mTidOfSurfaceCreated = new CompletableFuture<>(); + private final CompletableFuture mTidOfSurfaceDestroyed = new CompletableFuture<>(); + + private boolean mDuplicatedSurfaceDestroyed; + + /** + * Verifies that the calling thread of {@link SurfaceHolder.Callback2} should be the same as the + * thread that created the view root. + */ + @Test + public void testCallingTidOfSurfaceCallback() throws Exception { + final Activity activity = mActivityRule.getActivity(); + activity.setTurnScreenOn(true); + activity.setShowWhenLocked(true); + + // Create a dialog that runs on another thread and let it handle surface by itself. + final HandlerThread thread = new HandlerThread(TAG); + thread.start(); + thread.getThreadHandler().runWithScissors(() -> { + final AlertDialog dialog = new AlertDialog.Builder(activity) + .setTitle(TAG).setMessage(TAG).create(); + dialog.getWindow().takeSurface(this); + dialog.show(); + }, TIMEOUT_MS); + final int attachedTid = thread.getThreadId(); + + assertEquals(attachedTid, + mTidOfSurfaceCreated.get(TIMEOUT_MS, TimeUnit.MILLISECONDS).intValue()); + + // Make the activity invisible. + activity.moveTaskToBack(true /* nonRoot */); + InstrumentationRegistry.getInstrumentation().waitForIdleSync(); + + assertEquals(attachedTid, + mTidOfSurfaceDestroyed.get(TIMEOUT_MS, TimeUnit.MILLISECONDS).intValue()); + assertFalse("surfaceDestroyed should not be called twice", mDuplicatedSurfaceDestroyed); + } + + @Override + public void surfaceCreated(SurfaceHolder holder) { + mTidOfSurfaceCreated.complete(myTid()); + } + + @Override + public void surfaceDestroyed(SurfaceHolder holder) { + if (!mTidOfSurfaceDestroyed.isDone()) { + mTidOfSurfaceDestroyed.complete(myTid()); + } else { + mDuplicatedSurfaceDestroyed = true; + } + } + + @Override + public void surfaceChanged(SurfaceHolder holder, int format, int width, int height) { + } + + @Override + public void surfaceRedrawNeeded(SurfaceHolder holder) { + } +}