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 5 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::unique_ptr<Message>(HandleIncomingMessage(
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::unique_ptr<Message>(
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);
const std::unique_ptr<RawMessage> result_message(
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 All @@ -218,9 +218,8 @@ class MobileMessageHandlerTest : public testing::Test {
void TestHandlingOutgoingMessageProtocolWithBinaryData(
const uint32_t protocol_version) {
// Arrange
application_manager::BinaryData* bin_dat =
new application_manager::BinaryData;
bin_dat->push_back('\a');
application_manager::BinaryData bin_dat;
bin_dat.push_back('\a');

const uint32_t function_id = 247u;
const uint32_t correlation_id = 92u;
Expand All @@ -231,14 +230,14 @@ class MobileMessageHandlerTest : public testing::Test {
correlation_id,
connection_key,
data,
bin_dat);
&bin_dat);
// Act
RawMessage* result_message =
MobileMessageHandler::HandleOutgoingMessageProtocol(message_to_send);
const std::unique_ptr<RawMessage> result_message(
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);
sizeof(uint8_t) * full_data.size() + bin_dat.size() * sizeof(uint8_t);

// Checks
EXPECT_EQ(protocol_version, result_message->protocol_version());
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);
const std::unique_ptr<application_manager::Message> ptr(
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::unique_ptr<application_manager::Message>(
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);
const std::unique_ptr<RawMessage> ptr(
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,66 @@ 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

}

test_submenu_map.clear();

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

test_commands_map.clear();

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

test_choiceset_map.clear();

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

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

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

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

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

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;
}
}

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 +502,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 +526,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 +559,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 +680,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
Original file line number Diff line number Diff line change
Expand Up @@ -900,7 +900,8 @@ class CacheManager : public CacheManagerInterface {
* @brief Checks snapshot initialization and initializes to default values, if
* necessary
*/
void CheckSnapshotInitialization();
void CheckSnapshotInitialization(
std::shared_ptr<policy_table::Table>& snapshot);

/**
* @brief Calculates difference between two provided custom vehicle data items
Expand Down Expand Up @@ -941,7 +942,6 @@ class CacheManager : public CacheManagerInterface {

private:
std::shared_ptr<policy_table::Table> pt_;
std::shared_ptr<policy_table::Table> snapshot_;
std::shared_ptr<PTRepresentation> backup_;
bool update_required;
typedef std::set<std::string> UnpairedDevices;
Expand Down
Loading