Fix memory leaks when we use doc::Keyframes structure

Detected with LeakSanitizer on keyframes_tests but also reported in
PR #3141. We've finally solved this problem using std::unique_ptr.
This commit is contained in:
David Capello 2022-02-24 19:51:20 -03:00
parent d16c34b247
commit 2100c45de2
4 changed files with 37 additions and 30 deletions

View File

@ -1,5 +1,5 @@
// Aseprite // Aseprite
// Copyright (C) 2019 Igara Studio S.A. // Copyright (C) 2019-2022 Igara Studio S.A.
// Copyright (C) 2001-2017 David Capello // Copyright (C) 2001-2017 David Capello
// //
// This program is distributed under the terms of // This program is distributed under the terms of
@ -86,7 +86,7 @@ doc::color_t color_from_hex(const char* str)
template<typename Container, template<typename Container,
typename ChildNameGetterFunc, typename ChildNameGetterFunc,
typename UpdateXmlChildFunc> typename UpdateXmlChildFunc>
void update_xml_collection(Container& container, void update_xml_collection(const Container& container,
TiXmlElement* xmlParent, TiXmlElement* xmlParent,
const char* childElemName, const char* childElemName,
const char* idAttrName, const char* idAttrName,
@ -111,7 +111,7 @@ void update_xml_collection(Container& container,
continue; continue;
bool found = false; bool found = false;
for (auto child : container) { for (const auto& child : container) {
std::string thisChildName = childNameGetter(child); std::string thisChildName = childNameGetter(child);
if (thisChildName == xmlChildName) { if (thisChildName == xmlChildName) {
existent.insert(thisChildName); existent.insert(thisChildName);
@ -128,7 +128,7 @@ void update_xml_collection(Container& container,
} }
// Add new children // Add new children
for (auto child : container) { for (const auto& child : container) {
std::string thisChildName = childNameGetter(child); std::string thisChildName = childNameGetter(child);
if (existent.find(thisChildName) == existent.end()) { if (existent.find(thisChildName) == existent.end()) {
TiXmlElement xmlChild(childElemName); TiXmlElement xmlChild(childElemName);

View File

@ -1,4 +1,5 @@
// Aseprite Document Library // Aseprite Document Library
// Copyright (c) 2022 Igara Studio S.A.
// Copyright (c) 2017 David Capello // Copyright (c) 2017 David Capello
// //
// This file is released under the terms of the MIT license. // This file is released under the terms of the MIT license.
@ -10,6 +11,7 @@
#include "doc/frame.h" #include "doc/frame.h"
#include <memory>
#include <vector> #include <vector>
namespace doc { namespace doc {
@ -19,14 +21,22 @@ namespace doc {
public: public:
class Key { class Key {
public: public:
Key(const frame_t frame, T* value) : m_frame(frame), m_value(value) { } Key(const frame_t frame,
std::unique_ptr<T>&& value)
: m_frame(frame)
, m_value(std::move(value)) { }
Key(const Key& o)
: m_frame(o.m_frame)
, m_value(o.m_value ? new T(*o.m_value): nullptr) { }
Key(Key&& o) = default;
Key& operator=(Key&& o) = default;
frame_t frame() const { return m_frame; } frame_t frame() const { return m_frame; }
T* value() const { return m_value; } T* value() const { return m_value.get(); }
void setFrame(const frame_t frame) { m_frame = frame; } void setFrame(const frame_t frame) { m_frame = frame; }
void setValue(T* value) { m_value = value; } void setValue(std::unique_ptr<T>&& value) { m_value = std::move(value); }
private: private:
frame_t m_frame; frame_t m_frame = 0;
T* m_value; std::unique_ptr<T> m_value = nullptr;
}; };
typedef std::vector<Key> List; typedef std::vector<Key> List;
@ -112,30 +122,25 @@ namespace doc {
Keyframes(const Keyframes& other) { Keyframes(const Keyframes& other) {
for (const auto& key : other.m_keys) for (const auto& key : other.m_keys)
m_keys.push_back(Key(key.frame(), new T(*key.value()))); m_keys.push_back(Key(key.frame(), std::make_unique<T>(*key.value())));
} }
void insert(const frame_t frame, T* value) { void insert(const frame_t frame, std::unique_ptr<T>&& value) {
auto it = getIterator(frame); auto it = getIterator(frame);
if (it == end()) if (it == end())
m_keys.push_back(Key(frame, value)); m_keys.push_back(Key(frame, std::move(value)));
else if (it->frame() == frame) else if (it->frame() == frame)
it->setValue(value); it->setValue(std::move(value));
else { else {
++it; ++it;
m_keys.insert(it, Key(frame, value)); m_keys.insert(it, Key(frame, std::move(value)));
} }
} }
T* remove(const frame_t frame) { void remove(const frame_t frame) {
auto it = getIterator(frame); auto it = getIterator(frame);
if (it != end()) { if (it != end())
T* value = it->value();
m_keys.erase(it); m_keys.erase(it);
return value;
}
else
return nullptr;
} }
T* operator[](const frame_t frame) { T* operator[](const frame_t frame) {

View File

@ -1,4 +1,5 @@
// Aseprite Document Library // Aseprite Document Library
// Copyright (c) 2022 Igara Studio S.A.
// Copyright (c) 2017 David Capello // Copyright (c) 2017 David Capello
// //
// This file is released under the terms of the MIT license. // This file is released under the terms of the MIT license.
@ -21,7 +22,7 @@ TEST(Keyframes, Operations)
EXPECT_TRUE(k.empty()); EXPECT_TRUE(k.empty());
EXPECT_EQ(0, k.size()); EXPECT_EQ(0, k.size());
k.insert(0, new int(5)); k.insert(0, std::make_unique<int>(5));
EXPECT_FALSE(k.empty()); EXPECT_FALSE(k.empty());
EXPECT_EQ(1, k.size()); EXPECT_EQ(1, k.size());
EXPECT_EQ(0, k.fromFrame()); EXPECT_EQ(0, k.fromFrame());
@ -31,7 +32,7 @@ TEST(Keyframes, Operations)
EXPECT_EQ(5, *k[1]); EXPECT_EQ(5, *k[1]);
EXPECT_EQ(5, *k[2]); EXPECT_EQ(5, *k[2]);
k.insert(2, new int(6)); k.insert(2, std::make_unique<int>(6));
EXPECT_EQ(2, k.size()); EXPECT_EQ(2, k.size());
EXPECT_EQ(0, k.fromFrame()); EXPECT_EQ(0, k.fromFrame());
EXPECT_EQ(2, k.toFrame()); EXPECT_EQ(2, k.toFrame());
@ -41,7 +42,7 @@ TEST(Keyframes, Operations)
EXPECT_EQ(6, *k[2]); EXPECT_EQ(6, *k[2]);
EXPECT_EQ(6, *k[3]); EXPECT_EQ(6, *k[3]);
k.insert(1, new int(3)); k.insert(1, std::make_unique<int>(3));
EXPECT_EQ(3, k.size()); EXPECT_EQ(3, k.size());
EXPECT_EQ(0, k.fromFrame()); EXPECT_EQ(0, k.fromFrame());
EXPECT_EQ(2, k.toFrame()); EXPECT_EQ(2, k.toFrame());
@ -72,10 +73,10 @@ TEST(Keyframes, Operations)
TEST(Keyframes, Range) TEST(Keyframes, Range)
{ {
Keyframes<int> k; Keyframes<int> k;
k.insert(0, new int(5)); k.insert(0, std::make_unique<int>(5));
k.insert(2, nullptr); k.insert(2, nullptr);
k.insert(4, new int(8)); k.insert(4, std::make_unique<int>(8));
k.insert(6, new int(2)); k.insert(6, std::make_unique<int>(2));
EXPECT_EQ(0, k.fromFrame()); EXPECT_EQ(0, k.fromFrame());
EXPECT_EQ(6, k.toFrame()); EXPECT_EQ(6, k.toFrame());
@ -131,7 +132,7 @@ TEST(Keyframes, Range)
TEST(Keyframes, BugEmptyCount) TEST(Keyframes, BugEmptyCount)
{ {
Keyframes<int> k; Keyframes<int> k;
k.insert(7, new int(5)); k.insert(7, std::make_unique<int>(5));
EXPECT_EQ(0, k.range(-1, 6).countKeys()); EXPECT_EQ(0, k.range(-1, 6).countKeys());
EXPECT_EQ(1, k.range(0, 7).countKeys()); EXPECT_EQ(1, k.range(0, 7).countKeys());
EXPECT_EQ(1, k.range(8, 9).countKeys()); EXPECT_EQ(1, k.range(8, 9).countKeys());

View File

@ -1,4 +1,5 @@
// Aseprite Document Library // Aseprite Document Library
// Copyright (c) 2022 Igara Studio S.A.
// Copyright (c) 2017 David Capello // Copyright (c) 2017 David Capello
// //
// This file is released under the terms of the MIT license. // This file is released under the terms of the MIT license.
@ -61,12 +62,12 @@ int Slice::getMemSize() const
void Slice::insert(const frame_t frame, const SliceKey& key) void Slice::insert(const frame_t frame, const SliceKey& key)
{ {
m_keys.insert(frame, new SliceKey(key)); m_keys.insert(frame, std::make_unique<SliceKey>(key));
} }
void Slice::remove(const frame_t frame) void Slice::remove(const frame_t frame)
{ {
delete m_keys.remove(frame); m_keys.remove(frame);
} }
const SliceKey* Slice::getByFrame(const frame_t frame) const const SliceKey* Slice::getByFrame(const frame_t frame) const