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

Commit d217745d authored by Brad Ebinger's avatar Brad Ebinger
Browse files

ImsServiceController should properly unbind when remote crashes 

There is currently an issue where unbind() calls will be ignored
in ImsServiceController because we were mistakenly clearing the
ImsServiceConnection in the onServiceDisconnected method. When in
this state, the ImsServiceController is never properly unbound.

1) Unless unbindService() is explicitly called, the ImsServiceConnection
should not be cleared.

2) Fix some iffy locking issues.

Bug: 159047328
Test: atest FrameworksTelephonyTests CtsTelephonyTestCases:ImsServiceTest
Change-Id: I88d4a5aad637f76b77f056dd6b5a2b5cfd185c87
parent c2c7092a

src/java/com/android/internal/telephony/ims/ImsServiceController.java

+36 −37
Original line number Original line Diff line number Diff line
@@ -72,8 +72,8 @@ public class ImsServiceController { 




        @Override

        @Override

        public void onServiceConnected(ComponentName name, IBinder service) {

        public void onServiceConnected(ComponentName name, IBinder service) {

            mBackoff.stop();

            synchronized (mLock) {

            synchronized (mLock) {

                mBackoff.stop();

                mIsBound = true;

                mIsBound = true;

                mIsBinding = false;

                mIsBinding = false;

                try {

                try {
@@ -89,8 +89,10 @@ public class ImsServiceController { 
                } catch (RemoteException e) {

                } catch (RemoteException e) {

                    mIsBound = false;

                    mIsBound = false;

                    mIsBinding = false;

                    mIsBinding = false;

                    // Remote exception means that the binder already died.

                    // RemoteException means that the process holding the binder died or something

                    // unexpected happened... try a full rebind.

                    cleanupConnection();

                    cleanupConnection();

                    unbindService();

                    startDelayedRebindToService();

                    startDelayedRebindToService();

                    mLocalLog.log("onConnected exception=" + e.getMessage() + ", retry in "

                    mLocalLog.log("onConnected exception=" + e.getMessage() + ", retry in "

                            + mBackoff.getCurrentDelay() + " mS");

                            + mBackoff.getCurrentDelay() + " mS");
@@ -104,8 +106,8 @@ public class ImsServiceController { 
        public void onServiceDisconnected(ComponentName name) {

        public void onServiceDisconnected(ComponentName name) {

            synchronized (mLock) {

            synchronized (mLock) {

                mIsBinding = false;

                mIsBinding = false;

            }

                cleanupConnection();

                cleanupConnection();

            }

            mLocalLog.log("onServiceDisconnected");

            mLocalLog.log("onServiceDisconnected");

            Log.w(LOG_TAG, "ImsService(" + name + "): onServiceDisconnected. Waiting...");

            Log.w(LOG_TAG, "ImsService(" + name + "): onServiceDisconnected. Waiting...");

            // Service disconnected, but we are still technically bound. Waiting for reconnect.

            // Service disconnected, but we are still technically bound. Waiting for reconnect.
@@ -116,14 +118,12 @@ public class ImsServiceController { 
            synchronized (mLock) {

            synchronized (mLock) {

                mIsBinding = false;

                mIsBinding = false;

                mIsBound = false;

                mIsBound = false;

            }

            if (mImsServiceConnection != null) {

                // according to the docs, we should fully unbind before rebinding again.

                // according to the docs, we should fully unbind before rebinding again.

                mContext.unbindService(mImsServiceConnection);

            }

                cleanupConnection();

                cleanupConnection();

            Log.w(LOG_TAG, "ImsService(" + name + "): onBindingDied. Starting rebind...");

                unbindService();

                startDelayedRebindToService();

                startDelayedRebindToService();

            }

            Log.w(LOG_TAG, "ImsService(" + name + "): onBindingDied. Starting rebind...");

            mLocalLog.log("onBindingDied, retrying in " + mBackoff.getCurrentDelay() + " mS");

            mLocalLog.log("onBindingDied, retrying in " + mBackoff.getCurrentDelay() + " mS");

        }

        }
@@ -133,19 +133,23 @@ public class ImsServiceController { 
            mLocalLog.log("onNullBinding");

            mLocalLog.log("onNullBinding");

            synchronized (mLock) {

            synchronized (mLock) {

                mIsBinding = false;

                mIsBinding = false;

                mIsBound = false;

                // Service connection exists, so we are bound but the binder is null. Wait for

            }

                // ImsResolver to trigger the unbind here.

                mIsBound = true;

                cleanupConnection();

                cleanupConnection();

            }

            if (mCallbacks != null) {

            if (mCallbacks != null) {

                // Will trigger an unbind.

                // Will trigger an unbind.

                mCallbacks.imsServiceBindPermanentError(getComponentName());

                mCallbacks.imsServiceBindPermanentError(getComponentName());

            }

            }

        }

        }





        // Does not clear features, just removes all active features.

        // Does not clear feature configuration, just cleans up the active callbacks and

        // invalidates remote FeatureConnections.

        // This should only be called when locked

        private void cleanupConnection() {

        private void cleanupConnection() {

            cleanupAllFeatures();

            cleanupAllFeatures();

            cleanUpService();

            setServiceController(null);

        }

        }

    }

    }
@@ -427,18 +431,13 @@ public class ImsServiceController { 
    public void unbind() throws RemoteException {

    public void unbind() throws RemoteException {

        synchronized (mLock) {

        synchronized (mLock) {

            mBackoff.stop();

            mBackoff.stop();

            if (mImsServiceConnection == null) {

                return;

            }

            // Clean up all features

            // Clean up all features

            changeImsServiceFeatures(new HashSet<>());

            changeImsServiceFeatures(new HashSet<>());

            removeImsServiceFeatureCallbacks();

            removeImsServiceFeatureCallbacks();

            Log.i(LOG_TAG, "Unbinding ImsService: " + mComponentName);

            mLocalLog.log("unbinding");

            mContext.unbindService(mImsServiceConnection);

            mIsBound = false;

            mIsBound = false;

            mIsBinding = false;

            mIsBinding = false;

            cleanUpService();

            setServiceController(null);

            unbindService();

        }

        }

    }

    }
@@ -625,15 +624,6 @@ public class ImsServiceController { 
        mIImsServiceController = IImsServiceController.Stub.asInterface(serviceController);

        mIImsServiceController = IImsServiceController.Stub.asInterface(serviceController);

    }

    }





    /**

     * @return true if the controller is currently bound.

     */

    public boolean isBound() {

        synchronized (mLock) {

            return mIsBound;

        }

    }



    /**

    /**

     * Check to see if the service controller is available, overridden for compat versions,

     * Check to see if the service controller is available, overridden for compat versions,

     * @return true if available, false otherwise;

     * @return true if available, false otherwise;
@@ -652,6 +642,22 @@ public class ImsServiceController { 
        mBackoff.start();

        mBackoff.start();

    }

    }





    private void unbindService() {

        synchronized (mLock) {

            if (mImsServiceConnection != null) {

                Log.i(LOG_TAG, "Unbinding ImsService: " + mComponentName);

                mLocalLog.log("unbinding: " + mComponentName);

                mContext.unbindService(mImsServiceConnection);

                mImsServiceConnection = null;

            } else {

                Log.i(LOG_TAG, "unbindService called on already unbound ImsService: "

                        + mComponentName);

                mLocalLog.log("Note: unbindService called with no ServiceConnection on "

                        + mComponentName);

            }

        }

    }



    // Grant runtime permissions to ImsService. PermissionManager ensures that the ImsService is

    // Grant runtime permissions to ImsService. PermissionManager ensures that the ImsService is

    // system/signed before granting permissions.

    // system/signed before granting permissions.

    private void grantPermissionsToService() {

    private void grantPermissionsToService() {
@@ -778,8 +784,8 @@ public class ImsServiceController { 
            // Don't update ImsService for emergency MMTEL feature.

            // Don't update ImsService for emergency MMTEL feature.

            Log.i(LOG_TAG, "doesn't support emergency calling on slot " + featurePair.slotId);

            Log.i(LOG_TAG, "doesn't support emergency calling on slot " + featurePair.slotId);

        }

        }

        // Send callback to ImsServiceProxy to change supported ImsFeatures

        // Send callback to FeatureConnection to change supported ImsFeatures

        // Ensure that ImsServiceProxy callback occurs after ImsResolver callback. If an

        // Ensure that FeatureConnection callback occurs after ImsResolver callback. If an

        // ImsManager requests the ImsService while it is being removed in ImsResolver, this

        // ImsManager requests the ImsService while it is being removed in ImsResolver, this

        // callback will clean it up after.

        // callback will clean it up after.

        sendImsFeatureRemovedCallback(featurePair.slotId, featurePair.featureType);

        sendImsFeatureRemovedCallback(featurePair.slotId, featurePair.featureType);
@@ -840,13 +846,6 @@ public class ImsServiceController { 
        }

        }

    }

    }





    private void cleanUpService() {

        synchronized (mLock) {

            mImsServiceConnection = null;

            setServiceController(null);

        }

    }



    @Override

    @Override

    public String toString() {

    public String toString() {

        synchronized (mLock) {

        synchronized (mLock) {

tests/telephonytests/src/com/android/internal/telephony/ims/ImsServiceControllerTest.java

+39 −0
Original line number Original line Diff line number Diff line
@@ -281,6 +281,44 @@ public class ImsServiceControllerTest extends ImsTestBase { 
        verify(mMockProxyCallbacks).imsFeatureRemoved(eq(SLOT_0), eq(ImsFeature.FEATURE_RCS));

        verify(mMockProxyCallbacks).imsFeatureRemoved(eq(SLOT_0), eq(ImsFeature.FEATURE_RCS));

    }

    }





    /**

     * Tests that when unbind is called while the ImsService is disconnected, we still handle

     * unbinding to the service correctly.

     */

    @SmallTest

    @Test

    public void testBindServiceAndConnectedDisconnectedUnbind() throws RemoteException {

        HashSet<ImsFeatureConfiguration.FeatureSlotPair> testFeatures = new HashSet<>();

        testFeatures.add(new ImsFeatureConfiguration.FeatureSlotPair(SLOT_0,

                ImsFeature.FEATURE_MMTEL));

        testFeatures.add(new ImsFeatureConfiguration.FeatureSlotPair(SLOT_0,

                ImsFeature.FEATURE_RCS));

        ServiceConnection conn = bindAndConnectService(testFeatures);



        conn.onServiceDisconnected(mTestComponentName);



        verify(mMockCallbacks).imsServiceFeatureRemoved(eq(SLOT_0), eq(ImsFeature.FEATURE_MMTEL),

                eq(mTestImsServiceController));

        verify(mMockCallbacks).imsServiceFeatureRemoved(eq(SLOT_0), eq(ImsFeature.FEATURE_RCS),

                eq(mTestImsServiceController));

        verify(mMockProxyCallbacks).imsFeatureRemoved(eq(SLOT_0), eq(ImsFeature.FEATURE_MMTEL));

        verify(mMockProxyCallbacks).imsFeatureRemoved(eq(SLOT_0), eq(ImsFeature.FEATURE_RCS));



        mTestImsServiceController.unbind();

        verify(mMockContext).unbindService(eq(conn));



        // Even though we unbound, this was already sent after service disconnected, so we shouldn't

        // see it again

        verify(mMockCallbacks, times(1)).imsServiceFeatureRemoved(eq(SLOT_0),

                eq(ImsFeature.FEATURE_MMTEL), eq(mTestImsServiceController));

        verify(mMockCallbacks, times(1)).imsServiceFeatureRemoved(eq(SLOT_0),

                eq(ImsFeature.FEATURE_RCS), eq(mTestImsServiceController));

        verify(mMockProxyCallbacks, times(1)).imsFeatureRemoved(eq(SLOT_0),

                eq(ImsFeature.FEATURE_MMTEL));

        verify(mMockProxyCallbacks, times(1)).imsFeatureRemoved(eq(SLOT_0),

                eq(ImsFeature.FEATURE_RCS));

    }



    /**

    /**

     * Tests ImsServiceController callbacks are properly called when an ImsService is bound and

     * Tests ImsServiceController callbacks are properly called when an ImsService is bound and

     * subsequently unbound.

     * subsequently unbound.
@@ -325,6 +363,7 @@ public class ImsServiceControllerTest extends ImsTestBase { 




        conn.onBindingDied(null /*null*/);

        conn.onBindingDied(null /*null*/);





        verify(mMockContext).unbindService(eq(conn));

        verify(mMockCallbacks).imsServiceFeatureRemoved(eq(SLOT_0), eq(ImsFeature.FEATURE_MMTEL),

        verify(mMockCallbacks).imsServiceFeatureRemoved(eq(SLOT_0), eq(ImsFeature.FEATURE_MMTEL),

                eq(mTestImsServiceController));

                eq(mTestImsServiceController));

        verify(mMockCallbacks).imsServiceFeatureRemoved(eq(SLOT_0), eq(ImsFeature.FEATURE_RCS),

        verify(mMockCallbacks).imsServiceFeatureRemoved(eq(SLOT_0), eq(ImsFeature.FEATURE_RCS),