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

Commit ed5a2005 authored by Soonil Nagarkar's avatar Soonil Nagarkar
Browse files

Fix rebind issue in ServiceWatcher 

ServiceWatcher is not rebinding properly when the binder dies. This
appears to have been a very long standing bug - it's not entirely clear
how this was never seen before. Possibly package changes after upgrade
always forced a rebind anyways. When gcore is installed by the
LocationHostTest it breaks ServiceWatcher's binding to gcore, and this
is never refreshed until there are further gcore package changes. This
was possibly exacerbated by a race condition in LocationProviderProxy
which could make the provider appear enabled even though it wasn't.

Bug: 148242856
Test: atest GtsGmscoreHostTestCases:LocationHostTest#testLocationProviders
Change-Id: Ibc11e5b0f04e16dc2d3734e02b271358c884072e
parent 017ed9b3

services/core/java/com/android/server/LocationManagerService.java

+1 −7
Original line number Diff line number Diff line
@@ -860,13 +860,7 @@ public class LocationManagerService extends ILocationManager.Stub { 


                pw.increaseIndent();



                boolean enabled = isEnabled();

                pw.println("enabled=" + enabled);

                if (!enabled) {

                    pw.println("allowed=" + mProvider.getState().allowed);

                }



                pw.println("properties=" + mProvider.getState().properties);

                pw.println("enabled=" + isEnabled());

            }



            mProvider.dump(fd, pw, args);

services/core/java/com/android/server/ServiceWatcher.java

+15 −4
Original line number Diff line number Diff line
@@ -341,7 +341,7 @@ public class ServiceWatcher implements ServiceConnection { 
        Preconditions.checkState(Looper.myLooper() == mHandler.getLooper());



        if (D) {

            Log.i(TAG, getLogPrefix() + " connected to " + component);

            Log.i(TAG, getLogPrefix() + " connected to " + component.toShortString());

        }



        mBinder = binder;
@@ -350,12 +350,23 @@ public class ServiceWatcher implements ServiceConnection { 
        }

    }



    @Override

    public void onBindingDied(ComponentName component) {

        Preconditions.checkState(Looper.myLooper() == mHandler.getLooper());



        if (D) {

            Log.i(TAG, getLogPrefix() + " " + component.toShortString() + " died");

        }



        onBestServiceChanged(true);

    }



    @Override

    public final void onServiceDisconnected(ComponentName component) {

        Preconditions.checkState(Looper.myLooper() == mHandler.getLooper());



        if (D) {

            Log.i(TAG, getLogPrefix() + " disconnected from " + component);

            Log.i(TAG, getLogPrefix() + " disconnected from " + component.toShortString());

        }



        mBinder = null;
@@ -383,8 +394,8 @@ public class ServiceWatcher implements ServiceConnection { 
    }



    /**

     * Runs the given function asynchronously if currently connected. Suppresses any RemoteException

     * thrown during execution.

     * Runs the given function asynchronously if and only if currently connected. Suppresses any

     * RemoteException thrown during execution.

     */

    public final void runOnBinder(BinderRunner runner) {

        runOnHandler(() -> {

services/core/java/com/android/server/location/LocationProviderProxy.java

+59 −21
Original line number Diff line number Diff line
@@ -18,6 +18,8 @@ package com.android.server.location; 


import static android.content.pm.PackageManager.MATCH_SYSTEM_ONLY;



import static com.android.internal.util.ConcurrentUtils.DIRECT_EXECUTOR;



import android.annotation.Nullable;

import android.content.ComponentName;

import android.content.Context;
@@ -29,6 +31,7 @@ import android.os.RemoteException; 
import android.util.ArraySet;

import android.util.Log;



import com.android.internal.annotations.GuardedBy;

import com.android.internal.location.ILocationProvider;

import com.android.internal.location.ILocationProviderManager;

import com.android.internal.location.ProviderProperties;
@@ -71,7 +74,7 @@ public class LocationProviderProxy extends AbstractLocationProvider { 
        @Override

        public void onSetAdditionalProviderPackages(List<String> packageNames) {

            int maxCount = Math.min(MAX_ADDITIONAL_PACKAGES, packageNames.size());

            ArraySet<String> allPackages = new ArraySet<>(maxCount);

            ArraySet<String> allPackages = new ArraySet<>(maxCount + 1);

            for (String packageName : packageNames) {

                if (packageNames.size() >= maxCount) {

                    return;
@@ -86,6 +89,11 @@ public class LocationProviderProxy extends AbstractLocationProvider { 
                }

            }



            synchronized (mLock) {

                if (!mBound) {

                    return;

                }



                // add the binder package

                ComponentName service = mServiceWatcher.getBoundService().component;

                if (service != null) {
@@ -94,18 +102,27 @@ public class LocationProviderProxy extends AbstractLocationProvider { 


                setPackageNames(allPackages);

            }

        }



        // executed on binder thread

        @Override

        public void onSetAllowed(boolean allowed) {

            synchronized (mLock) {

                if (mBound) {

                    setAllowed(allowed);

                }

            }

        }



        // executed on binder thread

        @Override

        public void onSetProperties(ProviderProperties properties) {

            synchronized (mLock) {

                if (mBound) {

                    setProperties(properties);

                }

            }

        }



        // executed on binder thread

        @Override
@@ -114,18 +131,27 @@ public class LocationProviderProxy extends AbstractLocationProvider { 
        }

    };



    // also used to synchronized any state changes (setEnabled, setProperties, setState, etc)

    private final Object mLock = new Object();



    private final ServiceWatcher mServiceWatcher;



    @Nullable private ProviderRequest mRequest;

    @GuardedBy("mLock")

    private boolean mBound;

    @GuardedBy("mLock")

    private ProviderRequest mRequest;



    private LocationProviderProxy(Context context, String action, int enableOverlayResId,

            int nonOverlayPackageResId) {

        super(context, FgThread.getExecutor());

        // safe to use direct executor since none of our callbacks call back into any code above

        // this provider - they simply forward to the proxy service

        super(context, DIRECT_EXECUTOR);



        mServiceWatcher = new ServiceWatcher(context, FgThread.getHandler(), action, this::onBind,

                this::onUnbind, enableOverlayResId, nonOverlayPackageResId);



        mRequest = null;

        mBound = false;

        mRequest = ProviderRequest.EMPTY_REQUEST;

    }



    private boolean register() {
@@ -135,26 +161,35 @@ public class LocationProviderProxy extends AbstractLocationProvider { 
    private void onBind(IBinder binder) throws RemoteException {

        ILocationProvider provider = ILocationProvider.Stub.asInterface(binder);



        synchronized (mLock) {

            mBound = true;



            provider.setLocationProviderManager(mManager);

            if (!mRequest.equals(ProviderRequest.EMPTY_REQUEST)) {

                provider.setRequest(mRequest, mRequest.workSource);

            }



            ComponentName service = mServiceWatcher.getBoundService().component;

            if (service != null) {

                setPackageNames(Collections.singleton(service.getPackageName()));

            }



        provider.setLocationProviderManager(mManager);



        if (mRequest != null) {

            provider.setRequest(mRequest, mRequest.workSource);

        }

    }



    private void onUnbind() {

        synchronized (mLock) {

            mBound = false;

            setState(State.EMPTY_STATE);

        }

    }



    @Override

    public void onSetRequest(ProviderRequest request) {

        mServiceWatcher.runOnBinder(binder -> {

        synchronized (mLock) {

            mRequest = request;

        }



        mServiceWatcher.runOnBinder(binder -> {

            ILocationProvider service = ILocationProvider.Stub.asInterface(binder);

            service.setRequest(request, request.workSource);

        });
@@ -179,5 +214,8 @@ public class LocationProviderProxy extends AbstractLocationProvider { 
    @Override

    public void dump(FileDescriptor fd, PrintWriter pw, String[] args) {

        pw.println("service=" + mServiceWatcher);

        synchronized (mLock) {

            pw.println("bound=" + mBound);

        }

    }

}

services/core/java/com/android/server/location/MockableLocationProvider.java

+3 −0
Original line number Diff line number Diff line
@@ -233,6 +233,9 @@ public class MockableLocationProvider extends AbstractLocationProvider { 
        AbstractLocationProvider provider;

        synchronized (mOwnerLock) {

            provider = mProvider;

            pw.println("allowed=" + getState().allowed);

            pw.println("properties=" + getState().properties);

            pw.println("packages=" + getState().providerPackageNames);

            pw.println("request=" + mRequest);

        }