[PATCH] Solve memory leaks in Deskbar and some replicants.

[Barrett]

I have just sanitized a bit the situation with malloc/free and BMessage ownership things.

I've monitored Deskbar with top for about 2 hours and here the memory decrease always to a lower bound of 21%.

[ttcoder]

Is it completely portable to write

 char truncatedLabel[strlen(label) + 4]; 

I seem to remember runtime-specified array sizes are a gcc specific extension, not supported on all compilers (?)

Cleanups look good other than that (though maybe it's not my role as an outsider to comment on them) and patch 3 especially looks like a winner :-)

We will have to collect more data on our side to see who is leaking and on what scale (we're tied up with so many things currently, sigh)

[pdziepak]

VLAs are a C99 feature so I would expect most compilers to support them even when compiling C++ code (well, at least gcc and clang do), although a warning may be emitted. Moreover, there have been some attempts to add VLAs to C++, but, unfortunately, they didn't made it into C++14. Maybe things will change with C++17.

Anyway, raw VLAs are tricky and I don't think that the code from the first patch is performance critical, so it probably is better to leave it as it was. (And in case it actually is performance critical, some variation of small_vector<> may be useful).

[anevilyak]

Patch 2 is broken in several ways:

-	BMessage archivedView;
-	if (message->FindMessage("view", &archivedView) == B_OK) {
+	BMessage* archivedView = NULL;
+	if (message->FindMessage("view", archivedView) == B_OK) {

This code was correct before, FindMessage() expects a pointer to an already allocated message object, which the code was doing just fine prior to this change. Consequently, the following code is also passing around and deleting a NULL pointer. The change in BShelf is similarly broken.

The ProcessController and Volume Control patches are correct, but unlikely to be the culprits behind any real leaks, since the ProcessController code in question is only invoked when run as a standalone app, and even then only once (ReadyToRun() is only called at startup). Likewise, the VolumeControl code in question is only called when initially instantiating the replicant, and as such can't really be responsible for any extended leaks. The TimeView change is correct, but the same situation applies, since it's only invoked when opening the preferences via the context menu.

As such, I really don't see how these patches fix any major memory leak issues with Deskbar.

[Barrett]

From what i know in standard c++, the PODs arrays are supported just as in C99. Anyway this was obviously just a cleanup change nothing, but i see often changes like this in the commit-list so i supposed it was appreciated.

@anevilyak thanks for noticing, i should have looked better at the implementation. Anyway i know that i've not solved the big leaks, i'm just starting, but they are still leaks, i've fixed them once i have seen the problems. Unfortunately the Deskbar code is very confusing sometimes and before or after will need a refactor.

[Barrett]

Very sorry, forgot to commit and format-patch.

[korli]

-		BMessage* dragMessage = NULL;
-		if (message->HasMessage("be:drag_message")) {
-			dragMessage = new BMessage();
-			message->FindMessage("be:drag_message", dragMessage);
-		}
+		BMessage dragMessage;
+		if (message->HasMessage("be:drag_message"))
+			message->FindMessage("be:drag_message", &dragMessage);

With this patch, MouseMoved() is later called with &dragMessage, which is wrong. If there is no drag message, the third argument of MouseMoved() should be NULL.

[pulkomandy]

Instead of using VLA arrays, you can use BStackOrHeapArray. The problem with VLAs is that you can get a stack overflow if the data is big enough, as they are allocated on the stack. BStackOrHeapArray will only allocate small enough data on the stack (you set the threshold as a template parameter), and will allocate extra memory if needed. It takes care of freeing it when it goes out of scope.

Alternatively, a BAutoDeleter could be used (but in that case the data will never be on the stack).

[Barrett]

Thanks a lot to all, understood the lesson.

[korli]

0001:

    BStackOrHeapArray<char, 128> truncatedLabel(strlen(label) + 4); 

truncatedLabel.IsValid() should be called to check that the allocation succeeded. truncatedLabel != NULL should be replaced with truncatedLabel.IsValid().

0002:

    if (replicant.FindMessage("message", replMsg) == B_OK) 
        if (AddReplicant(replMsg, point) != B_OK) 
            delete replMsg;

When the first condition fails, replMsg is leaked.

[Barrett]

BTW why Unset and Delete do the same thing?

[Barrett]

Is there already some other reason for not applying it? So that i can make my local branch more manteinable.

[pulkomandy]

0001: isn't there a way to use a BString instead of this? If TruncateLabel was taking a BString as a parameter and returning another BString, I think the code would be simpler.

0002: It looks like some of the new+BAutoDeleter could be replaced with simply allocating the BMessage on the stack. Something like this:

BMessage archivedView;
BMessage* archivedViewPtr = &archivedView;
if (message->FindMessage("view", &archivedViewPtr) == B_OK) {

These are only code readability issues, so the patch could be applied still (not at home yet, can't do it myself).

[Barrett]

Using the stack can be done if the Add* functions involved copy the message instead of taking ownership but this also imply changes in how BShelf is operating.

[pulkomandy]

I am referring to messages that are already wrapped in an ObjectDeleter. If I'm not mistaen the ObjectDeleter will delete the object as soon as it goes out of scope, similar to what allocating on the stack does. What is the difference then?

[Barrett]

0001: it's the first thing i've think of, but changing this in BMenuItem isn't breaking eventual classes using it? Since it was there from BeOS times. I could still add another version and composite the first into it, but is it worth the effort? No problem for me to add another version though, i have just think of it being unneeded.

0002: The functions acquire ownership only on success and i am detaching it from the deleter when the Add* return B_OK, otherwise the deleter will retain ownership and do what you said.

[pulkomandy]

0001: ok, I missed that. we can leave it as it is now then. 0002: a function that takes ownership only on success is difficult to understand. If we can't rearrange that (for example, can the function delete the object directly in case of failure?), we should at least add a comment as to why the code has to do these unusual things with the ObjectDeleter.

[Barrett]

The problem is if i set BShelf::AddReplicant to delete the message there's a condition for double free if some code is deleting it on it's own, or it may be that some code around is using the message on failure and result in a segmentation fault.

[pulkomandy]

Ok, in this case we should add a comment to explain the situation.

[Barrett]

Confused Unset with Detach.

[pulkomandy]

Applied in hrev49033.

Reminder for next time: git commit messages in your patches should have the first line <70 characters, a blank line, then additional details as needed. Your patches have everything on a single line instead.

Thanks for working on this!