Skip Menu |
 

This queue is for tickets about the XML-LibXML CPAN distribution.

Report information
The Basics
Id: 83744
Status: resolved
Priority: 0/
Queue: XML-LibXML

People
Owner: Nobody in particular
Requestors: DGINEV [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: 2.0014
Fixed in: (no value)



Subject: XPathContext memory leak on registerFunction
I am using the following test: ######################## ######################## use Test::Builder; my $Test=Test::Builder->new(); $Test->expected_tests(2); use Test::LeakTrace; no_leaks_ok { use XML::LibXML::XPathContext; } ' load XPathContext without leaks '; no_leaks_ok { my $context = XML::LibXML::XPathContext->new(); $context->registerFunction('match-font',sub{1;}); $context->unregisterFunction('match-font'); } ' register an XPath function and unregister it, without leaks ' ######################## ######################## There is a memory leak here, that occurs only after registerFunction has been called. From what I traced, registerFunctionNS, in the native C library uses xmlHashCreate to allocate a new hash in the hashFunc field of the context. Looking at the current XS code at: http://cpansearch.perl.org/src/SHLOMIF/XML-LibXML-2.0014/LibXML.xs the DESTROY method of XPathContext is not freeing any of the function datastructures (allocated via registerFunctionNS and friends). It would be quite nice to get a patch here.
Download (untitled) / with headers
text/plain 489b
On Mon Mar 04 01:48:05 2013, DGINEV wrote: Show quoted text
> It would be quite nice to get a patch here.
Not sure if this actually works (have to rebuild LibXML and retest), but my hunch is that you need the following code added to the DESTROY subroutine for XPath Context in LibXML.xs if (ctxt->funcHash !=NULL) { xmlHashFree(ctxt->funcHash, NULL); ctxt->funcHash = NULL; } If I succeed rebuilding LibXML I will let you know if this solves the leak.
Hi! Sorry to interfere, but the problem with reference counting (patch attached). -- Yuriy
Subject: 83744.patch
Download 83744.patch
text/x-diff 1.6k
diff -Nura XML-LibXML-2.0101-orig/LibXML.xs XML-LibXML-2.0101/LibXML.xs --- XML-LibXML-2.0101-orig/LibXML.xs 2013-08-15 08:26:18.000000000 +0300 +++ XML-LibXML-2.0101/LibXML.xs 2013-08-15 16:57:24.481326332 +0300 @@ -7751,7 +7751,7 @@ || SvPOK(func)))) { if (ctxt->funcLookupData == NULL) { if (SvOK(func)) { - pfdr = newRV_inc((SV*) newHV()); + pfdr = newRV_noinc((SV*) newHV()); ctxt->funcLookupData = pfdr; } else { /* looks like no perl function was never registered, */ diff -Nura XML-LibXML-2.0101-orig/t/52_xpc_memory_leak_rt83744.t XML-LibXML-2.0101/t/52_xpc_memory_leak_rt83744.t --- XML-LibXML-2.0101-orig/t/52_xpc_memory_leak_rt83744.t 1970-01-01 03:00:00.000000000 +0300 +++ XML-LibXML-2.0101/t/52_xpc_memory_leak_rt83744.t 2013-08-15 17:18:30.866355228 +0300 @@ -0,0 +1,27 @@ + +use strict; +use warnings; + +=head1 DESCRIPTION + +XPathContext memory leak on registerFunction. + +See L<https://rt.cpan.org/Ticket/Display.html?id=83744>. + +=cut + +use constant HAS_LEAKTRACE => eval{ require Test::LeakTrace }; +use Test::More HAS_LEAKTRACE ? (tests => 2) : (skip_all => 'Test::LeakTrace is required for memory leak tests.'); +use Test::LeakTrace; + +# TEST +no_leaks_ok { + use XML::LibXML::XPathContext; +} 'load XPathContext without leaks'; + +# TEST +no_leaks_ok { + my $context = XML::LibXML::XPathContext->new(); + $context->registerFunction('match-font', sub {1;}); + $context->unregisterFunction('match-font'); +} 'register an XPath function and unregister it, without leaks';
Download (untitled) / with headers
text/plain 539b
Hi Yuriy, On Thu Aug 15 11:13:39 2013, yoreek wrote: Show quoted text
> Hi! > > Sorry to interfere, but the problem with reference counting (patch > attached). >
Thanks for the patch. However, I'd rather not make XML::LibXML dependent on Test::LeakTrace, so please use the «if (! eval "use Test::LeakTrace;") { plan skip_all => ...}» paradigm. Furthermore, please don't extend the test number beyond 51 for that - you can put it in t/48*.t anyway (test numbers have become quite meaningless anyway). Thanks! Regards, -- Shlomi Fish Show quoted text
> -- > Yuriy
Download (untitled) / with headers
text/plain 938b
On Thu Aug 15 12:27:55 2013, SHLOMIF wrote: Show quoted text
> Hi Yuriy, > > On Thu Aug 15 11:13:39 2013, yoreek wrote:
> > Hi! > > > > Sorry to interfere, but the problem with reference counting (patch > > attached). > >
> > Thanks for the patch. However, I'd rather not make XML::LibXML > dependent on Test::LeakTrace, so please use the «if (! eval "use > Test::LeakTrace;") { plan skip_all => ...}» paradigm. Furthermore, > please don't extend the test number beyond 51 for that - you can put > it in t/48*.t anyway (test numbers have become quite meaningless > anyway). > > Thanks! > > Regards, > > -- Shlomi Fish
> > -- > > Yuriy
Code "«if (! eval "use Test::LeakTrace;")..." does not work with this module, possible exporter is not working correctly: "Can't locate object method "no_leaks_ok" via package...". So I made based on example: https://metacpan.org/module/Test::LeakTrace. Can you solve this problem in a more elegant ? -- Yuriy
Download (untitled) / with headers
text/plain 1.1k
On Thu Aug 15 18:09:57 2013, yoreek wrote: Show quoted text
> On Thu Aug 15 12:27:55 2013, SHLOMIF wrote:
> > Hi Yuriy, > > > > On Thu Aug 15 11:13:39 2013, yoreek wrote:
> > > Hi! > > > > > > Sorry to interfere, but the problem with reference counting (patch > > > attached). > > >
> > > > Thanks for the patch. However, I'd rather not make XML::LibXML > > dependent on Test::LeakTrace, so please use the «if (! eval "use > > Test::LeakTrace;") { plan skip_all => ...}» paradigm. Furthermore, > > please don't extend the test number beyond 51 for that - you can put > > it in t/48*.t anyway (test numbers have become quite meaningless > > anyway). > > > > Thanks! > > > > Regards, > > > > -- Shlomi Fish
> > > -- > > > Yuriy
> > Code "«if (! eval "use Test::LeakTrace;")..." does not work with this > module, possible exporter is not working correctly: "Can't locate > object method "no_leaks_ok" via package...". > So I made based on example: > https://metacpan.org/module/Test::LeakTrace. > > Can you solve this problem in a more elegant ? >
Fixed in the repository trunk. Will be released soon with other fixes. Thanks for the report and the fix. Regards, -- Shlomi Fish Show quoted text
> -- > Yuriy


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.