From: Eunki, Hong Date: Tue, 29 Oct 2024 08:58:23 +0000 (+0900) Subject: [Tizen] Make WeakHandleBase::mImpl as unique_ptr + nullcheck for Reset and comparision X-Git-Tag: accepted/tizen/7.0/unified/20241106.055426^0 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=refs%2Fheads%2Ftizen_7.0;p=platform%2Fcore%2Fuifw%2Fdali-core.git [Tizen] 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 --- diff --git a/automated-tests/src/dali/utc-Dali-WeakHandle.cpp b/automated-tests/src/dali/utc-Dali-WeakHandle.cpp index 4b8fe8b..68d8f8e 100644 --- a/automated-tests/src/dali/utc-Dali-WeakHandle.cpp +++ b/automated-tests/src/dali/utc-Dali-WeakHandle.cpp @@ -264,6 +264,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; } @@ -284,6 +286,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; } @@ -359,6 +363,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; @@ -407,6 +463,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; } diff --git a/dali/public-api/object/weak-handle.cpp b/dali/public-api/object/weak-handle.cpp index 3a8ff33..bf827c1 100644 --- 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(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(handle); } return *this; } WeakHandleBase::WeakHandleBase(WeakHandleBase&& rhs) -: mImpl(rhs.mImpl) +: mImpl(std::move(rhs.mImpl)) { - rhs.mImpl = nullptr; } WeakHandleBase& WeakHandleBase::operator=(WeakHandleBase&& rhs) { 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) 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 b3a8aa7..b627559 100644 --- 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 ///< for std::unique_ptr // INTERNAL INCLUDES #include @@ -130,7 +132,7 @@ public: protected: /// @cond internal struct Impl; - Impl* mImpl; + std::unique_ptr mImpl; /// @endcond };