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

Commit d3ef4bf6 authored by Martijn Coenen's avatar Martijn Coenen
Browse files

Move BinderProxy serialization into Java. 

The BinderProxy class is not thread-safe, and all calls into
it were serialized by holding gProxyLock from JNI code. More
recently, we've been wanting to access BinderProxy from Java
code directly, and having the lock in native complicated things.

This change removes the lock in native code and adds it in the
Java layer. A benefit of this change is that it reduces the
scope of where a lock is held. On the flip side, we no longer
have a cached BinderProxyNativeData object lying around. This
means we now allocate/free a BinderProxyNativeData even if we
already have a Java object lying around for the native object,
which can happen quite frequently. But we deem the impact of
this to be acceptable.

Bug: 109888955
Test: sailfish builds, boots, proxy warnings still show
Change-Id: If2f4dbe5486ec7af0ef8ea42d24ac3a4330cc05a
parent ce774071

core/java/android/os/Binder.java

+21 −20
Original line number Diff line number Diff line
@@ -23,6 +23,7 @@ import android.util.Log; 
import android.util.Slog;

import android.util.SparseIntArray;



import com.android.internal.annotations.GuardedBy;

import com.android.internal.os.BinderCallsStats;

import com.android.internal.os.BinderInternal;

import com.android.internal.util.FastPrintWriter;
@@ -1027,19 +1028,19 @@ final class BinderProxy implements IBinder { 
                new ArrayList[MAIN_INDEX_SIZE];

    }



    private static ProxyMap sProxyMap = new ProxyMap();

    @GuardedBy("sProxyMap")

    private static final ProxyMap sProxyMap = new ProxyMap();



    /**

      * Dump proxy debug information.

      *

      * Note: this method is not thread-safe; callers must serialize with other

      * accesses to sProxyMap, in particular {@link #getInstance(long, long)}.

      *

      * @hide

      */

    private static void dumpProxyDebugInfo() {

        if (Build.IS_DEBUGGABLE) {

            synchronized (sProxyMap) {

                sProxyMap.dumpProxyInterfaceCounts();

            }

            // Note that we don't call dumpPerUidProxyCounts(); this is because this

            // method may be called as part of the uid limit being hit, and calling

            // back into the UID tracking code would cause us to try to acquire a mutex
@@ -1049,8 +1050,6 @@ final class BinderProxy implements IBinder { 


    /**

     * Return a BinderProxy for IBinder.

     * This method is thread-hostile!  The (native) caller serializes getInstance() calls using

     * gProxyLock.

     * If we previously returned a BinderProxy bp for the same iBinder, and bp is still

     * in use, then we return the same bp.

     *
@@ -1062,6 +1061,7 @@ final class BinderProxy implements IBinder { 
     */

    private static BinderProxy getInstance(long nativeData, long iBinder) {

        BinderProxy result;

        synchronized (sProxyMap) {

            try {

                result = sProxyMap.get(iBinder);

                if (result != null) {
@@ -1077,6 +1077,7 @@ final class BinderProxy implements IBinder { 
            NoImagePreloadHolder.sRegistry.registerNativeAllocation(result, nativeData);

            // The registry now owns nativeData, even if registration threw an exception.

            sProxyMap.set(iBinder, result);

        }

        return result;

    }

core/jni/android_util_Binder.cpp

+17 −35
Original line number Diff line number Diff line
@@ -155,9 +155,8 @@ static struct thread_dispatch_offsets_t 
static constexpr int32_t PROXY_WARN_INTERVAL = 5000;

static constexpr uint32_t GC_INTERVAL = 1000;



// Protected by gProxyLock. We warn if this gets too large.

static int32_t gNumProxies = 0;

static int32_t gProxiesWarned = 0;

static std::atomic<uint32_t> gNumProxies(0);

static std::atomic<uint32_t> gProxiesWarned(0);



// Number of GlobalRefs held by JavaBBinders.

static std::atomic<uint32_t> gNumLocalRefsCreated(0);
@@ -632,12 +631,6 @@ BinderProxyNativeData* getBPNativeData(JNIEnv* env, jobject obj) { 
    return (BinderProxyNativeData *) env->GetLongField(obj, gBinderProxyOffsets.mNativeData);

}



static Mutex gProxyLock;



// We may cache a single BinderProxyNativeData node to avoid repeat allocation.

// All fields are null. Protected by gProxyLock.

static BinderProxyNativeData *gNativeDataCache;



// If the argument is a JavaBBinder, return the Java object that was used to create it.

// Otherwise return a BinderProxy for the IBinder. If a previous call was passed the

// same IBinder, and the original BinderProxy is still alive, return the same BinderProxy.
@@ -652,36 +645,31 @@ jobject javaObjectForIBinder(JNIEnv* env, const sp<IBinder>& val) 
        return object;

    }



    // For the rest of the function we will hold this lock, to serialize

    // looking/creation/destruction of Java proxies for native Binder proxies.

    AutoMutex _l(gProxyLock);

    BinderProxyNativeData* nativeData = new BinderProxyNativeData();

    nativeData->mOrgue = new DeathRecipientList;

    nativeData->mObject = val;



    BinderProxyNativeData* nativeData = gNativeDataCache;

    if (nativeData == nullptr) {

        nativeData = new BinderProxyNativeData();

    }

    // gNativeDataCache is now logically empty.

    jobject object = env->CallStaticObjectMethod(gBinderProxyOffsets.mClass,

            gBinderProxyOffsets.mGetInstance, (jlong) nativeData, (jlong) val.get());

    if (env->ExceptionCheck()) {

        // In the exception case, getInstance still took ownership of nativeData.

        gNativeDataCache = nullptr;

        return NULL;

    }

    BinderProxyNativeData* actualNativeData = getBPNativeData(env, object);

    if (actualNativeData == nativeData) {

        // New BinderProxy; we still have exclusive access.

        nativeData->mOrgue = new DeathRecipientList;

        nativeData->mObject = val;

        gNativeDataCache = nullptr;

        ++gNumProxies;

        if (gNumProxies >= gProxiesWarned + PROXY_WARN_INTERVAL) {

            ALOGW("Unexpectedly many live BinderProxies: %d\n", gNumProxies);

            gProxiesWarned = gNumProxies;

        // Created a new Proxy

        uint32_t numProxies = gNumProxies.fetch_add(1, std::memory_order_relaxed);

        uint32_t numLastWarned = gProxiesWarned.load(std::memory_order_relaxed);

        if (numProxies >= numLastWarned + PROXY_WARN_INTERVAL) {

            // Multiple threads can get here, make sure only one of them gets to

            // update the warn counter.

            if (gProxiesWarned.compare_exchange_strong(numLastWarned,

                        numLastWarned + PROXY_WARN_INTERVAL, std::memory_order_relaxed)) {

                ALOGW("Unexpectedly many live BinderProxies: %d\n", numProxies);

            }

        }

    } else {

        // nativeData wasn't used. Reuse it the next time.

        gNativeDataCache = nativeData;

        delete nativeData;

    }



    return object;
@@ -959,8 +947,7 @@ jint android_os_Debug_getLocalObjectCount(JNIEnv* env, jobject clazz) 


jint android_os_Debug_getProxyObjectCount(JNIEnv* env, jobject clazz)

{

    AutoMutex _l(gProxyLock);

    return gNumProxies;

    return gNumProxies.load();

}



jint android_os_Debug_getDeathObjectCount(JNIEnv* env, jobject clazz)
@@ -1008,8 +995,6 @@ static void android_os_BinderInternal_proxyLimitcallback(int uid) 
{

    JNIEnv *env = AndroidRuntime::getJNIEnv();

    {

        // Calls into BinderProxy must be serialized

        AutoMutex _l(gProxyLock);

        env->CallStaticObjectMethod(gBinderProxyOffsets.mClass,

                                    gBinderProxyOffsets.mDumpProxyDebugInfo);

    }
@@ -1367,9 +1352,6 @@ static jboolean android_os_BinderProxy_unlinkToDeath(JNIEnv* env, jobject obj, 


static void BinderProxy_destroy(void* rawNativeData)

{

    // Don't race with construction/initialization

    AutoMutex _l(gProxyLock);



    BinderProxyNativeData * nativeData = (BinderProxyNativeData *) rawNativeData;

    LOGDEATH("Destroying BinderProxy: binder=%p drl=%p\n",

            nativeData->mObject.get(), nativeData->mOrgue.get());