Merge "Merge changes Ib1c28c94,Id3485a2a,If5ad5acf,Idc9054bd,Ie4e7e25d am: 756621f54d am: aae80e18c3" into sc-dev-plus-aosp am: d2674a3906
Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/native/+/15128207
Change-Id: I61b2de49fa47a43e79bcc47dd5e01289d8381cb9
diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp
index 02321cd..53750c9 100644
--- a/libs/binder/Binder.cpp
+++ b/libs/binder/Binder.cpp
@@ -179,6 +179,17 @@
return transact(SET_RPC_CLIENT_TRANSACTION, data, &reply);
}
+void IBinder::withLock(const std::function<void()>& doWithLock) {
+ BBinder* local = localBinder();
+ if (local) {
+ local->withLock(doWithLock);
+ return;
+ }
+ BpBinder* proxy = this->remoteBinder();
+ LOG_ALWAYS_FATAL_IF(proxy == nullptr, "binder object must be either local or remote");
+ proxy->withLock(doWithLock);
+}
+
// ---------------------------------------------------------------------------
class BBinder::RpcServerLink : public IBinder::DeathRecipient {
@@ -311,15 +322,13 @@
return NO_ERROR;
}
-void BBinder::attachObject(
- const void* objectID, void* object, void* cleanupCookie,
- object_cleanup_func func)
-{
+void* BBinder::attachObject(const void* objectID, void* object, void* cleanupCookie,
+ object_cleanup_func func) {
Extras* e = getOrCreateExtras();
- if (!e) return; // out of memory
+ if (!e) return nullptr; // out of memory
AutoMutex _l(e->mLock);
- e->mObjects.attach(objectID, object, cleanupCookie, func);
+ return e->mObjects.attach(objectID, object, cleanupCookie, func);
}
void* BBinder::findObject(const void* objectID) const
@@ -331,13 +340,20 @@
return e->mObjects.find(objectID);
}
-void BBinder::detachObject(const void* objectID)
-{
+void* BBinder::detachObject(const void* objectID) {
Extras* e = mExtras.load(std::memory_order_acquire);
- if (!e) return;
+ if (!e) return nullptr;
AutoMutex _l(e->mLock);
- e->mObjects.detach(objectID);
+ return e->mObjects.detach(objectID);
+}
+
+void BBinder::withLock(const std::function<void()>& doWithLock) {
+ Extras* e = getOrCreateExtras();
+ LOG_ALWAYS_FATAL_IF(!e, "no memory");
+
+ AutoMutex _l(e->mLock);
+ doWithLock();
}
BBinder* BBinder::localBinder()
diff --git a/libs/binder/BpBinder.cpp b/libs/binder/BpBinder.cpp
index 5e44a0f..3099296 100644
--- a/libs/binder/BpBinder.cpp
+++ b/libs/binder/BpBinder.cpp
@@ -61,22 +61,22 @@
kill();
}
-void BpBinder::ObjectManager::attach(
- const void* objectID, void* object, void* cleanupCookie,
- IBinder::object_cleanup_func func)
-{
+void* BpBinder::ObjectManager::attach(const void* objectID, void* object, void* cleanupCookie,
+ IBinder::object_cleanup_func func) {
entry_t e;
e.object = object;
e.cleanupCookie = cleanupCookie;
e.func = func;
- if (mObjects.indexOfKey(objectID) >= 0) {
- ALOGE("Trying to attach object ID %p to binder ObjectManager %p with object %p, but object ID already in use",
- objectID, this, object);
- return;
+ if (ssize_t idx = mObjects.indexOfKey(objectID); idx >= 0) {
+ ALOGI("Trying to attach object ID %p to binder ObjectManager %p with object %p, but object "
+ "ID already in use",
+ objectID, this, object);
+ return mObjects[idx].object;
}
mObjects.add(objectID, e);
+ return nullptr;
}
void* BpBinder::ObjectManager::find(const void* objectID) const
@@ -86,9 +86,12 @@
return mObjects.valueAt(i).object;
}
-void BpBinder::ObjectManager::detach(const void* objectID)
-{
- mObjects.removeItem(objectID);
+void* BpBinder::ObjectManager::detach(const void* objectID) {
+ ssize_t idx = mObjects.indexOfKey(objectID);
+ if (idx < 0) return nullptr;
+ void* value = mObjects[idx].object;
+ mObjects.removeItemsAt(idx, 1);
+ return value;
}
void BpBinder::ObjectManager::kill()
@@ -406,14 +409,11 @@
recipient->binderDied(wp<BpBinder>::fromExisting(this));
}
-
-void BpBinder::attachObject(
- const void* objectID, void* object, void* cleanupCookie,
- object_cleanup_func func)
-{
+void* BpBinder::attachObject(const void* objectID, void* object, void* cleanupCookie,
+ object_cleanup_func func) {
AutoMutex _l(mLock);
ALOGV("Attaching object %p to binder %p (manager=%p)", object, this, &mObjects);
- mObjects.attach(objectID, object, cleanupCookie, func);
+ return mObjects.attach(objectID, object, cleanupCookie, func);
}
void* BpBinder::findObject(const void* objectID) const
@@ -422,10 +422,14 @@
return mObjects.find(objectID);
}
-void BpBinder::detachObject(const void* objectID)
-{
+void* BpBinder::detachObject(const void* objectID) {
AutoMutex _l(mLock);
- mObjects.detach(objectID);
+ return mObjects.detach(objectID);
+}
+
+void BpBinder::withLock(const std::function<void()>& doWithLock) {
+ AutoMutex _l(mLock);
+ doWithLock();
}
BpBinder* BpBinder::remoteBinder()
diff --git a/libs/binder/TEST_MAPPING b/libs/binder/TEST_MAPPING
index e04e326..748fa72 100644
--- a/libs/binder/TEST_MAPPING
+++ b/libs/binder/TEST_MAPPING
@@ -19,7 +19,7 @@
"name": "binderTextOutputTest"
},
{
- "name": "binderParcelTest"
+ "name": "binderUnitTest"
},
{
"name": "binderLibTest"
diff --git a/libs/binder/include/binder/Binder.h b/libs/binder/include/binder/Binder.h
index 472e546..46223bb 100644
--- a/libs/binder/include/binder/Binder.h
+++ b/libs/binder/include/binder/Binder.h
@@ -54,12 +54,11 @@
uint32_t flags = 0,
wp<DeathRecipient>* outRecipient = nullptr);
- virtual void attachObject( const void* objectID,
- void* object,
- void* cleanupCookie,
- object_cleanup_func func) final;
+ virtual void* attachObject(const void* objectID, void* object, void* cleanupCookie,
+ object_cleanup_func func) final;
virtual void* findObject(const void* objectID) const final;
- virtual void detachObject(const void* objectID) final;
+ virtual void* detachObject(const void* objectID) final;
+ void withLock(const std::function<void()>& doWithLock);
virtual BBinder* localBinder();
diff --git a/libs/binder/include/binder/BpBinder.h b/libs/binder/include/binder/BpBinder.h
index 61bf018..c69bb9e 100644
--- a/libs/binder/include/binder/BpBinder.h
+++ b/libs/binder/include/binder/BpBinder.h
@@ -72,12 +72,11 @@
uint32_t flags = 0,
wp<DeathRecipient>* outRecipient = nullptr);
- virtual void attachObject( const void* objectID,
- void* object,
- void* cleanupCookie,
- object_cleanup_func func) final;
+ virtual void* attachObject(const void* objectID, void* object, void* cleanupCookie,
+ object_cleanup_func func) final;
virtual void* findObject(const void* objectID) const final;
- virtual void detachObject(const void* objectID) final;
+ virtual void* detachObject(const void* objectID) final;
+ void withLock(const std::function<void()>& doWithLock);
virtual BpBinder* remoteBinder();
@@ -91,27 +90,23 @@
static void setLimitCallback(binder_proxy_limit_callback cb);
static void setBinderProxyCountWatermarks(int high, int low);
- class ObjectManager
- {
+ class ObjectManager {
public:
- ObjectManager();
- ~ObjectManager();
+ ObjectManager();
+ ~ObjectManager();
- void attach( const void* objectID,
- void* object,
- void* cleanupCookie,
- IBinder::object_cleanup_func func);
- void* find(const void* objectID) const;
- void detach(const void* objectID);
+ void* attach(const void* objectID, void* object, void* cleanupCookie,
+ IBinder::object_cleanup_func func);
+ void* find(const void* objectID) const;
+ void* detach(const void* objectID);
- void kill();
+ void kill();
private:
- ObjectManager(const ObjectManager&);
+ ObjectManager(const ObjectManager&);
ObjectManager& operator=(const ObjectManager&);
- struct entry_t
- {
+ struct entry_t {
void* object;
void* cleanupCookie;
IBinder::object_cleanup_func func;
diff --git a/libs/binder/include/binder/IBinder.h b/libs/binder/include/binder/IBinder.h
index f9cdac7..c484d83 100644
--- a/libs/binder/include/binder/IBinder.h
+++ b/libs/binder/include/binder/IBinder.h
@@ -22,6 +22,8 @@
#include <utils/String16.h>
#include <utils/Vector.h>
+#include <functional>
+
// linux/binder.h defines this, but we don't want to include it here in order to
// avoid exporting the kernel headers
#ifndef B_PACK_CHARS
@@ -255,26 +257,31 @@
* objects are invoked with their respective objectID, object, and
* cleanupCookie. Access to these APIs can be made from multiple threads,
* but calls from different threads are allowed to be interleaved.
+ *
+ * This returns the object which is already attached. If this returns a
+ * non-null value, it means that attachObject failed. TODO(b/192023359):
+ * remove logs and add [[nodiscard]]
*/
- virtual void attachObject( const void* objectID,
- void* object,
- void* cleanupCookie,
- object_cleanup_func func) = 0;
+ virtual void* attachObject(const void* objectID, void* object, void* cleanupCookie,
+ object_cleanup_func func) = 0;
/**
* Returns object attached with attachObject.
*/
virtual void* findObject(const void* objectID) const = 0;
/**
- * WARNING: this API does not call the cleanup function for legacy reasons.
- * It also does not return void* for legacy reasons. If you need to detach
- * an object and destroy it, there are two options:
- * - if you can, don't call detachObject and instead wait for the destructor
- * to clean it up.
- * - manually retrieve and destruct the object (if multiple of your threads
- * are accessing these APIs, you must guarantee that attachObject isn't
- * called after findObject and before detachObject is called).
+ * Returns object attached with attachObject, and detaches it. This does not
+ * delete the object. This is equivalent to using attachObject to attach a null
+ * object.
*/
- virtual void detachObject(const void* objectID) = 0;
+ virtual void* detachObject(const void* objectID) = 0;
+
+ /**
+ * Use the lock that this binder contains internally. For instance, this can
+ * be used to modify an attached object without needing to add an additional
+ * lock (though, that attached object must be retrieved before calling this
+ * method). Calling (most) IBinder methods inside this will deadlock.
+ */
+ void withLock(const std::function<void()>& doWithLock);
virtual BBinder* localBinder();
virtual BpBinder* remoteBinder();
diff --git a/libs/binder/ndk/ibinder.cpp b/libs/binder/ndk/ibinder.cpp
index 1dcb41b..953b2be 100644
--- a/libs/binder/ndk/ibinder.cpp
+++ b/libs/binder/ndk/ibinder.cpp
@@ -57,7 +57,6 @@
namespace ABpBinderTag {
-static std::mutex gLock;
static const void* kId = "ABpBinder";
struct Value {
wp<ABpBinder> binder;
@@ -232,19 +231,14 @@
ABpBinder::~ABpBinder() {}
void ABpBinder::onLastStrongRef(const void* id) {
- {
- std::lock_guard<std::mutex> lock(ABpBinderTag::gLock);
- // Since ABpBinder is OBJECT_LIFETIME_WEAK, we must remove this weak reference in order for
- // the ABpBinder to be deleted. Since a strong reference to this ABpBinder object should no
- // longer be able to exist at the time of this method call, there is no longer a need to
- // recover it.
+ // Since ABpBinder is OBJECT_LIFETIME_WEAK, we must remove this weak reference in order for
+ // the ABpBinder to be deleted. Since a strong reference to this ABpBinder object should no
+ // longer be able to exist at the time of this method call, there is no longer a need to
+ // recover it.
- ABpBinderTag::Value* value =
- static_cast<ABpBinderTag::Value*>(remote()->findObject(ABpBinderTag::kId));
- if (value != nullptr) {
- value->binder = nullptr;
- }
- }
+ ABpBinderTag::Value* value =
+ static_cast<ABpBinderTag::Value*>(remote()->detachObject(ABpBinderTag::kId));
+ if (value) ABpBinderTag::clean(ABpBinderTag::kId, value, nullptr /*cookie*/);
BpRefBase::onLastStrongRef(id);
}
@@ -257,23 +251,28 @@
return static_cast<ABBinder*>(binder.get());
}
- // The following code ensures that for a given binder object (remote or local), if it is not an
- // ABBinder then at most one ABpBinder object exists in a given process representing it.
- std::lock_guard<std::mutex> lock(ABpBinderTag::gLock);
-
- ABpBinderTag::Value* value =
- static_cast<ABpBinderTag::Value*>(binder->findObject(ABpBinderTag::kId));
+ auto* value = static_cast<ABpBinderTag::Value*>(binder->findObject(ABpBinderTag::kId));
if (value == nullptr) {
value = new ABpBinderTag::Value;
- binder->attachObject(ABpBinderTag::kId, static_cast<void*>(value), nullptr /*cookie*/,
- ABpBinderTag::clean);
+ auto oldValue = static_cast<ABpBinderTag::Value*>(
+ binder->attachObject(ABpBinderTag::kId, static_cast<void*>(value),
+ nullptr /*cookie*/, ABpBinderTag::clean));
+
+ // allocated by another thread
+ if (oldValue) {
+ delete value;
+ value = oldValue;
+ }
}
- sp<ABpBinder> ret = value->binder.promote();
- if (ret == nullptr) {
- ret = new ABpBinder(binder);
- value->binder = ret;
- }
+ sp<ABpBinder> ret;
+ binder->withLock([&]() {
+ ret = value->binder.promote();
+ if (ret == nullptr) {
+ ret = sp<ABpBinder>::make(binder);
+ value->binder = ret;
+ }
+ });
return ret;
}
diff --git a/libs/binder/ndk/ibinder_internal.h b/libs/binder/ndk/ibinder_internal.h
index 6824306..3b515ab 100644
--- a/libs/binder/ndk/ibinder_internal.h
+++ b/libs/binder/ndk/ibinder_internal.h
@@ -105,6 +105,7 @@
ABpBinder* asABpBinder() override { return this; }
private:
+ friend android::sp<ABpBinder>;
explicit ABpBinder(const ::android::sp<::android::IBinder>& binder);
};
diff --git a/libs/binder/tests/Android.bp b/libs/binder/tests/Android.bp
index d5a2b61..565f88e 100644
--- a/libs/binder/tests/Android.bp
+++ b/libs/binder/tests/Android.bp
@@ -77,14 +77,14 @@
// unit test only, which can run on host and doesn't use /dev/binder
cc_test {
- name: "binderParcelTest",
+ name: "binderUnitTest",
host_supported: true,
target: {
darwin: {
enabled: false,
},
},
- srcs: ["binderParcelTest.cpp"],
+ srcs: ["binderParcelUnitTest.cpp", "binderBinderUnitTest.cpp"],
shared_libs: [
"libbinder",
"libutils",
diff --git a/libs/binder/tests/binderBinderUnitTest.cpp b/libs/binder/tests/binderBinderUnitTest.cpp
new file mode 100644
index 0000000..1be0c59
--- /dev/null
+++ b/libs/binder/tests/binderBinderUnitTest.cpp
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <binder/Binder.h>
+#include <binder/IBinder.h>
+#include <gtest/gtest.h>
+
+using android::BBinder;
+using android::OK;
+using android::sp;
+
+const void* kObjectId1 = reinterpret_cast<const void*>(1);
+const void* kObjectId2 = reinterpret_cast<const void*>(2);
+void* kObject1 = reinterpret_cast<void*>(101);
+void* kObject2 = reinterpret_cast<void*>(102);
+void* kObject3 = reinterpret_cast<void*>(103);
+
+TEST(Binder, AttachObject) {
+ auto binder = sp<BBinder>::make();
+ EXPECT_EQ(nullptr, binder->attachObject(kObjectId1, kObject1, nullptr, nullptr));
+ EXPECT_EQ(nullptr, binder->attachObject(kObjectId2, kObject2, nullptr, nullptr));
+ EXPECT_EQ(kObject1, binder->attachObject(kObjectId1, kObject3, nullptr, nullptr));
+}
+
+TEST(Binder, DetachObject) {
+ auto binder = sp<BBinder>::make();
+ EXPECT_EQ(nullptr, binder->attachObject(kObjectId1, kObject1, nullptr, nullptr));
+ EXPECT_EQ(kObject1, binder->detachObject(kObjectId1));
+ EXPECT_EQ(nullptr, binder->attachObject(kObjectId1, kObject2, nullptr, nullptr));
+}
diff --git a/libs/binder/tests/binderParcelTest.cpp b/libs/binder/tests/binderParcelUnitTest.cpp
similarity index 100%
rename from libs/binder/tests/binderParcelTest.cpp
rename to libs/binder/tests/binderParcelUnitTest.cpp