Make WeakHandleBase::mImpl as unique_ptr + nullcheck for Reset and comparision

Until now, WeakHandleBase::Reset() function and == operator
don't check mImpl was nullptr.

But it could be nullptr when we use move operations.

So we should check nullptr before call mImpl->Reset() and operator==.

===

Also, to avoid this kind of leak, let we use std::unique_ptr instead of raw pointer.

Change-Id: Icd0b603b825d6a1f5eb7697466da98af05540b2b
Signed-off-by: Eunki, Hong <eunkiki.hong@samsung.com>

diff --git a/automated-tests/src/dali/utc-Dali-WeakHandle.cpp b/automated-tests/src/dali/utc-Dali-WeakHandle.cpp
index 77d698120613a9bc5bda054de1bc28ef54ed6d69..57aa048435313c2651c01891dc3cb5aa732223f3 100644 (file)
--- a/automated-tests/src/dali/utc-Dali-WeakHandle.cpp
+++ b/automated-tests/src/dali/utc-Dali-WeakHandle.cpp
@@ -268,6 +268,8 @@ int UtcDaliWeakHandleBaseMoveConstructor(void)
   DALI_TEST_EQUALS(1, actor.GetBaseObject().ReferenceCount(), TEST_LOCATION); // reference count of the actor is not increased
   DALI_TEST_CHECK(!object.GetBaseHandle());                                   // object moved
 
+  object.Reset(); /// No effect to moved object
+
   END_TEST;
 }
 
@@ -288,6 +290,8 @@ int UtcDaliWeakHandleBaseMoveAssignment(void)
   DALI_TEST_EQUALS(1, actor.GetBaseObject().ReferenceCount(), TEST_LOCATION); // reference count of the actor is not increased
   DALI_TEST_CHECK(!object.GetBaseHandle());                                   // object moved
 
+  object.Reset(); /// No effect to moved object
+
   END_TEST;
 }
 
@@ -363,6 +367,58 @@ int UtcDaliWeakHandleBaseInequalityOperatorN(void)
   END_TEST;
 }
 
+int UtcDaliWeakHandleBaseEqualityOperatorVariousCases(void)
+{
+  TestApplication application;
+  tet_infoline("Positive Test Dali::WeakHandleBase::operator== with various cases");
+
+  WeakHandleBase object;
+  WeakHandleBase theSameObject;
+  DALI_TEST_CHECK(object == theSameObject);
+
+  Actor actor = Actor::New();
+
+  object = WeakHandleBase(actor);
+  DALI_TEST_CHECK(object.GetBaseHandle() == actor);
+
+  theSameObject = WeakHandleBase(actor);
+  DALI_TEST_CHECK(theSameObject.GetBaseHandle() == actor);
+  DALI_TEST_CHECK(object == theSameObject);
+
+  // Compare with empty object
+  tet_printf("Compare with empty object\n");
+  WeakHandleBase emptyObject;
+  DALI_TEST_CHECK(object != emptyObject);
+
+  tet_printf("Compare with moved object\n");
+  WeakHandleBase movedObject = std::move(theSameObject);
+
+  DALI_TEST_CHECK(movedObject.GetBaseHandle() == actor);
+  DALI_TEST_CHECK(object == movedObject);
+
+  DALI_TEST_CHECK(!theSameObject.GetBaseHandle());
+  DALI_TEST_CHECK(emptyObject == theSameObject);
+
+  tet_printf("Compare after Reset called\n");
+
+  object.Reset();
+
+  DALI_TEST_CHECK(emptyObject == object);
+  DALI_TEST_CHECK(theSameObject == object);
+  DALI_TEST_CHECK(object != movedObject);
+
+  tet_printf("Compare between moved objects\n");
+
+  movedObject = std::move(object);
+  DALI_TEST_CHECK(emptyObject == object);
+  DALI_TEST_CHECK(theSameObject == object);
+  DALI_TEST_CHECK(object == movedObject);
+  DALI_TEST_CHECK(emptyObject == movedObject);
+  DALI_TEST_CHECK(theSameObject == movedObject);
+
+  END_TEST;
+}
+
 int UtcDaliWeakHandleBaseGetBaseHandle(void)
 {
   TestApplication application;
@@ -411,6 +467,12 @@ int UtcDaliWeakHandleBaseReset(void)
   DALI_TEST_CHECK(object == WeakHandleBase());
   DALI_TEST_CHECK(object.GetBaseHandle() == Handle());
 
+  // Call Reset one more time.
+  object.Reset();
+
+  DALI_TEST_CHECK(object == WeakHandleBase());
+  DALI_TEST_CHECK(object.GetBaseHandle() == Handle());
+
   END_TEST;
 }
 
@@ -486,8 +548,9 @@ class SelfDestructHandle : public Dali::BaseHandle
 public:
   static SelfDestructHandle New();
 
-  SelfDestructHandle() = default;
+  SelfDestructHandle()  = default;
   ~SelfDestructHandle() = default;
+
 private:
   explicit SelfDestructHandle(SelfDestructObject* object);
 };
@@ -523,8 +586,8 @@ SelfDestructHandle::SelfDestructHandle(SelfDestructObject* object)
 SelfDestructHandle SelfDestructHandle::New()
 {
   IntrusivePtr<SelfDestructObject> object = new SelfDestructObject();
-  auto handle = SelfDestructHandle(object.Get());
-  object->mWeakHandle = handle;
+  auto                             handle = SelfDestructHandle(object.Get());
+  object->mWeakHandle                     = handle;
   return handle;
 }
 
@@ -556,4 +619,3 @@ int UtcDaliWeakHandleInvalidDuringSelfDestruction(void)
 
   END_TEST;
 }
-

diff --git a/dali/public-api/object/weak-handle.cpp b/dali/public-api/object/weak-handle.cpp
index 4f5a3f17a110d2b03ea3723bc5943cd33dc2618e..6f5d769e7415254515c7c401b864edffa7251327 100644 (file)
--- a/dali/public-api/object/weak-handle.cpp
+++ b/dali/public-api/object/weak-handle.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020 Samsung Electronics Co., Ltd.
+ * Copyright (c) 2024 Samsung Electronics Co., Ltd.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -84,44 +84,38 @@ WeakHandleBase::WeakHandleBase(BaseHandle& handle)
 
 WeakHandleBase::~WeakHandleBase()
 {
-  delete mImpl;
-  mImpl = nullptr;
 }
 
 WeakHandleBase::WeakHandleBase(const WeakHandleBase& handle)
 : mImpl(nullptr)
 {
   BaseHandle object = handle.GetBaseHandle();
-  mImpl             = new Impl(object);
+  mImpl             = std::make_unique<Impl>(object);
 }
 
 WeakHandleBase& WeakHandleBase::operator=(const WeakHandleBase& rhs)
 {
   if(this != &rhs)
   {
-    delete mImpl;
+    mImpl.reset();
 
     BaseHandle handle = rhs.GetBaseHandle();
-    mImpl             = new Impl(handle);
+    mImpl             = std::make_unique<Impl>(handle);
   }
 
   return *this;
 }
 
 WeakHandleBase::WeakHandleBase(WeakHandleBase&& rhs) noexcept
-: mImpl(rhs.mImpl)
+: mImpl(std::move(rhs.mImpl))
 {
-  rhs.mImpl = nullptr;
 }
 
 WeakHandleBase& WeakHandleBase::operator=(WeakHandleBase&& rhs) noexcept
 {
   if(this != &rhs)
   {
-    delete mImpl;
-
-    mImpl     = rhs.mImpl;
-    rhs.mImpl = nullptr;
+    mImpl = std::move(rhs.mImpl);
   }
 
   return *this;
@@ -129,7 +123,25 @@ WeakHandleBase& WeakHandleBase::operator=(WeakHandleBase&& rhs) noexcept
 
 bool WeakHandleBase::operator==(const WeakHandleBase& rhs) const
 {
-  return this->mImpl->mObject == rhs.mImpl->mObject;
+  if(DALI_LIKELY(this->mImpl))
+  {
+    if(DALI_LIKELY(rhs.mImpl))
+    {
+      return this->mImpl->mObject == rhs.mImpl->mObject;
+    }
+    else
+    {
+      return this->mImpl->mObject == nullptr;
+    }
+  }
+  else
+  {
+    if(DALI_LIKELY(rhs.mImpl))
+    {
+      return rhs.mImpl->mObject == nullptr;
+    }
+  }
+  return true; /// Both are empty handle.
 }
 
 bool WeakHandleBase::operator!=(const WeakHandleBase& rhs) const
@@ -144,7 +156,10 @@ BaseHandle WeakHandleBase::GetBaseHandle() const
 
 void WeakHandleBase::Reset()
 {
-  mImpl->Reset();
+  if(DALI_LIKELY(mImpl))
+  {
+    mImpl->Reset();
+  }
 }
 
 } // namespace Dali

diff --git a/dali/public-api/object/weak-handle.h b/dali/public-api/object/weak-handle.h
index b64e82c10150c60b11eb78d77263918bd30d59af..baa4699e4de6128d0584196681f6ba5279711359 100644 (file)
--- a/dali/public-api/object/weak-handle.h
+++ b/dali/public-api/object/weak-handle.h
@@ -2,7 +2,7 @@
 #define DALI_WEAK_HANDLE_H
 
 /*
- * Copyright (c) 2020 Samsung Electronics Co., Ltd.
+ * Copyright (c) 2024 Samsung Electronics Co., Ltd.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,6 +17,8 @@
  * limitations under the License.
  *
  */
+// EXTERNAL_INCLUDES
+#include <memory> ///< for std::unique_ptr
 
 // INTERNAL INCLUDES
 #include <dali/public-api/actors/custom-actor.h>
@@ -130,7 +132,7 @@ public:
 protected:
   /// @cond internal
   struct Impl;
-  Impl* mImpl;
+  std::unique_ptr<Impl> mImpl;
   /// @endcond
 };