Fix/SDL memory leaks #41

SNiukalov · 2020-05-26T07:56:31Z

Fixes #Jira task

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

ATF scripts & Valgrind

Memory Leaks Report

Summary

Fix memory leaks in sdl and unit tests

CLA

I have signed the CLA

LuxoftAKutsan · 2020-05-26T09:56:21Z

Could you please attach information about leaks?
Statistics would be good.

AByzhynar · 2020-05-27T12:19:30Z

src/components/application_manager/test/mobile_message_handler_test.cc

@@ -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(


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

@AByzhynar
Fixed in:be4a738

AByzhynar · 2020-05-27T12:19:51Z

src/components/application_manager/test/mobile_message_handler_test.cc

@@ -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>(


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

@AByzhynar @LitvinenkoIra
Fixed in:be4a738

AByzhynar · 2020-05-27T12:20:25Z

src/components/application_manager/test/mobile_message_handler_test.cc

@@ -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(


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

@AByzhynar
Fixed in:be4a738

AByzhynar · 2020-05-27T12:21:22Z

src/components/application_manager/test/mobile_message_handler_test.cc

@@ -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(


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

@AByzhynar
Fixed in:be4a738

AByzhynar · 2020-05-27T12:24:22Z

src/components/application_manager/test/mobile_message_handler_test.cc

    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;


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

@AByzhynar
Fixed in:be4a738

AByzhynar · 2020-05-27T12:25:35Z

src/components/application_manager/test/mobile_message_handler_v1_test.cc

@@ -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(


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

@AByzhynar
Fixed in:be4a738

AByzhynar · 2020-05-27T12:27:35Z

src/components/application_manager/test/resumption/resumption_data_test.cc

@@ -52,6 +52,52 @@ using ::testing::Return;
 using ::testing::ReturnPointee;
 using ::testing::ReturnRef;

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


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

@AByzhynar
Fixed in:be4a738

AByzhynar · 2020-05-27T12:29:54Z

src/components/application_manager/test/resumption/resumption_data_test.cc

  menu_title_ = new sm::SmartObject(sm_title);
+
+  if (menu_icon_) {
+    delete menu_icon_;


@SNiukalov Smart pointers for all such items?

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

AByzhynar · 2020-05-27T12:33:02Z

src/components/utils/src/threads/async_runner.cc

+    std::swap(delegates_queue_, empty_queue);
+    delegates_queue_lock_.Release();
+    do {
+      auto delegate = empty_queue.front();


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

@AByzhynar @LitvinenkoIra
Fixed in:ed572e6

AByzhynar · 2020-05-27T12:45:39Z

src/components/utils/test/async_runner_test.cc

  {
    std::shared_ptr<AsyncRunner> async_runner =
        std::make_shared<AsyncRunner>("test");
    async_runner->Stop();
-    async_runner->AsyncRun(&mock_thread_delegate);
+    async_runner->AsyncRun(mock_thread_delegate);


@SNiukalov Is Async_runner responsible for thread delegate destruction?

@AByzhynar
Yes, Async_runner responsible for thread delegate destruction

SNiukalov · 2020-06-04T07:49:26Z

@LitvinenkoIra @LuxoftAKutsan

AByzhynar · 2020-06-09T07:26:11Z

src/components/include/utils/threads/async_runner.h

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


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

@AByzhynar
Fixed in:dff6e3e

AByzhynar · 2020-06-09T07:33:05Z

src/components/utils/src/threads/async_runner.cc

+  if (!delegates_queue_.empty()) {
+    std::queue<threads::ThreadDelegate*> queue_to_delete;
+    delegates_queue_lock_.Acquire();
+    std::swap(delegates_queue_, queue_to_delete);


@SNiukalov No appropriate header files included

@AByzhynar
Fixed in:dff6e3e

AByzhynar · 2020-06-09T07:33:10Z

src/components/utils/src/threads/async_runner.cc

@@ -88,6 +92,22 @@ void AsyncRunner::AsyncRunnerDelegate::waitForDelegate() {
  }
 }

+void AsyncRunner::AsyncRunnerDelegate::clearDelegateQueue() {
+  if (!delegates_queue_.empty()) {
+    std::queue<threads::ThreadDelegate*> queue_to_delete;


@SNiukalov No appropriate header files included

@AByzhynar
Header file for the std::queue present in async_runner.h

@SNiukalov It does not matter. You define the entity here - so don't create implicit dependency from header file. Include all headers explicitly

@AByzhynar
I don’t understand why we need duplicates.
This inclusion is present in the header file and also defines the entity.
If you change this header, we will still be forced to change the code in this place. Because it depends on the member of the delegates_queue_ class.
What is the meaning of this inclusion then?

@SNiukalov You defining implicit dependency. This is BAD. You will change something in header e.g. change container from queue to list. What you get is CC file can't be compiled.
Read https://github.com/smartdevicelink/sdl_core/wiki/SDL-Coding-Style-Guide
Don't create dependencies between independent things.

@AByzhynar
Yes, if change container from queue to list. What you get is CC file can't be compiled.
And if make include to cc I get is CC file can't be compiled. :) Because deffirents types :)

Fixed in:e62a8eb & b985f83

AByzhynar · 2020-06-09T07:35:12Z

src/components/utils/test/scope_guard_test.cc

@@ -90,7 +90,7 @@ TEST(ScopeGuardTest, CallObjectFunctionWithParam) {
 TEST(ScopeGuardTest, DismissCallFreeFunctionWithParam) {
  {
    call_with_param_count = 0;
-    char* ptr = new char;
+    char* ptr = const_cast<char*>("dealloc is not called");


@SNiukalov Can you please explain this magic?

@AByzhynar
In this case, we check that the CallFreeFunction will never be called and the memory will not be freed.
But we pass the pointer from the heap and make a memory leak.
I change this behavior and leave a reminder with text: dealloc not called

LuxoftAKutsan

👍

sniukalov added 2 commits May 26, 2020 09:40

SDL fix memory leaks

ae8d848

UTs fix memory leaks

5a07552

SNiukalov requested review from AByzhynar, LuxoftAKutsan, LitvinenkoIra, ydementieiev and mvorobio May 26, 2020 07:56

SNiukalov changed the title ~~Fix/Sdl memory leaks~~ Fix/SDL memory leaks May 26, 2020

AByzhynar suggested changes May 27, 2020

View reviewed changes

SDL fix accumulation memory

5f9174c

ydementieiev approved these changes May 29, 2020

View reviewed changes

sniukalov added 2 commits June 4, 2020 10:23

fixup! UTs fix memory leaks

be4a738

fixup! SDL fix memory leaks

ed572e6

AByzhynar reviewed Jun 9, 2020

View reviewed changes

AByzhynar suggested changes Jun 9, 2020

View reviewed changes

sniukalov added 3 commits June 9, 2020 12:08

fixup! SDL fix memory leaks

dff6e3e

fixup! SDL fix memory leaks

e62a8eb

fixup! SDL fix memory leaks

b985f83

AByzhynar approved these changes Jun 9, 2020

View reviewed changes

mvorobio approved these changes Jun 9, 2020

View reviewed changes

LuxoftAKutsan approved these changes Jun 16, 2020

View reviewed changes

LitvinenkoIra approved these changes Jun 16, 2020

View reviewed changes

Fix/SDL memory leaks #41

Are you sure you want to change the base?

Fix/SDL memory leaks #41

Conversation

SNiukalov commented May 26, 2020 • edited Loading

Risk

Testing Plan

Memory Leaks Report

Summary

CLA

LuxoftAKutsan commented May 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SNiukalov commented Jun 4, 2020 • edited Loading

AByzhynar Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AByzhynar Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

SNiukalov Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

AByzhynar Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

SNiukalov Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuxoftAKutsan left a comment

Choose a reason for hiding this comment

SNiukalov commented May 26, 2020 •

edited

Loading

SNiukalov commented Jun 4, 2020 •

edited

Loading

AByzhynar Jun 9, 2020 •

edited

Loading

AByzhynar Jun 9, 2020 •

edited

Loading

SNiukalov Jun 9, 2020 •

edited

Loading

AByzhynar Jun 9, 2020 •

edited

Loading

SNiukalov Jun 9, 2020 •

edited

Loading