Bug 46811

Created attachment 23346 [details]
Patch from proposed changes

(In reply to comment #0)
> To reproduce, succesively add, start and remove many many animation elements
> in a document programatically.

Could you please attach a test case? I haven't investigated this properly, but being able to reproduce usually helps a lot. ;-)


> Possible solution: removing elements from these two maps when an animation is
> removed, last two lines in the method. 

Stating which file was modified would avoid a find in files... :-P
The changes refer to sources/org/apache/batik/anim/AnimationEngine.java. I've created a patch to ease this. :-)

Bug 46155 and bug 46124 may also be related with this -- may this be a regression from those changes?...


Hope this helps,
 Helder Magalhães

> Could you please attach a test case? I haven't investigated this properly, but
> being able to reproduce usually helps a lot. ;-)

A possible test case would look like this:

Element[] animatedElements = new Element[100];
while(true){	
	//start 100 animations
	for(int i=0;i<100;i++){
		//create the element to be animated
		Element animatedElement = document.createElementNS("http://www.w3.org/2000/svg", "rect");	
		animatedElement.setAttributeNS(null, "x", "0");
		animatedElement.setAttributeNS(null, "y", "0");		
		animatedElement.setAttributeNS(null, "width", "100");
		animatedElement.setAttributeNS(null, "height", "100");
		animatedElement.setAttributeNS(null, "style", "fill:red");
	
		// add animation to the animatedElement
		Element animation = document.createElementNS("http://www.w3.org/2000/svg", "animateMotion");	
		animation.setAttributeNS(null, "begin", "indefinite");
		animation.setAttributeNS(null, "dur", "1s");
		animation.setAttributeNS(null, "fill", "freeze");
		animation.setAttributeNS(null, "path", "M 0 0 L 100 100");
									
		// add animation to animatedElement
		animatedElement.appendChild(animation);
		// add the animated element to the document
		((SVGDocument) document).getRootElement().appendChild(animatedElement);				
		// start the animation now
		((ElementTimeControl) animation).beginElement();
		//remember the animated element so that we can later remove it
		animatedElements[i]=animatedElement;
	}
	Thread.sleep(10);
	//remove the above created 100 animated elements
	for(int i=0;i<100;i++){			
		//remove animated element, this also removes the animation
		animatedElements[i].getParentNode().removeChild(animatedElements[i]);	
	}
}

> Stating which file was modified would avoid a find in files... :-P
I did mention the name of the class AnimationEngine, sorry I didn't put it so that it's easier to see, it's the first time I submit a bug, ever as far as I can remember.

> Bug 46155 and bug 46124 may also be related with this -- may this be a
> regression from those changes?...

I really don't know the details of batik developement or how things work in this complex library, I just profiled and found the place the memory leak happened and fixed the sources, I don't know if it's a good solution. Just needed a quick fix four our application, and tought to help you out a little to improve the libs.

Best regards,
Cristi.

(In reply to comment #2)
> > Stating which file was modified would avoid a find in files... :-P
> I did mention the name of the class AnimationEngine, sorry I didn't put it so
> that it's easier to see, it's the first time I submit a bug, ever as far as I
> can remember.

Sorry, I didn't notice the class name. Of course that was sufficient. ;-)
Thanks for the report! :-)


@Batik developers:
Patch seems simple and safe to apply. Nevertheless, the proposed fix may be simply obfuscating something deeper... I'm also still not certain if this wasn't caused by any of the mentioned fixes (see comment 1).

Also, do you believe it's worth to wrap up a stand-alone SVG test case (based on the one in comment 2)? Maybe even try to integrate it into regard's memory leak tests...?