Bug #1043571 “mod->set_param_string memory leak” : Bugs : Inkscape

Kris (kris-degussem) on 2012-09-02

tags:

added: performance

Revision history for this message

Kris (kris-degussem) wrote on 2012-09-02:

#1

With revision 11646, some memory leaks should have been fixed. Could you have a look whether it solves this issue?

Revision history for this message

Kris (kris-degussem) wrote on 2012-09-03:

#2

rev 11646 caused some issues. Reverted temporarily

Revision history for this message

Kris (kris-degussem) wrote on 2012-09-08:

#3

1043571.patch Edit (12.4 KiB, text/plain)

The patch in attachment should fix four memory leaks related to not releasing memory occupied by sp_repr_css_write_string.
Hopefully this patch does not lead to errors such as in the reverted revision.

Can someone check with valgrind whether it solves the above bug? I can not check and I'm not sure ... If it does not solve the issue, then quite probably the culprit is that the memory occupied by share_string on line 375 of xml/simple-node.cpp (new_value = share_string(cleaned_value);) is not released.

Revision history for this message

Kris (kris-degussem) wrote on 2012-09-08:

#4

For completeness the issues referred to in comment 2 are mentioned in bug #1045117 though they should not be present anymore with the patch in comment 3.

jazzynico (jazzynico) on 2012-09-10

Changed in inkscape:
importance:	Undecided → Medium
status:	New → In Progress

Revision history for this message

Kris (kris-degussem) wrote on 2012-09-18:

#5

1043571v2.patch Edit (14.0 KiB, text/plain)

Minor update to patch (some more string class usage) and up to date with trunk r11675.
If no side effects detected in the next two days, I will commit the patch.

Revision history for this message

su_v (suv-lp) wrote on 2012-09-19:

#6

> If no side effects detected in the next two days, I will commit the patch.

None of the issues described in bug #1045117 reproduced with last version of patch on OS X 10.7.4 (dbus builds without failure, no objects get autolocked when imported or newly created, locked objects and layers can be unlocked again). Haven't done other tests beyond this (tested on OS X 10.7.4, 64bit).

Revision history for this message

Kris (kris-degussem) wrote on 2012-09-20:

#7

Patch committed in trunk r11686.
Bug status still to be checked.
David: could you have a look with valgrind if the issue is solved?

Revision history for this message

David Mathog (mathog) wrote on 2012-09-25:

#8

Download full text (4.9 KiB)

I have 11679 as branch lp988601, is the problem patch in or out of that version? setAttribute() is still causing problems, so I am guessing no. But I could not apply the patch from (5) successfully:

patch -p0 </tmp/1043571v2.patch
patching file src/attribute-rel-util.cpp
Reversed (or previously applied) patch detected! Assume -R? [n]
Apply anyway? [n] y
Hunk #1 FAILED at 128.
Hunk #2 FAILED at 147.
Hunk #3 FAILED at 155.
3 out of 3 hunks FAILED -- saving rejects to file src/attribute-rel-util.cpp.rej
patching file src/attribute-rel-util.h
Reversed (or previously applied) patch detected! Assume -R? [n]
etc.

In any case, 11679/lp988601 has other issues which do not fall on setAttribute lines, and there does not seem to be anything in the repr-css.cpp part of 1043571v2.patch which addresses them (possibly addressed elsewhere in the patch though):

1. just after the comment containing

* HACK for now is to strip off em and ex units and add them back at the end

there is a piece of code that does not check that l>2 before using strncmp, which spammed the valgrind output
with 40 something invalid reads. It should be:

    int l = strlen(value_unquoted);
    if (l>2 &&
            (!strncmp(&value_unquoted[l-2], "em", 2) ||
             !strncmp(&value_unquoted[l-2], "ex", 2)
            )) {
        units = g_strndup(&value_unquoted[l-2], 2);
        value_unquoted = g_strndup(value_unquoted, l-2);
    }

Also, please have mercy and don't use "l" for a variable, it is nearly indistinguishable from "1", especially if your eyes are as
bad as mine.

2. (many) leak(s) at line 343
dst->mergeFrom(src, "");

here is one

==5076== 160 (128 direct, 32 indirect) bytes in 8 blocks are definitely lost in loss record 51,144 of 58,146
==5076== at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==5076== by 0x834A9B2: Inkscape::XML::SimpleNode::setAttribute(char const*, char const*, bool) (gc-core.h:74)
==5076== by 0x8347CAD: Inkscape::XML::SimpleNode::mergeFrom(Inkscape::XML::Node const*, char const*) (simple-node.cpp:660)
==5076== by 0x833D814: sp_repr_css_merge(SPCSSAttr*, SPCSSAttr*) (repr-css.cpp:343)
==5076== by 0x8B08327: ???

3. 1 leak at 351 (6200 bytes in the test run)

guchar *const str_value_unsigned = cr_term_to_string(decl->value);

when called from 410

sp_repr_css_merge_from_decl(css, decl_list);

==5076== 6,200 bytes in 25 blocks are possibly lost in loss record 57,124 of 58,146
==5076== at 0x402A420: memalign (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==5076== by 0x402A4DE: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==5076== by 0x52FF2E1: ??? (in /lib/i386-linux-gnu/libglib-2.0.so.0.3200.3)
==5076== by 0x534B73F: g_slice_alloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.3200.3)
==5076== by 0x534EEE9: g_string_sized_new (in /lib/i386-linux-gnu/libglib-2.0.so.0.3200.3)
==5076== by 0x534F526: g_string_new (in /lib/i386-linux-gnu/libglib-2.0.so.0.3200.3)
==5076== by 0x86CE81E: cr_term_to_string (cr-term.c:287)
==5076== by 0x833DD09: _ZL27sp_repr_css_merge_from_declP9SPCSSAttrPK14_CRDeclaration.isra.27 (repr-css.cpp:35...

I have 11679 as branch lp988601, is the problem patch in or out of that version?  setAttribute() is still causing problems, so I  am guessing no.   But I could not apply the patch from (5) successfully:

patch -p0 </tmp/1043571v2.patch
patching file src/attribute-rel-util.cpp
Reversed (or previously applied) patch detected!  Assume -R? [n] 
Apply anyway? [n] y
Hunk #1 FAILED at 128.
Hunk #2 FAILED at 147.
Hunk #3 FAILED at 155.
3 out of 3 hunks FAILED -- saving rejects to file src/attribute-rel-util.cpp.rej
patching file src/attribute-rel-util.h
Reversed (or previously applied) patch detected!  Assume -R? [n] 
etc.

In any case, 11679/lp988601 has other issues which do not fall on setAttribute lines, and there does not seem to be anything in the repr-css.cpp part of 1043571v2.patch which addresses them (possibly addressed elsewhere in the patch though):

1.  just after the comment containing

* HACK for now is to strip off em and ex units and add them back at the end

there is a piece of code that does not check that l>2 before using strncmp, which spammed the valgrind output
with 40 something invalid reads.  It should be:

int l = strlen(value_unquoted);
    if (l>2 &&
            (!strncmp(&value_unquoted[l-2], "em", 2) ||
             !strncmp(&value_unquoted[l-2], "ex", 2)
            )) {
        units = g_strndup(&value_unquoted[l-2], 2);
        value_unquoted = g_strndup(value_unquoted, l-2);
    }

Also, please have mercy  and don't use "l" for a variable, it is nearly indistinguishable from "1", especially if your eyes are as
bad as mine.

2.   (many) leak(s) at line 343
    dst->mergeFrom(src, "");

here is one

==5076== 160 (128 direct, 32 indirect) bytes in 8 blocks are definitely lost in loss record 51,144 of 58,146
==5076==    at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==5076==    by 0x834A9B2: Inkscape::XML::SimpleNode::setAttribute(char const*, char const*, bool) (gc-core.h:74)
==5076==    by 0x8347CAD: Inkscape::XML::SimpleNode::mergeFrom(Inkscape::XML::Node const*, char const*) (simple-node.cpp:660)
==5076==    by 0x833D814: sp_repr_css_merge(SPCSSAttr*, SPCSSAttr*) (repr-css.cpp:343)
==5076==    by 0x8B08327: ???

3.  1 leak at 351  (6200 bytes in the test run)

guchar *const str_value_unsigned = cr_term_to_string(decl->value);

when called from 410

sp_repr_css_merge_from_decl(css, decl_list);

==5076== 6,200 bytes in 25 blocks are possibly lost in loss record 57,124 of 58,146
==5076==    at 0x402A420: memalign (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==5076==    by 0x402A4DE: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==5076==    by 0x52FF2E1: ??? (in /lib/i386-linux-gnu/libglib-2.0.so.0.3200.3)
==5076==    by 0x534B73F: g_slice_alloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.3200.3)
==5076==    by 0x534EEE9: g_string_sized_new (in /lib/i386-linux-gnu/libglib-2.0.so.0.3200.3)
==5076==    by 0x534F526: g_string_new (in /lib/i386-linux-gnu/libglib-2.0.so.0.3200.3)
==5076==    by 0x86CE81E: cr_term_to_string (cr-term.c:287)
==5076==    by 0x833DD09: _ZL27sp_repr_css_merge_from_declP9SPCSSAttrPK14_CRDeclaration.isra.27 (repr-css.cpp:351)
==5076==    by 0x833E77B: sp_repr_css_attr_add_from_string(SPCSSAttr*, char const*) (repr-css.cpp:410)
==5076==    by 0x818C837: sp_css_attr_from_style(SPStyle const*, unsigned int) (style.cpp:4516)
==5076==    by 0x86A607C: MarkerComboBox::create_marker_image(unsigned int, char const*, SPDocument*, Inkscape::Drawing&, unsigned int) (stroke-marker-selector.cpp:493)
==5076==    by 0x86A6CF2: MarkerComboBox::add_markers(_GSList*, SPDocument*, int) (stroke-marker-selector.cpp:404)

5. a bunch of leaks at 43
    : SimpleNode(g_quark_from_static_string("css"), doc) {}

This last one drops a lot of memory:

==5076== 25,239 (400 direct, 24,839 indirect) bytes in 25 blocks are definitely lost in loss record 57,757 of 58,146
==5076==    at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==5076==    by 0x865D205: Inkscape::XML::CompositeNodeObserver::add(Inkscape::XML::NodeObserver&) (gc-core.h:74)
==5076==    by 0x8348521: Inkscape::XML::SimpleNode::SimpleNode(int, Inkscape::XML::Document*) (simple-node.cpp:178)
==5076==    by 0x833D022: sp_repr_css_attr_new() (repr-css.cpp:43)
==5076==    by 0x818C827: sp_css_attr_from_style(SPStyle const*, unsigned int) (style.cpp:4515)
==5076==    by 0x86A607C: MarkerComboBox::create_marker_image(unsigned int, char const*, SPDocument*, Inkscape::Drawing&, unsigned int) (stroke-marker-selector.cpp:493)
==5076==    by 0x86A6CF2: MarkerComboBox::add_markers(_GSList*, SPDocument*, int) (stroke-marker-selector.cpp:404)
==5076==    by 0x86A71F6: MarkerComboBox::sp_marker_list_from_doc(SPDocument*, int) (stroke-marker-selector.cpp:318)
==5076==    by 0x86A7834: MarkerComboBox::init_combo() (stroke-marker-selector.cpp:193)
==5076==    by 0x86A8285: MarkerComboBox::MarkerComboBox(char const*, int) (stroke-marker-selector.cpp:74)
==5076==    by 0xA387CFF: ???

Revision history for this message

David Mathog (mathog) wrote on 2012-09-25:

#9

How is "bzr merge" used? I copied the two files I modified to a safe place and then did:

bzr revert #because there was no "commit"
bzr merge #to bring the branch in sync with trunk
bzr: ERROR: Cannot lock LockDir(http://bazaar.launchpad.net/~inkscape.dev/inkscape/lp988601/.bzr/branch/lock): Transport operation not possible: http does not support mkdir()

Doesn't merge take the changes from trunk and merge them into the local branch? Why would it need a lock file?

Revision history for this message

Kris (kris-degussem) wrote on 2012-09-25:

#10

I have difficulties understanding what you mean (comment 8).
If I'm right, you tested with trunk revision 11679, which does not include the modifications of the patch (committed in revision 11686).

1. is a new issue. I will have a look at it. There is a memory leak at line 367 ("value_unquoted = g_strndup(value_unquoted, l-2);"). Will see whether using the Glib::ustring class as alternative to C-strings can be used.

2. is the original issue of this report. I'm not sure it is solved with revision 11686 (I actually doubt). If not solved, it should be checked whether the share_string function (used on line 375 of xml/simple-node.cpp) could be converted to use c++ string classes as opposed to C-strings/shared pointer strings.

Revision history for this message

Kris (kris-degussem) wrote on 2012-09-25:

#11

1043571comment8issue3.patch Edit (625 bytes, text/plain)

(some more on comment 8)
3. should be fixed with attached patch (not including any other changes previously reported as they are already in trunk). If I'm right though the library in which the code is, is deprecated though.

Changed in inkscape:
assignee:	nobody → Kris (kris-degussem)

Revision history for this message

Kris (kris-degussem) wrote on 2012-09-25:

#12

(comment 9): What do you want to do? Do you need:
bzr pull

Do you want to override local changes and download modifications from trunk? (if so "bzr pull --overwrite" should do the trick)

Revision history for this message

David Mathog (mathog) wrote on 2012-09-25:

#13

(comment 12). lp988601 is based on revision 11679. There are changes incorporated in lp988601 that are post r11689, some
are in the branch that I downloaded, and some were made locally after that. There are also changes in trunk since r11679. I want to modify lp988601 so that the local copy has both sets of changes applied.

What I had been doing before ~suv made the lp988601 branch was to keep all of my changes in a big patch file. To start
work on a new trunk revision, it would be downloaded with

bzr checkout --lightweight lp:inkscape

and then the patches would be applied to it with

patch -p0 <patches"

For some reason the patch command doesn't work with lp988601 though, even when I know the patch isn't in lp988601, it still gives messages like at the beginning of (8).

Revision history for this message

Kris (kris-degussem) wrote on 2012-09-25:

#14

(comment 13) OK: r11678 from the branch https://code.launchpad.net/~inkscape.dev/inkscape/lp988601 includes the revisions in trunk up to revision r11698, so it is normal what you get (you can not add a modification in the code that is already included :-) ). So my question in comment 10 is solved then and some more work (on the long term) is needed.

Revision history for this message

su_v (suv-lp) wrote on 2012-09-25:

#15

> I have difficulties understanding what you mean (comment 8).
> If I'm right, you tested with trunk revision 11679, which does not include
> the modifications of the patch (committed in revision 11686).

'lp988601' (comment #8) refers to the branch I created for the enhanced EMF support (tracked in bug #988601):
<https://code.launchpad.net/~inkscape.dev/inkscape/lp988601>

(I have now updated the branch to include the latest changes from trunk up to r11704).

Revision history for this message

David Mathog (mathog) wrote on 2012-09-26:

#16

Download full text (6.9 KiB)

Used "bzr update" to bring my local copy up to the changes ~suv noted in #15. Tested the updated lp988601 and it is still broken.

This is emf-inout.cpp:308

mod->set_param_string("destination", oldoutput);

and here is the associated valgrind record:

==3499== 22 bytes in 1 blocks are possibly lost in loss record 30,463 of 58,207
==3499== at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==3499== by 0x8519BBE: Inkscape::Util::share_string(char const*, unsigned int) (gc-core.h:77)
==3499== by 0x8519CAE: Inkscape::Util::share_string(char const*) (share.cpp:20)
==3499== by 0x834A4AD: Inkscape::XML::SimpleNode::setAttribute(char const*, char const*, bool) (simple-node.cpp:375)
==3499== by 0x80DEAC2: Inkscape::Preferences::_setRawValue(Glib::ustring const&, char const*) (preferences.cpp:732)
==3499== by 0x8200199: Inkscape::Extension::ParamString::set(char const*, SPDocument*, Inkscape::XML::Node*) (string.cpp:64)
==3499== by 0x825B1DC: Inkscape::Extension::Internal::Emf::save(Inkscape::Extension::Output*, SPDocument*, char const*) (emf-inout.cpp:308)
==3499== by 0x8203CF6: Inkscape::Extension::Output::save(SPDocument*, char const*) (output.cpp:218)
==3499== by 0x82026BF: Inkscape::Extension::save(Inkscape::Extension::Extension*, SPDocument*, char const*, bool, bool, bool, Inkscape::Extension::FileSaveMethod) (system.cpp:343)
==3499== by 0x80B0049: _ZL9file_saveRN3Gtk6WindowEP10SPDocumentRKN4Glib7ustringEPN8Inkscape9Extension9ExtensionEbbNS9_14FileSaveMethodE.constprop.345 (file.cpp:625)
==3499== by 0x80AFA7E: sp_file_save_dialog(Gtk::Window&, SPDocument*, Inkscape::Extension::FileSaveMethod) (file.cpp:897)
==3499== by 0xA7D25A3: ???

The other set_param_string lines all generate memory leaks too. So do set_param_bool lines like emf-inout.cpp:338

ext->set_param_bool("FixPPTCharPos",new_FixPPTCharPos);

==3499== 2 bytes in 1 blocks are definitely lost in loss record 474 of 58,207
==3499== at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==3499== by 0x8519BBE: Inkscape::Util::share_string(char const*, unsigned int) (gc-core.h:77)
==3499== by 0x8519CAE: Inkscape::Util::share_string(char const*) (share.cpp:20)
==3499== by 0x834A4AD: Inkscape::XML::SimpleNode::setAttribute(char const*, char const*, bool) (simple-node.cpp:375)
==3499== by 0x80DEAC2: Inkscape::Preferences::_setRawValue(Glib::ustring const&, char const*) (preferences.cpp:732)
==3499== by 0x81FA69E: Inkscape::Extension::ParamBool::set(bool, SPDocument*, Inkscape::XML::Node*) (bool.cpp:61)
==3499== by 0x825B00A: Inkscape::Extension::Internal::Emf::save(Inkscape::Extension::Output*, SPDocument*, char const*) (emf-inout.cpp:338)
==3499== by 0x8203CF6: Inkscape::Extension::Output::save(SPDocument*, char const*) (output.cpp:218)
==3499== by 0x82026BF: Inkscape::Extension::save(Inkscape::Extension::Extension*, SPDocument*, char const*, bool, bool, bool, Inkscape::Extension::FileSaveMethod) (system.cpp:343)
==3499== by 0x80B0049: _ZL9file_saveRN3Gtk6WindowEP10SPDocumentRKN4Glib7ustringEPN8Inkscape9Extension9ExtensionEbbNS9_14FileSaveMethodE.constprop.345 (file.cpp...

Used "bzr update" to bring my local copy up to the changes ~suv noted in #15.  Tested the updated lp988601 and it is still broken.

This is emf-inout.cpp:308

mod->set_param_string("destination", oldoutput);

and here is the associated valgrind record:

==3499== 22 bytes in 1 blocks are possibly lost in loss record 30,463 of 58,207
==3499==    at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==3499==    by 0x8519BBE: Inkscape::Util::share_string(char const*, unsigned int) (gc-core.h:77)
==3499==    by 0x8519CAE: Inkscape::Util::share_string(char const*) (share.cpp:20)
==3499==    by 0x834A4AD: Inkscape::XML::SimpleNode::setAttribute(char const*, char const*, bool) (simple-node.cpp:375)
==3499==    by 0x80DEAC2: Inkscape::Preferences::_setRawValue(Glib::ustring const&, char const*) (preferences.cpp:732)
==3499==    by 0x8200199: Inkscape::Extension::ParamString::set(char const*, SPDocument*, Inkscape::XML::Node*) (string.cpp:64)
==3499==    by 0x825B1DC: Inkscape::Extension::Internal::Emf::save(Inkscape::Extension::Output*, SPDocument*, char const*) (emf-inout.cpp:308)
==3499==    by 0x8203CF6: Inkscape::Extension::Output::save(SPDocument*, char const*) (output.cpp:218)
==3499==    by 0x82026BF: Inkscape::Extension::save(Inkscape::Extension::Extension*, SPDocument*, char const*, bool, bool, bool, Inkscape::Extension::FileSaveMethod) (system.cpp:343)
==3499==    by 0x80B0049: _ZL9file_saveRN3Gtk6WindowEP10SPDocumentRKN4Glib7ustringEPN8Inkscape9Extension9ExtensionEbbNS9_14FileSaveMethodE.constprop.345 (file.cpp:625)
==3499==    by 0x80AFA7E: sp_file_save_dialog(Gtk::Window&, SPDocument*, Inkscape::Extension::FileSaveMethod) (file.cpp:897)
==3499==    by 0xA7D25A3: ???

The other set_param_string lines all generate memory leaks too.  So do set_param_bool lines like emf-inout.cpp:338

ext->set_param_bool("FixPPTCharPos",new_FixPPTCharPos);

==3499== 2 bytes in 1 blocks are definitely lost in loss record 474 of 58,207
==3499==    at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==3499==    by 0x8519BBE: Inkscape::Util::share_string(char const*, unsigned int) (gc-core.h:77)
==3499==    by 0x8519CAE: Inkscape::Util::share_string(char const*) (share.cpp:20)
==3499==    by 0x834A4AD: Inkscape::XML::SimpleNode::setAttribute(char const*, char const*, bool) (simple-node.cpp:375)
==3499==    by 0x80DEAC2: Inkscape::Preferences::_setRawValue(Glib::ustring const&, char const*) (preferences.cpp:732)
==3499==    by 0x81FA69E: Inkscape::Extension::ParamBool::set(bool, SPDocument*, Inkscape::XML::Node*) (bool.cpp:61)
==3499==    by 0x825B00A: Inkscape::Extension::Internal::Emf::save(Inkscape::Extension::Output*, SPDocument*, char const*) (emf-inout.cpp:338)
==3499==    by 0x8203CF6: Inkscape::Extension::Output::save(SPDocument*, char const*) (output.cpp:218)
==3499==    by 0x82026BF: Inkscape::Extension::save(Inkscape::Extension::Extension*, SPDocument*, char const*, bool, bool, bool, Inkscape::Extension::FileSaveMethod) (system.cpp:343)
==3499==    by 0x80B0049: _ZL9file_saveRN3Gtk6WindowEP10SPDocumentRKN4Glib7ustringEPN8Inkscape9Extension9ExtensionEbbNS9_14FileSaveMethodE.constprop.345 (file.cpp:625)
==3499==    by 0x80AFA7E: sp_file_save_dialog(Gtk::Window&, SPDocument*, Inkscape::Extension::FileSaveMethod) (file.cpp:897)
==3499==    by 0xD44B7B3: ???

This may or may not be related - there is also what looks like a double delete() event associated with emf-inout.cpp:303

mod->base->invoke_hide(mod->dkey);

which does this:

==3499== Mismatched free() / delete / delete []
==3499==    at 0x402ACFC: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==3499==    by 0x81E8C64: NRStyle::~NRStyle() (nr-style.cpp:63)
==3499==    by 0x81CE8DF: Inkscape::DrawingShape::~DrawingShape() (drawing-shape.cpp:42)
==3499==    by 0x81CE92F: Inkscape::DrawingShape::~DrawingShape() (drawing-shape.cpp:48)
==3499==    by 0x812A51D: SPItem::invoke_hide(unsigned int) (sp-item.cpp:1121)
==3499==    by 0x812F256: CGroup::hide(unsigned int) (sp-item-group.cpp:783)
==3499==    by 0x812A470: SPItem::invoke_hide(unsigned int) (sp-item.cpp:1100)
==3499==    by 0x812F256: CGroup::hide(unsigned int) (sp-item-group.cpp:783)
==3499==    by 0x812A470: SPItem::invoke_hide(unsigned int) (sp-item.cpp:1100)
==3499==    by 0x812F256: CGroup::hide(unsigned int) (sp-item-group.cpp:783)
==3499==    by 0x812A470: SPItem::invoke_hide(unsigned int) (sp-item.cpp:1100)
==3499==    by 0x825B1AA: Inkscape::Extension::Internal::Emf::save(Inkscape::Extension::Output*, SPDocument*, char const*) (emf-inout.cpp:303)
==3499==  Address 0xc1bd3e0 is 0 bytes inside a block of size 16 alloc'd
==3499==    at 0x402B454: operator new[](unsigned int) (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==3499==    by 0x81E921B: NRStyle::set(SPStyle*) (nr-style.cpp:134)
==3499==    by 0x816B10C: SPShape::sp_shape_show(SPItem*, Inkscape::Drawing&, unsigned int, unsigned int) (sp-shape.cpp:801)
==3499==    by 0x812BEDC: SPItem::invoke_show(Inkscape::Drawing&, unsigned int, unsigned int) (sp-item.cpp:1047)
==3499==    by 0x812F34E: CGroup::_showChildren(Inkscape::Drawing&, Inkscape::DrawingItem*, unsigned int, unsigned int) (sp-item-group.cpp:768)
==3499==    by 0x813156C: CGroup::show(Inkscape::Drawing&, unsigned int, unsigned int) (sp-item-group.cpp:756)

And also, as long as we are at it,  this leaks (emf-inout.cpp:3083):

SPDocument *doc = SPDocument::createNewDocFromMem(d.outsvg->c_str(), strlen(d.outsvg->c_str()), TRUE);

and since that value is returned by Emf::open presumably something in the rest of Inkscape should be releasing it.

There is also a minor problem with the way extensions set up, which cause some memory to not be released before inkscape
exits.  It isn't a leak so much because it only happens once, but it does muddy the waters when looking for real leaks.  These are any calls like this:

Inkscape::Extension::build_from_mem(
        "<inkscape-extension xmlns=\"" INKSCAPE_EXTENSION_URI "\">\n"
            "<name>" N_("EMF Input") "</name>\n"
            "<id>org.inkscape.input.emf</id>\n"
            "<input>\n"
                "<extension>.emf</extension>\n"
                "<mimetype>image/x-emf</mimetype>\n"
                "<filetypename>" N_("Enhanced Metafiles (*.emf)") "</filetypename>\n"
                "<filetypetooltip>" N_("Enhanced Metafiles") "</filetypetooltip>\n"
                "<output_extension>org.inkscape.output.emf</output_extension>\n"
            "</input>\n"
        "</inkscape-extension>", new Emf());

which show up like this (so far always 4 bytes in 1 block):

==3499== 4 bytes in 1 blocks are still reachable in loss record 728 of 58,207
==3499==    at 0x402B9B4: operator new(unsigned int) (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==3499==    by 0x825C3AE: Inkscape::Extension::Internal::Emf::init() (emf-inout.cpp:3125)
==3499==    by 0x638807F: ???

That is a lot more than 4 bytes, so it looks like something elsewhere is removing the contents but not deleting the pointer, or something like that.

Revision history for this message

su_v (suv-lp) wrote on 2012-09-26:

#17

> Used "bzr update" to bring my local copy up to the changes ~suv noted in #15.
> Tested the updated lp988601 and it is still broken.

Are you saying that the branch (without any local changes) is broken for you (i..e. fails to compile)? If so, please provide details about what exactly is broken in the branch (not in your local copy - you have to resolve conflicts due to local changes yourself): the branch builds without failure for me on two different systems (without any further local changes except then one for autogen.sh from bug #992047 on Lion).

Revision history for this message

Kris (kris-degussem) wrote on 2012-09-26:

#18

In reply to comment 16:
Can we keep this report for the original bug, please? Already five different memory leaks have been fixed as a result of this report, 2 other memory leaks and another bug are also mentioned here, while there is still the original bug to be fixed ...

Revision history for this message

David Mathog (mathog) wrote on 2012-09-26:

#19

re #17: It compiles, it just still has memory issues, as shown in #16.
re #18: Sorry, got a little carried away. The set_param_bool() leak should be fixed with set_param_string()though. Seems
likely that all of the set_param_*() leak if these two do.

Revision history for this message

Kris (kris-degussem) wrote on 2012-09-29:

#20

Patch of comment 11 committed in rev 11715.
Comment 8 issue 1 might make it into Inkscape 0.49, others will be for later on (require refactoring, that should be done after 0.49 release conforming with the release plan).

Revision history for this message

Kris (kris-degussem) wrote on 2012-10-03:

#21

Forked comment 8 issue one in a separate bug report for clarity: bug #1061157

Comment 8 issue 3 is probably fixed in trunk. If not, more information and necessary steps to reproduce this are needed in a separate bug report.

Comment 8 issue 5: not clear on how to reproduce this problem. What are the necessary steps: should be reported in a separate bug report.

Comment 16 issue 2 should be fixed in trunk revision 11731.

Comment 16 issue 3: not sure what teh issue is and where it is located. Regarding the description it is best to look after this after the emf branch is merged into trunk and if the issue is still present to file a separate bug report then.

Original bug report, comment 8 issue 2 and comment 16 issue 1 are the same problem. They need quite some refactoring which can not be done until Inkscape 0.49 is released.

Revision history for this message

Kris (kris-degussem) wrote on 2013-04-17:

#22

Fixing this bug is way beyond my level of knowlegde atm. For more information see Inkscape garbage collection system (of with ptr_shared is a part). See Inkscape developers mailings discussion: http://tinyurl.com/cof8l5q

Changed in inkscape:
assignee:	Kris (kris-degussem) → nobody
status:	In Progress → Triaged

Inkscape

mod->set_param_string memory leak

Bug Description

Other bug subscribers

Patches

Remote bug watches