Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/SDL memory leaks #41

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/components/application_manager/src/application_data_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,21 @@ DynamicApplicationDataImpl::~DynamicApplicationDataImpl() {
show_command_ = NULL;
}

if (keyboard_props_) {
delete keyboard_props_;
keyboard_props_ = nullptr;
}

if (menu_title_) {
delete menu_title_;
menu_title_ = nullptr;
}

if (menu_icon_) {
delete menu_icon_;
menu_icon_ = nullptr;
}

if (tbt_show_command_) {
delete tbt_show_command_;
tbt_show_command_ = NULL;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,7 @@ HMICapabilitiesImpl::~HMICapabilitiesImpl() {
delete phone_capability_;
delete video_streaming_capability_;
delete rc_capability_;
delete seat_location_capability_;
}

bool HMICapabilitiesImpl::VerifyImageType(const int32_t image_type) const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,8 @@ TEST_F(ApplicationManagerImplTest,
app_manager_impl_->OnServiceStartedCallback(
device_handle, session_key, service_type, &input_params);

bson_object_deinitialize(&input_params);

// check: return value is true and list is empty
EXPECT_TRUE(result);
EXPECT_TRUE(rejected_params.empty());
Expand Down Expand Up @@ -806,6 +808,8 @@ TEST_F(ApplicationManagerImplTest,
app_manager_impl_->OnServiceStartedCallback(
device_handle, session_key, service_type, &input_params);

bson_object_deinitialize(&input_params);

// check: return value is false
EXPECT_FALSE(result);

Expand Down Expand Up @@ -854,6 +858,8 @@ TEST_F(ApplicationManagerImplTest,
app_manager_impl_->OnServiceStartedCallback(
device_handle, session_key, service_type, &input_params);

bson_object_deinitialize(&input_params);

// check: return value is true and list is empty
EXPECT_TRUE(result);
EXPECT_TRUE(rejected_params.empty());
Expand Down Expand Up @@ -939,6 +945,8 @@ TEST_F(ApplicationManagerImplTest,
app_manager_impl_->OnServiceStartedCallback(
device_handle, session_key, service_type, &input_params);

bson_object_deinitialize(&input_params);

// check: return value is true and list is empty
EXPECT_TRUE(result);
EXPECT_TRUE(rejected_params.empty());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,15 @@ using namespace mobile_apis;
class ResumptionDataTest : public ::testing::Test {
protected:
ResumptionDataTest()
: kCountOfCommands_(5u)
: help_prompt_(nullptr)
, timeout_prompt_(nullptr)
, vr_help_(nullptr)
, vr_help_title_(nullptr)
, vr_synonyms_(nullptr)
, keyboard_props_(nullptr)
, menu_title_(nullptr)
, menu_icon_(nullptr)
, kCountOfCommands_(5u)
, kCountOfChoice_(2u)
, kCountOfChoiceSets_(4u)
, kCountOfSubmenues_(3u)
Expand All @@ -77,6 +85,8 @@ class ResumptionDataTest : public ::testing::Test {
, ivilock_ptr_(std::make_shared<sync_primitives::Lock>())
, window_params_map_lock_ptr_(std::make_shared<sync_primitives::Lock>()) {
}

~ResumptionDataTest();
// Check structure in saved application
void CheckSavedApp(sm::SmartObject& saved_data);
// Set data for resumption
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,8 @@ class MobileMessageHandlerTest : public testing::Test {

// Act
size_t payload_size = data.size();
Message* message = HandleIncomingMessage(
protocol_version, json_plus_binary_data, payload_size);
const auto message = std::shared_ptr<Message>(HandleIncomingMessage(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Can't see any sharing here. What is teh reason to create shared_ptr?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in:be4a738

protocol_version, json_plus_binary_data, payload_size));

// Checks
EXPECT_EQ(data, message->json_message());
Expand All @@ -148,8 +148,8 @@ class MobileMessageHandlerTest : public testing::Test {
// Arrange
size_t payload_size = data.size();
size_t full_data_size = data.size() + PROTOCOL_HEADER_V2_SIZE;
Message* message =
HandleIncomingMessage(protocol_version, data, payload_size);
const auto message = std::shared_ptr<Message>(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Can't see any sharing here. What is teh reason to create shared_ptr?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HandleIncomingMessage(protocol_version, data, payload_size));

// Checks
EXPECT_EQ(data, message->json_message());
Expand Down Expand Up @@ -197,8 +197,8 @@ class MobileMessageHandlerTest : public testing::Test {
MobileMessage message_to_send = CreateMessageForSending(
protocol_version, function_id, correlation_id, connection_key, data);
// Act
RawMessage* result_message =
MobileMessageHandler::HandleOutgoingMessageProtocol(message_to_send);
std::shared_ptr<RawMessage> result_message(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Can't see any sharing here. What is the reason to create shared_ptr?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in:be4a738

MobileMessageHandler::HandleOutgoingMessageProtocol(message_to_send));

std::vector<uint8_t> full_data = joiner<std::vector<uint8_t> >(
binary_header, binary_header + PROTOCOL_HEADER_V2_SIZE, data);
Expand Down Expand Up @@ -233,13 +233,15 @@ class MobileMessageHandlerTest : public testing::Test {
data,
bin_dat);
// Act
RawMessage* result_message =
MobileMessageHandler::HandleOutgoingMessageProtocol(message_to_send);
std::shared_ptr<RawMessage> result_message(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Can't see any sharing here. What is the reason to create shared_ptr?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in:be4a738

MobileMessageHandler::HandleOutgoingMessageProtocol(message_to_send));
std::vector<uint8_t> full_data = joiner<std::vector<uint8_t> >(
binary_header, binary_header + PROTOCOL_HEADER_V2_SIZE, data);
size_t full_size =
sizeof(uint8_t) * full_data.size() + bin_dat->size() * sizeof(uint8_t);

delete bin_dat;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Why do you use heap? Is it possible to use stack for bin_dat? ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in:be4a738


// Checks
EXPECT_EQ(protocol_version, result_message->protocol_version());
EXPECT_EQ(connection_key, result_message->connection_key());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,10 @@ TEST(MobileMessageHandlerTestV1Test,
data_json.length(),
false);

application_manager::Message* ptr =
MobileMessageHandler::HandleIncomingMessageProtocol(message);
std::shared_ptr<application_manager::Message> ptr(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Can't see any sharing here. What is the reason to create shared_ptr?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in:be4a738

MobileMessageHandler::HandleIncomingMessageProtocol(message));

ASSERT_TRUE(ptr);
ASSERT_TRUE(ptr.get());

EXPECT_EQ(connection_key_p1, ptr->connection_key());
EXPECT_EQ(protocol_version_1, ptr->protocol_version());
Expand All @@ -98,10 +98,10 @@ TEST(MobileMessageHandlerTestV1Test,
full_data.length(),
false);

application_manager::Message* ptr =
MobileMessageHandler::HandleIncomingMessageProtocol(message);
const auto ptr = std::shared_ptr<application_manager::Message>(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Can't see any sharing here. What is the reason to create shared_ptr?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in:be4a738

MobileMessageHandler::HandleIncomingMessageProtocol(message));

ASSERT_TRUE(ptr);
ASSERT_TRUE(ptr.get());

EXPECT_EQ(connection_key_p1, ptr->connection_key());
EXPECT_EQ(protocol_version_1, ptr->protocol_version());
Expand All @@ -121,10 +121,10 @@ TEST(MobileMessageHandlerTestV1Test,
message->set_json_message(data_json);
message->set_connection_key(connection_key_p1);

RawMessage* ptr =
MobileMessageHandler::HandleOutgoingMessageProtocol(message);
std::shared_ptr<RawMessage> ptr(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Can't see any sharing here. What is the reason to create shared_ptr?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in:be4a738

MobileMessageHandler::HandleOutgoingMessageProtocol(message));

ASSERT_TRUE(ptr);
ASSERT_TRUE(ptr.get());

EXPECT_EQ(connection_key, ptr->connection_key());
EXPECT_EQ(static_cast<uint32_t>(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,52 @@ using ::testing::Return;
using ::testing::ReturnPointee;
using ::testing::ReturnRef;

ResumptionDataTest::~ResumptionDataTest() {
for (auto& submenu : test_submenu_map) {
delete submenu.second;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov why do we dlete only second item? Don't we clear the entire map?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in:be4a738

}

for (auto& command : test_commands_map) {
delete command.second;
}

for (auto& choiceset : test_choiceset_map) {
delete choiceset.second;
}

if (help_prompt_) {
delete help_prompt_;
}

if (timeout_prompt_) {
delete timeout_prompt_;
}

if (vr_help_) {
delete vr_help_;
}

if (vr_help_title_) {
delete vr_help_title_;
}

if (vr_synonyms_) {
delete vr_synonyms_;
}

if (keyboard_props_) {
delete keyboard_props_;
}

if (menu_title_) {
delete menu_title_;
}

if (menu_icon_) {
delete menu_icon_;
}
}

void ResumptionDataTest::CheckSavedApp(sm::SmartObject& resume_app_list) {
EXPECT_EQ(policy_app_id_, resume_app_list[am::strings::app_id].asString());
EXPECT_EQ(grammar_id_, resume_app_list[am::strings::grammar_id].asUInt());
Expand Down Expand Up @@ -442,7 +488,17 @@ void ResumptionDataTest::SetMenuTitleAndIcon() {

sm::SmartObject sm_title;
sm_title = "test title";

if (menu_title_) {
delete menu_title_;
}

menu_title_ = new sm::SmartObject(sm_title);

if (menu_icon_) {
delete menu_icon_;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Smart pointers for all such items?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
No, because this field we using int the ReturnPointee

}

menu_icon_ = new sm::SmartObject(sm_icon);
}

Expand All @@ -456,14 +512,24 @@ void ResumptionDataTest::SetHelpAndTimeoutPrompt() {
help_prompt[i][am::strings::text] = "help prompt name" + std::string(numb);
help_prompt[i][am::strings::type] = SpeechCapabilities::PRE_RECORDED;
}

if (help_prompt_) {
delete help_prompt_;
}

help_prompt_ = new sm::SmartObject(help_prompt);

for (uint i = 0; i < tts_chunks_count; ++i) {
char numb[12];
std::snprintf(numb, 12, "%d", i);
timeout_prompt[i][am::strings::text] = "timeout test" + std::string(numb);
timeout_prompt[i][am::strings::type] = SpeechCapabilities::SC_TEXT;
}

if (timeout_prompt_) {
delete timeout_prompt_;
}

timeout_prompt_ = new sm::SmartObject(timeout_prompt);
}

Expand All @@ -479,7 +545,16 @@ void ResumptionDataTest::SetVRHelpTitle() {
vr_help[i][am::strings::position] = i;
}

if (vr_help_) {
delete vr_help_;
}

vr_help_ = new sm::SmartObject(vr_help);

if (vr_help_title_) {
delete vr_help_title_;
}

vr_help_title_ = new sm::SmartObject(vr_help_title);
}

Expand Down Expand Up @@ -591,6 +666,11 @@ void ResumptionDataTest::SetKeyboardProperties() {
keyboard[am::strings::auto_complete_text] = "complete";
keyboard[am::strings::limited_character_list][0] = "y";
keyboard[am::strings::limited_character_list][1] = "n";

if (keyboard_props_) {
delete keyboard_props_;
}

keyboard_props_ = new sm::SmartObject(keyboard);
}

Expand Down
7 changes: 7 additions & 0 deletions src/components/include/utils/threads/async_runner.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ class AsyncRunner {
public:
AsyncRunnerDelegate();

~AsyncRunnerDelegate();

/**
* @brief threadMain runs delegates queue handling.
*/
Expand Down Expand Up @@ -111,6 +113,11 @@ class AsyncRunner {
*/
void waitForDelegate();

/**
* @bref clearDelegateQueue delete leftover delegates in the queue
*/
void clearDelegateQueue();
Copy link

@AByzhynar AByzhynar Jun 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov Wrong naming(Coding style). The function name should start with a capital letter

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AByzhynar
Fixed in:dff6e3e


std::queue<threads::ThreadDelegate*> delegates_queue_;
sync_primitives::ConditionalVariable delegate_notifier_;
sync_primitives::Lock delegates_queue_lock_;
Expand Down
5 changes: 4 additions & 1 deletion src/components/protocol_handler/src/protocol_handler_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1835,7 +1835,10 @@ void ProtocolHandlerImpl::NotifySessionStarted(
}

std::shared_ptr<BsonObject> start_session_ack_params(
new BsonObject(), [](BsonObject* obj) { bson_object_deinitialize(obj); });
new BsonObject(), [](BsonObject* obj) {
bson_object_deinitialize(obj);
delete obj;
});
bson_object_initialize_default(start_session_ack_params.get());
// when video service is successfully started, copy input parameters
// ("width", "height", "videoProtocol", "videoCodec") to the ACK packet
Expand Down
4 changes: 4 additions & 0 deletions src/components/utils/src/sqlite_wrapper/sql_database.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ bool SQLDatabase::Open() {
return true;
error_ = sqlite3_open(get_path().c_str(), &conn_);
if (error_ != SQLITE_OK) {
// Whether or not an error occurs when it is opened, resources associated
// with the database connection handle should be released by passing it to
// sqlite3_close() when it is no longer required.
sqlite3_close(conn_);
conn_ = NULL;
}
return error_ == SQLITE_OK;
Expand Down
20 changes: 20 additions & 0 deletions src/components/utils/src/threads/async_runner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ AsyncRunner::~AsyncRunner() {

AsyncRunner::AsyncRunnerDelegate::AsyncRunnerDelegate() : stop_flag_(false) {}

AsyncRunner::AsyncRunnerDelegate::~AsyncRunnerDelegate() {
clearDelegateQueue();
}

void AsyncRunner::AsyncRunnerDelegate::processDelegate() {
if (!delegates_queue_.empty()) {
delegates_queue_lock_.Acquire();
Expand All @@ -88,6 +92,22 @@ void AsyncRunner::AsyncRunnerDelegate::waitForDelegate() {
}
}

void AsyncRunner::AsyncRunnerDelegate::clearDelegateQueue() {
if (!delegates_queue_.empty()) {
std::queue<threads::ThreadDelegate*> empty_queue;
delegates_queue_lock_.Acquire();
std::swap(delegates_queue_, empty_queue);
delegates_queue_lock_.Release();
do {
auto delegate = empty_queue.front();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SNiukalov The name looks bad. How can we pop and delete something from an empty queue? ))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty_queue.pop();
if (delegate) {
delete delegate;
}
} while (!empty_queue.empty());
}
}

void AsyncRunner::AsyncRunnerDelegate::threadMain() {
LOG4CXX_AUTO_TRACE(logger_);
while (!stop_flag_) {
Expand Down
Loading