Merge "Move BinderProxy serialization into Java."

2 files changed, 38 insertions, 55 deletions

diff --git a/core/java/android/os/Binder.java b/core/java/android/os/Binder.java
index 5923529bdc21..f31130f8645d 100644
--- a/core/java/android/os/Binder.java
+++ b/core/java/android/os/Binder.java
@@ -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) {
-            sProxyMap.dumpProxyInterfaceCounts();
+            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,21 +1061,23 @@ final class BinderProxy implements IBinder {
      */
     private static BinderProxy getInstance(long nativeData, long iBinder) {
         BinderProxy result;
-        try {
-            result = sProxyMap.get(iBinder);
-            if (result != null) {
-                return result;
+        synchronized (sProxyMap) {
+            try {
+                result = sProxyMap.get(iBinder);
+                if (result != null) {
+                    return result;
+                }
+                result = new BinderProxy(nativeData);
+            } catch (Throwable e) {
+                // We're throwing an exception (probably OOME); don't drop nativeData.
+                NativeAllocationRegistry.applyFreeFunction(NoImagePreloadHolder.sNativeFinalizer,
+                        nativeData);
+                throw e;
             }
-            result = new BinderProxy(nativeData);
-        } catch (Throwable e) {
-            // We're throwing an exception (probably OOME); don't drop nativeData.
-            NativeAllocationRegistry.applyFreeFunction(NoImagePreloadHolder.sNativeFinalizer,
-                    nativeData);
-            throw e;
+            NoImagePreloadHolder.sRegistry.registerNativeAllocation(result, nativeData);
+            // The registry now owns nativeData, even if registration threw an exception.
+            sProxyMap.set(iBinder, result);
         }
-        NoImagePreloadHolder.sRegistry.registerNativeAllocation(result, nativeData);
-        // The registry now owns nativeData, even if registration threw an exception.
-        sProxyMap.set(iBinder, result);
         return result;
     }
 
diff --git a/core/jni/android_util_Binder.cpp b/core/jni/android_util_Binder.cpp
index c3ba9ba82826..7e2bad2f7946 100644
--- a/core/jni/android_util_Binder.cpp
+++ b/core/jni/android_util_Binder.cpp
@@ -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());