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

Commit 4ec0db91 authored by Kevin Chyn's avatar Kevin Chyn
Browse files

Identify and fix potential thread safety issues in SplitController 

1) Went through SplitController call paths
2) Some methods require locks and were not annotated. Checked the call
   paths for these methods, ensured callers had acquired the lock
   somewhere up the chain, and added GuardedBy to these methods
3) Fixed unsafe area(s):
    A) onStartActivityResult accesses #getContainer, which iterates
       through mTaskContainers, which requires a lock. Updated the
       onStartActivityResult block to use a lock

Bug: 242647030
Test: make androidx.window.extensions RUN_ERROR_PRONE=true
      check warnings
Test: Smoke test with Jetpack WindowManager Demos apk

Change-Id: I1099cf76f88e49c706b65e394afbb82dd4a1bb99
parent d8aadbd7

libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/SplitController.java

+34 −11
Original line number Diff line number Diff line
@@ -108,6 +108,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
    // Currently applied split configuration.

    @GuardedBy("mLock")

    private final List<EmbeddingRule> mSplitRules = new ArrayList<>();



    /**

     * A developer-defined {@link SplitAttributes} calculator to compute the current

     * {@link SplitAttributes} with the current device and window states.
@@ -125,6 +126,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
    @GuardedBy("mLock")

    @Nullable

    private SplitAttributesCalculator mSplitAttributesCalculator;



    /**

     * Map from Task id to {@link TaskContainer} which contains all TaskFragment and split pair info

     * below it.
@@ -230,6 +232,8 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
    }



    @NonNull

    @GuardedBy("mLock")

    @VisibleForTesting

    List<EmbeddingRule> getSplitRules() {

        return mSplitRules;

    }
@@ -246,7 +250,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
    }



    /**

     * Clears the listener set in {@link SplitController#setSplitInfoListener}.

     * Clears the listener set in {@link SplitController#setSplitInfoCallback(Consumer)}.

     */

    @Override

    public void clearSplitInfoCallback() {
@@ -466,6 +470,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
        updateContainersInTask(wct, taskContainer);

    }



    @GuardedBy("mLock")

    void updateContainersInTaskIfVisible(@NonNull WindowContainerTransaction wct, int taskId) {

        final TaskContainer taskContainer = getTaskContainer(taskId);

        if (taskContainer != null && taskContainer.isVisible()) {
@@ -473,6 +478,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
        }

    }



    @GuardedBy("mLock")

    private void updateContainersInTask(@NonNull WindowContainerTransaction wct,

            @NonNull TaskContainer taskContainer) {

        // Update all TaskFragments in the Task. Make a copy of the list since some may be
@@ -756,6 +762,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
    /**

     * Places the given activity to the top most TaskFragment in the task if there is any.

     */

    @GuardedBy("mLock")

    @VisibleForTesting

    void placeActivityInTopContainer(@NonNull WindowContainerTransaction wct,

            @NonNull Activity activity) {
@@ -879,6 +886,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
    /** Finds the activity below the given activity. */

    @VisibleForTesting

    @Nullable

    @GuardedBy("mLock")

    Activity findActivityBelow(@NonNull Activity activity) {

        Activity activityBelow = null;

        final TaskFragmentContainer container = getContainerWithActivity(activity);
@@ -1213,6 +1221,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
        return null;

    }



    @GuardedBy("mLock")

    TaskFragmentContainer newContainer(@NonNull Activity pendingAppearedActivity, int taskId) {

        return newContainer(pendingAppearedActivity, pendingAppearedActivity, taskId);

    }
@@ -1350,7 +1359,12 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
     * Removes a secondary container for the given primary container if an existing split is

     * already registered.

     */

    void removeExistingSecondaryContainers(@NonNull WindowContainerTransaction wct,

    // Suppress GuardedBy warning because lint asks to mark this method as

    // @GuardedBy(existingSplitContainer.getSecondaryContainer().mController.mLock), which is mLock

    // itself

    @SuppressWarnings("GuardedBy")

    @GuardedBy("mLock")

    private void removeExistingSecondaryContainers(@NonNull WindowContainerTransaction wct,

            @NonNull TaskFragmentContainer primaryContainer) {

        // If the primary container was already in a split - remove the secondary container that

        // is now covered by the new one that replaced it.
@@ -1368,6 +1382,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
    /**

     * Returns the topmost not finished container in Task of given task id.

     */

    @GuardedBy("mLock")

    @Nullable

    TaskFragmentContainer getTopActiveContainer(int taskId) {

        final TaskContainer taskContainer = mTaskContainers.get(taskId);
@@ -1737,6 +1752,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
    }



    @Nullable

    @GuardedBy("mLock")

    TaskFragmentContainer getContainer(@NonNull IBinder fragmentToken) {

        for (int i = mTaskContainers.size() - 1; i >= 0; i--) {

            final List<TaskFragmentContainer> containers = mTaskContainers.valueAt(i).mContainers;
@@ -1750,6 +1766,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
    }



    @Nullable

    @GuardedBy("mLock")

    TaskContainer getTaskContainer(int taskId) {

        return mTaskContainers.get(taskId);

    }
@@ -1758,6 +1775,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
        return mHandler;

    }



    @GuardedBy("mLock")

    int getTaskId(@NonNull Activity activity) {

        // Prefer to get the taskId from TaskFragmentContainer because Activity.getTaskId() is an

        // IPC call.
@@ -1850,6 +1868,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
    /**

     * @see #shouldRetainAssociatedContainer(TaskFragmentContainer, TaskFragmentContainer)

     */

    @GuardedBy("mLock")

    boolean shouldRetainAssociatedActivity(@NonNull TaskFragmentContainer finishingContainer,

            @NonNull Activity associatedActivity) {

        final TaskFragmentContainer associatedContainer = getContainerWithActivity(
@@ -1970,6 +1989,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
    @VisibleForTesting

    class ActivityStartMonitor extends Instrumentation.ActivityMonitor {

        @VisibleForTesting

        @GuardedBy("mLock")

        Intent mCurrentIntent;



        @Override
@@ -2034,8 +2054,10 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
        @Override

        public void onStartActivityResult(int result, @NonNull Bundle bOptions) {

            super.onStartActivityResult(result, bOptions);

            synchronized (mLock) {

                if (mCurrentIntent != null && result != START_SUCCESS) {

                // Clear the pending appeared intent if the activity was not started successfully.

                    // Clear the pending appeared intent if the activity was not started

                    // successfully.

                    final IBinder token = bOptions.getBinder(

                            ActivityOptions.KEY_LAUNCH_TASK_FRAGMENT_TOKEN);

                    if (token != null) {
@@ -2048,6 +2070,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen 
                mCurrentIntent = null;

            }

        }

    }



    /**

     * Checks if an activity is embedded and its presentation is customized by a

libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/SplitPresenter.java

+7 −11
Original line number Diff line number Diff line
@@ -39,7 +39,6 @@ import android.view.WindowMetrics; 
import android.window.TaskFragmentCreationParams;

import android.window.WindowContainerTransaction;



import androidx.annotation.GuardedBy;

import androidx.annotation.IntDef;

import androidx.annotation.NonNull;

import androidx.annotation.Nullable;
@@ -63,7 +62,10 @@ import java.util.concurrent.Executor; 
/**

 * Controls the visual presentation of the splits according to the containers formed by

 * {@link SplitController}.

 *

 * Note that all calls into this class must hold the {@link SplitController} internal lock.

 */

@SuppressWarnings("GuardedBy")

class SplitPresenter extends JetpackTaskFragmentOrganizer {

    @VisibleForTesting

    static final int POSITION_START = 0;
@@ -163,7 +165,6 @@ class SplitPresenter extends JetpackTaskFragmentOrganizer { 
     * @return The newly created secondary container.

     */

    @NonNull

    @GuardedBy("mController.mLock")

    TaskFragmentContainer createNewSplitWithEmptySideContainer(

            @NonNull WindowContainerTransaction wct, @NonNull Activity primaryActivity,

            @NonNull Intent secondaryIntent, @NonNull SplitPairRule rule) {
@@ -210,7 +211,6 @@ class SplitPresenter extends JetpackTaskFragmentOrganizer { 
     *                          created and the activity will be re-parented to it.

     * @param rule The split rule to be applied to the container.

     */

    @GuardedBy("mController.mLock")

    void createNewSplitContainer(@NonNull WindowContainerTransaction wct,

            @NonNull Activity primaryActivity, @NonNull Activity secondaryActivity,

            @NonNull SplitPairRule rule) {
@@ -285,7 +285,6 @@ class SplitPresenter extends JetpackTaskFragmentOrganizer { 
     * @param rule              The split rule to be applied to the container.

     * @param isPlaceholder     Whether the launch is a placeholder.

     */

    @GuardedBy("mController.mLock")

    void startActivityToSide(@NonNull WindowContainerTransaction wct,

            @NonNull Activity launchingActivity, @NonNull Intent activityIntent,

            @Nullable Bundle activityOptions, @NonNull SplitRule rule,
@@ -328,7 +327,6 @@ class SplitPresenter extends JetpackTaskFragmentOrganizer { 
     * @param updatedContainer The task fragment that was updated and caused this split update.

     * @param wct WindowContainerTransaction that this update should be performed with.

     */

    @GuardedBy("mController.mLock")

    void updateSplitContainer(@NonNull SplitContainer splitContainer,

            @NonNull TaskFragmentContainer updatedContainer,

            @NonNull WindowContainerTransaction wct) {
@@ -369,7 +367,6 @@ class SplitPresenter extends JetpackTaskFragmentOrganizer { 
        updateTaskFragmentWindowingModeIfRegistered(wct, secondaryContainer, windowingMode);

    }



    @GuardedBy("mController.mLock")

    private void setAdjacentTaskFragments(@NonNull WindowContainerTransaction wct,

            @NonNull TaskFragmentContainer primaryContainer,

            @NonNull TaskFragmentContainer secondaryContainer, @NonNull SplitRule splitRule,
@@ -393,7 +390,7 @@ class SplitPresenter extends JetpackTaskFragmentOrganizer { 
     * creation has not been reported from the server yet.

     */

    // TODO(b/190433398): Handle resize if the fragment hasn't appeared yet.

    void resizeTaskFragmentIfRegistered(@NonNull WindowContainerTransaction wct,

    private void resizeTaskFragmentIfRegistered(@NonNull WindowContainerTransaction wct,

            @NonNull TaskFragmentContainer container,

            @Nullable Rect bounds) {

        if (container.getInfo() == null) {
@@ -520,7 +517,6 @@ class SplitPresenter extends JetpackTaskFragmentOrganizer { 
        return !(splitAttributes.getSplitType() instanceof ExpandContainersSplitType);

    }



    @GuardedBy("mController.mLock")

    @NonNull

    SplitAttributes computeSplitAttributes(@NonNull TaskProperties taskProperties,

            @NonNull SplitRule rule, @Nullable Pair<Size, Size> minDimensionsPair) {
@@ -572,8 +568,8 @@ class SplitPresenter extends JetpackTaskFragmentOrganizer { 
    }



    @NonNull

    static Pair<Size, Size> getActivitiesMinDimensionsPair(@NonNull Activity primaryActivity,

            @NonNull Activity secondaryActivity) {

    private static Pair<Size, Size> getActivitiesMinDimensionsPair(

            @NonNull Activity primaryActivity, @NonNull Activity secondaryActivity) {

        return new Pair<>(getMinDimensions(primaryActivity), getMinDimensions(secondaryActivity));

    }
@@ -619,7 +615,7 @@ class SplitPresenter extends JetpackTaskFragmentOrganizer { 
        return new Size(windowLayout.minWidth, windowLayout.minHeight);

    }



    static boolean boundsSmallerThanMinDimensions(@NonNull Rect bounds,

    private static boolean boundsSmallerThanMinDimensions(@NonNull Rect bounds,

            @Nullable Size minDimensions) {

        if (minDimensions == null) {

            return false;

libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/TaskFragmentContainer.java

+3 −0
Original line number Diff line number Diff line
@@ -253,6 +253,7 @@ class TaskFragmentContainer { 
        mPendingAppearedActivities.remove(activityToken);

    }



    @GuardedBy("mController.mLock")

    void clearPendingAppearedActivities() {

        final List<IBinder> cleanupActivities = new ArrayList<>(mPendingAppearedActivities);

        // Clear mPendingAppearedActivities so that #getContainerWithActivity won't return the
@@ -452,6 +453,7 @@ class TaskFragmentContainer { 
     * Removes all activities that belong to this process and finishes other containers/activities

     * configured to finish together.

     */

    @GuardedBy("mController.mLock")

    void finish(boolean shouldFinishDependent, @NonNull SplitPresenter presenter,

            @NonNull WindowContainerTransaction wct, @NonNull SplitController controller) {

        if (!mIsFinished) {
@@ -476,6 +478,7 @@ class TaskFragmentContainer { 
        mInfo = null;

    }



    @GuardedBy("mController.mLock")

    private void finishActivities(boolean shouldFinishDependent, @NonNull SplitPresenter presenter,

            @NonNull WindowContainerTransaction wct, @NonNull SplitController controller) {

        // Finish own activities