Bug 644824

Created attachment 463103 [details]
Patch to plug more leaks

Even with he upstream patches included, our customer still reports the following leaks:

==1527== 8 bytes in 1 blocks are possibly lost in loss record 5,526 of
 59,799

==1527==    at 0x8020C5E: malloc (vg_replace_malloc.c:236)

==1527==    by 0xE906E07: XtMalloc (Alloc.c:127)

==1527==    by 0xE7D5AF8: XmeTraitSet (Trait.c:204)

==1527==    by 0xE7C97D6: ClassPartInitialize (VendorS.c:830)

==1527==    by 0xE90F2A6: CallClassPartInit (Create.c:85)

==1527==    by 0xE90F29A: CallClassPartInit (Create.c:82)

==1527==    by 0xE90F6F2: XtInitializeWidgetClass (Create.c:193)

==1527==    by 0xE90F7C5: XtInitializeWidgetClass (Create.c:189)

==1527==    by 0xE90FA47: xtWidgetAlloc (Create.c:268)

==1527==    by 0xE90FC2C: xtCreate (Create.c:357)

==1527==    by 0xE9101A4: _XtAppCreateShell (Create.c:698)

==1527==    by 0xE9103DB: XtAppCreateShell (Create.c:725)

==1527==    by 0xE91B759: XtOpenApplication (Initialize.c:990)

==1527==    by 0xE91B800: XtAppInitialize (Initialize.c:1015)

==1527==    by 0x892480C: initialise_toolkit (initialise.c:143)

==1527==    by 0x89439AE: motif__initialise (motif.adb:522)

...

==1527== 8 bytes in 1 blocks are possibly lost in loss record 5,527 of
 59,799

==1527==    at 0x8020C5E: malloc (vg_replace_malloc.c:236)

==1527==    by 0xE906E07: XtMalloc (Alloc.c:127)

==1527==    by 0xE7D5AF8: XmeTraitSet (Trait.c:204)

==1527==    by 0xE7C97F7: ClassPartInitialize (VendorS.c:833)

==1527==    by 0xE90F2A6: CallClassPartInit (Create.c:85)

==1527==    by 0xE90F29A: CallClassPartInit (Create.c:82)

==1527==    by 0xE90F6F2: XtInitializeWidgetClass (Create.c:193)

==1527==    by 0xE90F7C5: XtInitializeWidgetClass (Create.c:189)

==1527==    by 0xE90FA47: xtWidgetAlloc (Create.c:268)

==1527==    by 0xE90FC2C: xtCreate (Create.c:357)

==1527==    by 0xE9101A4: _XtAppCreateShell (Create.c:698)

==1527==    by 0xE9103DB: XtAppCreateShell (Create.c:725)

==1527==    by 0xE91B759: XtOpenApplication (Initialize.c:990)

==1527==    by 0xE91B800: XtAppInitialize (Initialize.c:1015)

==1527==    by 0x892480C: initialise_toolkit (initialise.c:143)

...

==1527== 8 bytes in 1 blocks are possibly lost in loss record 5,528 of
 59,799

==1527==    at 0x8020C5E: malloc (vg_replace_malloc.c:236)

==1527==    by 0xE906E07: XtMalloc (Alloc.c:127)

==1527==    by 0xE7D5AF8: XmeTraitSet (Trait.c:204)

==1527==    by 0xE7C9818: ClassPartInitialize (VendorS.c:836)

==1527==    by 0xE90F2A6: CallClassPartInit (Create.c:85)

==1527==    by 0xE90F29A: CallClassPartInit (Create.c:82)

==1527==    by 0xE90F6F2: XtInitializeWidgetClass (Create.c:193)

==1527==    by 0xE90F7C5: XtInitializeWidgetClass (Create.c:189)

==1527==    by 0xE90FA47: xtWidgetAlloc (Create.c:268)

==1527==    by 0xE90FC2C: xtCreate (Create.c:357)

==1527==    by 0xE9101A4: _XtAppCreateShell (Create.c:698)

==1527==    by 0xE9103DB: XtAppCreateShell (Create.c:725)

==1527==    by 0xE91B759: XtOpenApplication (Initialize.c:990)

==1527==    by 0xE91B800: XtAppInitialize (Initialize.c:1015)

==1527==    by 0x892480C: initialise_toolkit (initialise.c:143)

...


Repeated several times. Plus a few others, all pointing to XmeTraitSet().

It seems that if ClassPartInitialize() is called several times, the Trait is added each time.

Actually, Traits seems to be added to the Hash table in any case without prior checking if the Trait is already present, but are never removed, and that's the case in many parts of the code.

The following patch tries to avoid this. But please beware, this patch has received very light testing, I just checked that a few Motif programs still work and do not crash and burn. 

Given that the same or similar code is to be found in many different sources, the resulting patch is fairly big and touches a lot of sources.

I shall provide a test package to our customer so he can try with his application as well and verify if the leaks are not reported anymore with that.

PS: This patch goes on top of the previous one (it does not replace it).

Created attachment 463485 [details]
Simpler patch to plug Trait leaks

Actually, I think there is a much simpler way to address these Trait leaks.

The traits are stored in a hash table with the key made of the object and the property.

Therefore having more than one entry in that hash table with the same key would not make sense (and would indeed introduce a leak), which means that one object can have only one entry for a given property.

Instead of changing the code in each portion that adds traits, this new patch changes only Xm/Trait.c to check for an existing key before adding the same one again.

Comment on attachment 463485 [details]
Simpler patch to plug Trait leaks

Marking patch as obsolete, it does not address the leak (which is, Traits are added but never removed, even when the objects are destroyed)

I am not entirely convinced the problem reported in comment #1 is a true leak (unlike the initial issue reported in comment #0). A leak would be if new memory was allocated while references to previous allocated memory was lost, ie the memory usage would grow over time.

Here what we see is an allocation that occurs once, it's not freed, but it's not reallocated either.

This is because the traits reported to leak here are set to the widget class, ie the first time a widget of a given class is created, the widget class gets initialized and ClassPartInitialize() is called. ClassPartInitialize() will add traits for that particular widget class and store them in a hash table using a key made of the object (the widget class) and the name of the property.

If the widget is destroyed, the entry is not freed nor removed from the hash table. Yet when a new widget from the same class is created, the same traits are not added again, the previous allocated traits for that particular widget class/property remain and are reused. 

For example, creating a widget and destroying it will add a few Traits, but creating a 1000 of the same type of widgets will not create a 1000 more traits. 

Beside, I see no hook in the API for destroying the widget class so I see no way to remove the traits added at initialization. Of course we see in valgrind that the same sequence in VendorS.c is called more than once, but checking with GDB, we can see that it's for different Widget Classes, there is just one sequence for a given widget class.

To confirm this, run the reproducer for a 1000 iterations and check the leak summary:

==27404== LEAK SUMMARY:
==27404==    definitely lost: 60 bytes in 1 blocks
==27404==    indirectly lost: 0 bytes in 0 blocks
==27404==      possibly lost: 34,844 bytes in 79 blocks
==27404==    still reachable: 549,337 bytes in 3,021 blocks
==27404==         suppressed: 0 bytes in 0 blocks

Redo the same and leave it running for just a just 5 iterations and compare the results:

==27657== LEAK SUMMARY:
==27657==    definitely lost: 60 bytes in 1 blocks
==27657==    indirectly lost: 0 bytes in 0 blocks
==27657==      possibly lost: 34,844 bytes in 79 blocks
==27657==    still reachable: 514,273 bytes in 2,021 blocks
==27657==         suppressed: 0 bytes in 0 blocks

The values for definitely/indirectly/possibly lost are identical, there is no additional leak reported by repeating the creation/destruction of widgets, apparently.

Note, this is only for the problem reported in comment #1, the leak reported initially in comment #0 is real and being addressed by the proposed patch (attachment 454558 [details]) which is the fix from upstream.

This request was evaluated by Red Hat Product Management for
inclusion in the current release of Red Hat Enterprise Linux.
Because the affected component is not scheduled to be updated in the
current release, Red Hat is unfortunately unable to address this
request at this time. Red Hat invites you to ask your support
representative to propose this request, if appropriate and relevant,
in the next release of Red Hat Enterprise Linux.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2011-1451.html