!176 ipc readremoteobject bugfix

Merge pull request !176 from liubb_0516/master

ipc/native/src/core/include/ipc_process_skeleton.h

@@ -208,7 +208,7 @@ private:
     IPCProcessSkeleton();
     static IPCProcessSkeleton *instance_;
     static std::mutex procMutex_;
-    std::recursive_mutex mutex_;
+    std::shared_mutex mutex_;
     std::shared_mutex rawDataMutex_;
     std::map<std::u16string, wptr<IRemoteObject>> objects_;
     std::map<IRemoteObject *, bool> isContainStub_;

ipc/native/src/core/source/ipc_object_proxy.cpp

@@ -228,20 +228,11 @@ void IPCObjectProxy::OnLastStrongRef(const void *objectId)
         return;
     }
 #ifndef CONFIG_IPC_SINGLE
-    std::shared_ptr<DBinderSessionObject> session = nullptr;
+    ReleaseProto();
+    (void)current->ProxyDetachDBinderSession(handle_);
+    (void)current->DetachHandleToIndex(handle_);
 #endif
-    {
-        std::lock_guard<std::recursive_mutex> lock(current->mutex_);
-        if (current->DetachObjectInner(this) == false) { // if detach successfully, this proxy will be destroyed
-            return;
-        }
-#ifndef CONFIG_IPC_SINGLE
-        ReleaseProto();
-        session = current->ProxyQueryDBinderSession(handle_);
-        (void)current->ProxyDetachDBinderSession(handle_);
-        (void)current->DetachHandleToIndex(handle_);
-#endif
-    }
+    current->DetachObject(this);
     IRemoteInvoker *invoker = IPCThreadSkeleton::GetDefaultInvoker();
     if (invoker != nullptr) {
         invoker->ReleaseHandle(handle_);
@@ -362,24 +353,21 @@ bool IPCObjectProxy::RemoveDeathRecipient(const sptr<DeathRecipient> &recipient)
 
 void IPCObjectProxy::SendObituary()
 {
+    std::vector<sptr<DeathRecipient>> deathCallback;
     {
         std::lock_guard<std::recursive_mutex> lock(mutex_);
         MarkObjectDied();
-        size_t recipientCount = recipients_.size();
-        for (size_t i = 0; i < recipientCount; i++) {
-            sptr<DeathRecipient> recipient = recipients_[i];
-            ZLOGW(LABEL, "%s: handle = %{public}u call OnRemoteDied", __func__, handle_);
-            if (recipient != nullptr) {
-                recipient->OnRemoteDied(this);
-            }
+        deathCallback = recipients_;
+        IRemoteInvoker *invoker = IPCThreadSkeleton::GetDefaultInvoker();
+        if (deathCallback.size() > 0 && invoker != nullptr) {
+            invoker->RemoveDeathRecipient(handle_, this);
         }
         recipients_.clear();
-
-        if (recipientCount > 0) {
-            IRemoteInvoker *invoker = IPCThreadSkeleton::GetDefaultInvoker();
-            if (invoker != nullptr) {
-                invoker->RemoveDeathRecipient(handle_, this);
-            }
+    }
+    for (auto &deathRecipient : deathCallback) {
+        ZLOGW(LABEL, "%s: handle = %{public}u call OnRemoteDied", __func__, handle_);
+        if (deathRecipient != nullptr) {
+            deathRecipient->OnRemoteDied(this);
         }
     }
 #ifndef CONFIG_IPC_SINGLE
@@ -404,11 +392,11 @@ int32_t IPCObjectProxy::NoticeServiceDie()
 {
     MessageParcel data;
     MessageParcel reply;
-    MessageOption option(MessageOption::TF_SYNC);
+    MessageOption option(MessageOption::TF_ASYNC);
     data.WriteInt32(IRemoteObject::DeathRecipient::NOTICE_DEATH_RECIPIENT);
 
     int status = SendLocalRequest(DBINDER_OBITUARY_TRANSACTION, data, reply, option);
-    if (status != ERR_NONE || reply.ReadInt32() != ERR_NONE) {
+    if (status != ERR_NONE) {
         ZLOGE(LABEL, "%s: send local request fail, status = %d", __func__, status);
         return IPC_PROXY_TRANSACTION_ERR;
     }

ipc/native/src/core/source/ipc_process_skeleton.cpp

@@ -126,10 +126,10 @@ IRemoteObject *IPCProcessSkeleton::FindOrNewObject(int handle)
     IRemoteObject *remoteObject = nullptr;
     std::u16string descriptor = MakeHandleDescriptor(handle);
     {
-        std::lock_guard<std::recursive_mutex> lock(mutex_);
-
+        std::unique_lock<std::shared_mutex> lockGuard(mutex_);
         remoteObject = QueryObjectInner(descriptor);
-        if (remoteObject == nullptr) {
+        if (remoteObject == nullptr || !remoteObject->AttemptIncStrong(this)) {
+            // create a new proxy, the old proxy is destroying concurrently.
             if (handle == REGISTRY_HANDLE) {
                 IRemoteInvoker *invoker = IPCThreadSkeleton::GetRemoteInvoker(IRemoteObject::IF_PROT_DEFAULT);
                 if (invoker == nullptr) {
@@ -141,20 +141,16 @@ IRemoteObject *IPCProcessSkeleton::FindOrNewObject(int handle)
                     return nullptr;
                 }
             }
-
             auto proxy = new (std::nothrow) IPCObjectProxy(handle, descriptor);
             if (proxy == nullptr) {
+                DBINDER_LOGE("Construct ipc object proxy failed");
                 return nullptr;
             }
-            proxy->AttemptAcquire(this); // AttemptAcquire always returns true as life time is extended
+            // AttemptAcquire always returns true as life time is extended.
+            // OnFirstStrongRef will be called.
+            proxy->AttemptAcquire(this);
             remoteObject = reinterpret_cast<IRemoteObject *>(proxy);
-            if (!AttachObjectInner(remoteObject)) {
-                DBINDER_LOGE("attach object failed");
-                delete proxy;
-                return nullptr;
-            }
-        } else {
-            remoteObject->AttemptAcquire(this);
+            AttachObjectInner(remoteObject);
         }
     }
 
@@ -236,31 +232,24 @@ bool IPCProcessSkeleton::IsContainsObject(IRemoteObject *object)
 
 bool IPCProcessSkeleton::DetachObject(IRemoteObject *object)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex_);
-    return DetachObjectInner(object);
-}
-
-bool IPCProcessSkeleton::DetachObjectInner(IRemoteObject *object)
-{
-    int strongRef = object->GetSptrRefCount();
-    if (strongRef > 0) {
-        DBINDER_LOGI("proxy is still strong referenced:%{public}d", strongRef);
-        return false;
-    }
-
-    // If it fails, clear it in the destructor.
+    std::unique_lock<std::shared_mutex> lockGuard(mutex_);
     (void)isContainStub_.erase(object);
+
     std::u16string descriptor = object->GetObjectDescriptor();
     if (descriptor.empty()) {
         return false;
     }
-
-    return (objects_.erase(descriptor) > 0);
+    auto iterator = objects_.find(descriptor);
+    if (iterator->second == object) {
+        objects_.erase(iterator);
+        return true;
+    }
+    return false;
 }
 
 bool IPCProcessSkeleton::AttachObject(IRemoteObject *object)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex_);
+    std::unique_lock<std::shared_mutex> lockGuard(mutex_);
     return AttachObjectInner(object);
 }
 
@@ -273,8 +262,9 @@ bool IPCProcessSkeleton::AttachObjectInner(IRemoteObject *object)
         return false;
     }
 
-    auto result = objects_.insert(std::pair<std::u16string, wptr<IRemoteObject>>(descriptor, object));
-    return result.second;
+    wptr<IRemoteObject> wp = object;
+    objects_.insert_or_assign(descriptor, wp);
+    return true;
 }
 
 IRemoteObject *IPCProcessSkeleton::QueryObject(const std::u16string &descriptor)
@@ -283,7 +273,7 @@ IRemoteObject *IPCProcessSkeleton::QueryObject(const std::u16string &descriptor)
         return nullptr;
     }
 
-    std::lock_guard<std::recursive_mutex> lock(mutex_);
+    std::shared_lock<std::shared_mutex> lockGuard(mutex_);
     return QueryObjectInner(descriptor);
 }