A few memory leaks

testing your patch with Inkscape 0.48+devel r10112 on OS X 10.5.8 (i386) - command line version - I get these console messages when launching Inkscape:

(inkscape:97531): Pango-WARNING **: Invalid UTF-8 string passed to pango_layout_set_text()

(inkscape:97531): Pango-WARNING **: Invalid UTF-8 string passed to pango_layout_set_text()

(inkscape:97531): Pango-WARNING **: Invalid UTF-8 string passed to pango_layout_set_text()

(inkscape:97531): Pango-WARNING **: Invalid UTF-8 string passed to pango_layout_set_text()

A new version, without the piece that could be affecting pango, and with a few more fixes that I had missed.

Testing second patch with Inkscape 0.48+devel r10112 on OS X 10.5.8 (i386) - command line version, gtk+/x11:
Inkscape hangs at launch and consumes all memory it gets. I tested only once and killed the process after it was apparent that it would not even manage to open the initial window (IIRC 'Virtual Memory' in 'Activity Monitor' had reached > 1GB, usually it's around 105 MB just after launching) .
After reverting the changes and rebuilding, inkscape launches without issues.

Using the new patch (I didn't test the original), I get the same console warnings as mentioned earlier, but Inkscape runs fine here and appears to use less memory (49 megs here) on startup. I'm on Ubuntu Natty.

Hang (with high memory usage) repeatable with my builds on OS X 10.5.8 (i386), patch reverted and reapplied to r10115.

After renaming the preferences file to start with default settings, Inkscape crashes on launch (backtrace attached).

Working through a few issues here. First problem is that the g_value_unset() calls are clearing the object while we still hold pointers to the internal value data it has. Need to wait and do that later.

Inkscape 0.48+devel r10115 on OS X 10.5.8 (i386), gtk2 @2.24.3_0+x11

First I tested the patch by splitting it into individual chunks:
- none of the chunks by itself triggers the failure to launch on my system
- the changes to ege-adjustment-action.cpp trigger 4 pango warnings

Then I patched all files that had also been in the first version of the patch:
   src/ege-adjustment-action.cpp
   src/ege-output-action.cpp
   src/ege-select-one-action.cpp
   src/ink-action.cpp
and added the new changes from the second patch one by one (launching inkscape in-between for testing):
   src/inkscape.cpp
   src/interface.cpp
   src/preferences.cpp
   src/desktop.cpp
   src/extension/internal/filter/filter-file.cpp

all work fine except when adding the changes to
   src/extension/implementation/xslt.cpp

I have no clue why that happens.

Additional issues with the patch (except for the XSLT extensions) applied (r10115):
- repeated occurrances of the pango warnings when switching tools
- some of the labels and tooltips on the various tool controls bars are no longer correctly rendered or visible at all
- some spinboxes are missing (e.g. for the rectangle tool),
- hovering over some of the spinboxes to show the tooltip create additional warnings like (e.g. 'Angle', calligraphic brush):

(inkscape:6975): Gtk-WARNING **: Failed to set text from markup due to error parsing markup: Error on line 1 char 43: Invalid UTF-8 encoded text in name - not valid '8\u001c\xb0\u0005\xe0\x83\xa2\u00058\u001c\xb0\u0005'

@~suv - Do you notice something specific about the xslt extensions with the patched version?
I've tested this patch alone, and it seems to work as expected (except that there are other unload issues that apparently existed before).

JazzyNico wrote
> I've tested this patch alone, and it seems to work as expected

As described in comment #8, each chunk of the patch applied separately works fine. When combining all chunks, it's with the changes to 'src/extension/implementation/xslt.cpp' that I can't launch inkscape anymore. I don't know why this happens… nor did I test _all_ variants of combining the different chunks to be 100% certain which combination triggers my problem.

Do you also see the issues with the labels and spinboxes on the various controls bars when applying the full patch?

I do see the label issue for example with the rectangle tool. The labels are showing up as the same as the contents of their respective spinboxes (on load only, the labels don't change when the spinbox contents change).

JazzyNico wrote
> @~suv - Do you notice something specific about the xslt
> extensions with the patched version?

To clarify - I didn't test to import/export a file format based on an XSLT extension - my problem is that with that specific chunk of the patch applied, inkscape hangs at launch and grabs all memory until it crashes with

GLib-ERROR **: gmem.c:239: failed to allocate 4294967295 bytes
aborting...

if I don't interrupt earlier.

Seeing an overt visible problem does not mean all is well. Running valgrind with memcheck is the main way to be sure the parts are good.

For the xslt change, it reverses the logic for when things are freed or not. Might not be good. Again, this was code that had several issues I think cause problems, so it's not surprising that we're hitting them here. ( multiple statements collapsed to a single line, early return, etc.)

For the label issue, I think I see where I was wrong. A few of the g_value_unset are called too soon.
Example in ege-output-action.cpp:

        g_value_init( &value, G_TYPE_STRING );
        g_object_get_property( G_OBJECT(action), "short_label", &value );
        const gchar* sss = g_value_get_string( &value );
        g_value_unset( &value );

g_value_unset should not be called before we are done with sss, since sss is just a reference.

Is it more convenient if I split the patch into individual files?

patch-group1.diff contains the same changes as before for the following files:

=== modified file 'src/desktop.cpp'
=== modified file 'src/ege-select-one-action.cpp'
=== modified file 'src/extension/internal/filter/filter-file.cpp'
=== modified file 'src/ink-action.cpp'
=== modified file 'src/inkscape.cpp'
=== modified file 'src/interface.cpp'
=== modified file 'src/preferences.cpp'

and updated patches for:

=== modified file 'src/ege-adjustment-action.cpp'
=== modified file 'src/ege-output-action.cpp'

I have left out changes mentioned before to:

src/libnrtype/FontFactory.cpp
src/extension/implementation/xslt.cpp

patch-group1 now correctly displays labels... however, the pango warnings in the console are still there and the memory usage is now 40megs greater for me than the previous patch. I am unsure if the memory difference is because you were on to something with the FontFactory & xslt files or because of the incorrect stuff with toolbar labels in the previous patch... either way, a pretty significant difference.

patch-group1.diff tested with Inkscape 0.48+devel r10115 on OS X 10.5.8:
- Inkscape launches ok,
- pango warnings (apparently depend on the font used in the GTK theme)
- labels for the sliders on the controls bars show either gibberish or a generic name

I branched out of trunk at r10068 and need to check if this has any impact.
~suv, I don't get any of the two things you attached a screenshot for.
ScisclaC, here is the patch for FontFactory only. I might have removed it for the wrong reasons (pango warnings...).

Okay, the FontFactory patch does give memory saving on startup of roughly 29 megs from current trunk. I do see part of what ~suv described (no gibberish though)... the slider labels are not what should be displayed as none of the labels should be "BlahBlahAction".

A great way to compare things is to expand the window so all sliders on the spray tool show and make note of the "Action" names, then make the window as small as possible and use the arrow on the tool controls bar that shows the non-visible widgets and the correct names will show there.

Gellule wrote
> ~suv, I don't get any of the two things you attached a screenshot for.

Here's another screenshot of the spray tool, after changing the GUI font to 'Lucida Grande': this time the font name appears as string in some of the labels… (repeatable - looks the same whether Inkscape is launched from gdb or normally).

Could you apply this patch in addition to the group1 patch, and see if the mangled text goes away?

labels appear fixed on sliders here after the new patch

I also forgot to report back, the FontFactory patch stopped the pango warnings in the console.

r10115+patch-group1.diff+patch-ege-adjustment-action.cpp.diff on OS X 10.5.8 (i386):

- launch ok
- no more pango warnings
- labels of the sliders on the controls bar ok

(patch-FontFactory.diff not yet applied)

No regressions noticed with patch-FontFactory.diff (r10115, OS X 10.5.8 (i386))

~suv: Any difference in memory usage?

Working through a few cleanup passes on the patch

> ~suv: Any difference in memory usage?

AFAICT not really. How exactly do you measure and compare "appears to use less memory (49 megs here) on startup"?

If I launch the patch and the unpatched version of r10115 side-by-side, Activity monitor shows minor differences of about 0.5 MB Real Memory and 1MB Virtual memory.

For me? I use the standard Gnome System Monitor... Before patching it's ~87MB of ram after launching my trunk build via CLI... the patched version now shows 56MB on launch under the same circumstances. My tests are not scientific in any way. I probably need to do cold boots between launches of specific binaries w/ nothing else loaded. I will test and report back tomorrow.

There is a better way of fixing a few of these leaks. I see in GtkAction things like gtk_action_get_label or gtk_action_get_short_label, available since 2.16. If GTK version >=2.16 could be made a requirement, some code could be cleaned-up. See the attached patch, that fixes the memory leak for ege-adjustment-action.cpp and simplifies the code with the functions mentioned above.

ScislaC wrote
> Before patching it's ~87MB of ram
> the patched version now shows 56MB on launch

for me:
  patched r10115: 58.72 MB (Real Memory) 102.44 MB (Virtual Memory)
unpatched r10117: 58.47 MB (Real Memory) 101.43 MB (Virtual Memory)

oops - revert those numbers:
  patched r10115: 58.47 MB (Real Memory) 101.43 MB (Virtual Memory)
unpatched r10117: 58.72 MB (Real Memory) 102.44 MB (Virtual Memory)

Got things cleaned up a little and reconciled.

Applied to trunk as revision 10118.