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

Commit 60a494ae authored by Riddle Hsu's avatar Riddle Hsu
Browse files

Make the disconnecting list from activity up-to-date 

After the holder has enqueued the runnable to disconnect, the
connections may be removed by other paths, e.g.
 ActivityThread#handleDestroyActivity
  -> ContextImpl#scheduleFinalCleanup
  -> LoadedApk#removeContextRegistrations
  -> unbindService
Then when executing the runnable, it will remove the removed
connection again and try to bind down the service. If a new service
instance has been started again, it will cause IllegalStateException.

This change keeps the reference of connections, so removeConnection
from ActiveServices still updates the exact list. The original
double-disconnect is protected by null out the holder and a new
flag to indicate disconnecting.

Bug: 146825978
Test: atest CtsAppTestCases:ServiceTest
Test: 1. Add sleep 5s in the disconnect-runnable to delay calling
         ActivityManagerInternal#disconnectActivityFromServices.
      2. Launch an activity which binds a service.
      3. Finish the activity without unbind (so the
         disconnect-runnable is enqueued and unbindService from
         final-cleanup will be called. If DEBUG_CLEANUP is enabled,
         the log in removeConnection should appear).
      4. After the activity is destroyed for 2s, start the service
         again (previous service is destroyed by unbind, so a new
         service record is created).
      5. The 5s delayed disconnect-runnable executes, and system
         is still alive without any exception logs (originally
         it will always crash the system).

Change-Id: I3ae85927da1c11b2560524b6642401741b5beae2
parent 749e585a

core/java/android/app/ActivityManagerInternal.java

+1 −1
Original line number Original line Diff line number Diff line
@@ -278,7 +278,7 @@ public abstract class ActivityManagerInternal { 
            String resolvedType, boolean fgRequired, String callingPackage, @UserIdInt int userId,

            String resolvedType, boolean fgRequired, String callingPackage, @UserIdInt int userId,

            boolean allowBackgroundActivityStarts) throws TransactionTooLargeException;

            boolean allowBackgroundActivityStarts) throws TransactionTooLargeException;





    public abstract void disconnectActivityFromServices(Object connectionHolder, Object conns);

    public abstract void disconnectActivityFromServices(Object connectionHolder);

    public abstract void cleanUpServices(@UserIdInt int userId, ComponentName component,

    public abstract void cleanUpServices(@UserIdInt int userId, ComponentName component,

            Intent baseIntent);

            Intent baseIntent);

    public abstract ActivityInfo getActivityInfoForUser(ActivityInfo aInfo, @UserIdInt int userId);

    public abstract ActivityInfo getActivityInfoForUser(ActivityInfo aInfo, @UserIdInt int userId);

services/core/java/com/android/server/am/ActivityManagerService.java

+5 −8
Original line number Original line Diff line number Diff line
@@ -18924,19 +18924,16 @@ public class ActivityManagerService extends IActivityManager.Stub 
        }

        }

















        // The arguments here are untyped because the base ActivityManagerInternal class







        // The arguments here are untyped because the base ActivityManagerInternal class











        // doesn't have compile-time visiblity into ActivityServiceConnectionHolder or







        // doesn't have compile-time visibility into ActivityServiceConnectionHolder or











        // ConnectionRecord.







        // ConnectionRecord.











        @Override







        @Override











        public void disconnectActivityFromServices(Object connectionHolder, Object conns) {







        public void disconnectActivityFromServices(Object connectionHolder) {











            // 'connectionHolder' is an untyped ActivityServiceConnectionsHolder







            // 'connectionHolder' is an untyped ActivityServiceConnectionsHolder











            // 'conns' is an untyped HashSet<ConnectionRecord>














            final ActivityServiceConnectionsHolder holder =







            final ActivityServiceConnectionsHolder holder =











                    (ActivityServiceConnectionsHolder) connectionHolder;







                    (ActivityServiceConnectionsHolder) connectionHolder;











            final HashSet<ConnectionRecord> toDisconnect = (HashSet<ConnectionRecord>) conns;














            synchronized (ActivityManagerService.this) {







            synchronized (ActivityManagerService.this) {











                for (ConnectionRecord cr : toDisconnect) {







                holder.forEachConnection(cr -> mServices.removeConnectionLocked(











                    mServices.removeConnectionLocked(cr, null, holder);







                        (ConnectionRecord) cr, null /* skipApp */, holder /* skipAct */));











                }














            }







            }











        }







        }









































services/core/java/com/android/server/wm/ActivityRecord.java



+2
−0




























Original line number
Original line
Diff line number
Diff line








@@ -3034,6 +3034,8 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A









        }







        }











        // Throw away any services that have been bound by this activity.







        // Throw away any services that have been bound by this activity.











        mServiceConnectionsHolder.disconnectActivityFromServices();







        mServiceConnectionsHolder.disconnectActivityFromServices();














        // This activity record is removing, make sure not to disconnect twice.














        mServiceConnectionsHolder = null;











    }







    }































    @Override







    @Override

































services/core/java/com/android/server/wm/ActivityServiceConnectionsHolder.java



+45
−14




























Original line number
Original line
Diff line number
Diff line








@@ -18,10 +18,13 @@ package com.android.server.wm;





























import static com.android.server.wm.ActivityStack.ActivityState.PAUSING;







import static com.android.server.wm.ActivityStack.ActivityState.PAUSING;











import static com.android.server.wm.ActivityStack.ActivityState.RESUMED;







import static com.android.server.wm.ActivityStack.ActivityState.RESUMED;














import static com.android.server.wm.ActivityTaskManagerDebugConfig.DEBUG_CLEANUP;














import static com.android.server.wm.ActivityTaskManagerDebugConfig.TAG_ATM;





























import android.util.ArraySet;














import android.util.Slog;































import java.io.PrintWriter;







import java.io.PrintWriter;











import java.util.HashSet;














import java.util.Iterator;














import java.util.function.Consumer;







import java.util.function.Consumer;































/**







/**









@@ -30,6 +33,8 @@ import java.util.function.Consumer;









 * instance of this per activity for tracking all services connected to that activity. AM will







 * instance of this per activity for tracking all services connected to that activity. AM will











 * sometimes query this to bump the OOM score for the processes with services connected to visible







 * sometimes query this to bump the OOM score for the processes with services connected to visible











 * activities.







 * activities.














 * <p>














 * Public methods are called in AM lock, otherwise in WM lock.











 */







 */











public class ActivityServiceConnectionsHolder<T> {







public class ActivityServiceConnectionsHolder<T> {





























@@ -44,7 +49,10 @@ public class ActivityServiceConnectionsHolder<T> {









     * on the WM side since we don't perform operations on the object. Mainly here for communication







     * on the WM side since we don't perform operations on the object. Mainly here for communication











     * and booking with the AM side.







     * and booking with the AM side.











     */







     */











    private HashSet<T> mConnections;







    private ArraySet<T> mConnections;





























    /** Whether all connections of {@link #mActivity} are being removed. */














    private volatile boolean mIsDisconnecting;































    ActivityServiceConnectionsHolder(ActivityTaskManagerService service, ActivityRecord activity) {







    ActivityServiceConnectionsHolder(ActivityTaskManagerService service, ActivityRecord activity) {











        mService = service;







        mService = service;









@@ -54,8 +62,16 @@ public class ActivityServiceConnectionsHolder<T> {









    /** Adds a connection record that the activity has bound to a specific service. */







    /** Adds a connection record that the activity has bound to a specific service. */











    public void addConnection(T c) {







    public void addConnection(T c) {











        synchronized (mService.mGlobalLock) {







        synchronized (mService.mGlobalLock) {














            if (mIsDisconnecting) {














                // This is unlikely to happen because the caller should create a new holder.














                if (DEBUG_CLEANUP) {














                    Slog.e(TAG_ATM, "Skip adding connection " + c + " to a disconnecting holder of "














                            + mActivity);














                }














                return;














            }











            if (mConnections == null) {







            if (mConnections == null) {











                mConnections = new HashSet<>();







                mConnections = new ArraySet<>();











            }







            }











            mConnections.add(c);







            mConnections.add(c);











        }







        }









@@ -67,6 +83,9 @@ public class ActivityServiceConnectionsHolder<T> {









            if (mConnections == null) {







            if (mConnections == null) {











                return;







                return;











            }







            }














            if (DEBUG_CLEANUP && mIsDisconnecting) {














                Slog.v(TAG_ATM, "Remove pending disconnecting " + c + " of " + mActivity);














            }











            mConnections.remove(c);







            mConnections.remove(c);











        }







        }











    }







    }









@@ -88,26 +107,33 @@ public class ActivityServiceConnectionsHolder<T> {









            if (mConnections == null || mConnections.isEmpty()) {







            if (mConnections == null || mConnections.isEmpty()) {











                return;







                return;











            }







            }











            final Iterator<T> it = mConnections.iterator();







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











            while (it.hasNext()) {







                consumer.accept(mConnections.valueAt(i));











                T c = it.next();














                consumer.accept(c);














            }







            }











        }







        }











    }







    }































    /** Removes the connection between the activity and all services that were connected to it. */







    /**














     * Removes the connection between the activity and all services that were connected to it. In














     * general, this method is used to clean up if the activity didn't unbind services before it














     * is destroyed.














     */











    void disconnectActivityFromServices() {







    void disconnectActivityFromServices() {











        if (mConnections == null || mConnections.isEmpty()) {







        if (mConnections == null || mConnections.isEmpty() || mIsDisconnecting) {











            return;







            return;











        }







        }











        // Capture and null out mConnections, to guarantee that we process







        // Mark as disconnecting, to guarantee that we process











        // disconnect of these specific connections exactly once even if







        // disconnect of these specific connections exactly once even if











        // we're racing with rapid activity lifecycle churn and this







        // we're racing with rapid activity lifecycle churn and this











        // method is invoked more than once on this object.







        // method is invoked more than once on this object.











        final Object disc = mConnections;







        // It is possible that {@link #removeConnection} is called while the disconnect-runnable is











        mConnections = null;







        // still in the message queue, so keep the reference of {@link #mConnections} to make sure











        mService.mH.post(() -> mService.mAmInternal.disconnectActivityFromServices(this, disc));







        // the connection list is up-to-date.














        mIsDisconnecting = true;














        mService.mH.post(() -> {














            mService.mAmInternal.disconnectActivityFromServices(this);














            mIsDisconnecting = false;














        });











    }







    }































    public void dump(PrintWriter pw, String prefix) {







    public void dump(PrintWriter pw, String prefix) {









@@ -116,4 +142,9 @@ public class ActivityServiceConnectionsHolder<T> {









        }







        }











    }







    }


































    /** Used by {@link ActivityRecord#dump}. */














    @Override














    public String toString() {














        return String.valueOf(mConnections);














    }











}






}