Exception removing event listener #905

jshailes · 2018-09-18T17:58:53Z

I think I've followed the documentation when trying to remove a click event listener (http://svgjs.com/events/#element-click) however it results in an exception (I'm using 2.6.3):

Uncaught TypeError: Cannot read property 'bind' of null
    at Function.SVG.on (svg.js:3417)
    at initializer.SVG.Element.(/impact/anonymous function) [as click] (https://cdnjs.cloudflare.com/ajax/libs/svg.js/2.6.3/svg.js:3404:9)

Code is as follows:

var draw = SVG('drawing').size(300, 130);
var rect = draw.rect(100, 100);
rect.click(null);

It might be worth noting that the error also occurs when removing the listener in the more likely case where a click listener has previously been set:

var draw = SVG('drawing').size(300, 130);
var rect = draw.rect(100, 100);
rect.click(function() { alert("Clicked" });
rect.click(null);

The text was updated successfully, but these errors were encountered:

Fuzzyma · 2018-09-18T18:58:22Z

I think the documentation is not up to date with this one (or the code isnt - it depends^^).
For the meantime just use the slightly longer verion: rect.off('click')

jshailes · 2018-09-18T20:32:56Z

Great, thanks very much

jshailes · 2018-09-18T21:09:06Z

I just tried this but it seems that a reference to the object is still held in memory after 'removing' the listener. I'm reasonably sure the listener is the issue as the rectangle object disappears from memory if I don't add a listener to it.

Any suggestions?

Fuzzyma · 2018-09-18T21:16:23Z

Hm - all listener are saved on the element itself. We fixed this memory leak a while ago (#805 #807). Are you sure you are using the latest release?

jshailes · 2018-09-19T02:05:03Z

Am I right in thinking that went in to 2.6.5? I'm currently running 2.6.3.

Fuzzyma · 2018-09-19T06:11:00Z

It's time to upgrade then :D. You can take a look into the changelog file and double check which version it is was added and what changed overall. It should not cause any issues to update a minor version

jshailes · 2018-09-19T16:31:33Z

I've updated to version 2.6.6 and it seems the issue remains. Below is the code I've been testing with:

<html>
	<head>
		<script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/svg.js/2.6.6/svg.js"></script>
		<script   src="https://code.jquery.com/jquery-3.2.1.js"   integrity="sha256-DZAnKJ/6XZ9si04Hgrsxu/8s717jcIzLy3oi35EouyE="   crossorigin="anonymous"></script>
	</head>
	<body>
		<div id="drawing"></div>
		<button id="addEventListeners">Add Event Listeners</button>
		<button id="removeEventListeners">Remove Event Listeners</button>
		<button id="removeElements">Remove Elements</button>

		<script type="text/javascript">
			//create svg drawing
			var draw = SVG('drawing').size(300, 130);
			
			//create new elements in the drawing
			var svgElements = [];
			for(var i=0; i<20; i++) {
				var svgElement = draw.circle(10, 10);
				svgElements.push(svgElement);
			}

			// add click event listener to each element
			$('#addEventListeners').click(function() {
				for(var i=0; i<svgElements.length; i++) {
					var svgElement = svgElements[i];
					svgElement.click(function(){
						alert("Hi SVG Element");
					});
				}
			});

			// remove click event listener to each element
			$('#removeEventListeners').click(function() {
				for(var i=0; i<svgElements.length; i++) {
					var svgElement = svgElements[i];
					svgElement.off("click");
				}
			});

			// remove elements including the drawing
			$('#removeElements').click(function() {
				for(var i=0; i<svgElements.length; i++) {
					var svgElement = svgElements[i];
					svgElement.remove();
					svgElement = null;
				}
				svgElements = [];

				draw.clear();
				draw.remove();
				draw = null;
			});
		</script>
	</body>
</html>

I've taken memory dumps in the following scenarios:

Load page
Load page, Remove Elements
Load page, Add Event Listeners, Remove Event Listeners, Remove Elements

In Scenario 1, 23 circle elements exist and none are detached. I'm not sure what the extra 3 are but it seems consistent amongst scenarios so I've ignore these. This is the behaviour I expect.

In Scenario 2, 3 circle elements exist and none are detached. This is the behaviour I expect.

In Scenario 3, 3 circle elements exist and 20 are detached (but still reachable from the window). I would have expected 3 to exist and none to be detached (as in scenario 2 where listeners hadn't been created).

Fuzzyma · 2018-09-20T08:34:46Z

What do you mean with:

(but still reachable from the window).

jshailes · 2018-09-20T11:01:59Z

That's the description the dev tools in Google Chrome gives. Detached means that it's no longer in the dom, and since it still appears in the memory dump I assume 'but still reachable from the window' means that it's still held in memory for the window.

Fuzzyma · 2018-09-20T11:23:24Z

can it also give you the path from where it is reachable? That would help a lot!

jshailes · 2018-09-20T12:06:07Z

It does - when you take the memory dump, select one of the detached circle instances and it lists the retainers at the bottom. I tried to fathom it out but it didn't make sense to me.

handlerMap in ()     -   svg.js:29 
  - SVG in Window

Fuzzyma · 2018-09-20T12:13:50Z

Thank you!

jshailes · 2018-09-20T12:19:56Z

Just having a look at it again. In the SVG.off method, where !ns && !ev there's a reference to deleting items from the handlerMap. I've not yet understood how it works but I see no reference to deleting from handlerMap in !ns && ev where the code I posted enters. I tried adding delete SVG.handlerMap[index] in there and it made most of the detached circles get removed from memory but left 1 instance for some reason.

Fuzzyma · 2018-09-20T12:25:45Z

oooohh right! We fixed this in the 3.0 and forgot to backport it! So yes you are completely right. There is a memory leak. Take a look into the 3.0 branch in the event.js file were you can see what was changed to fix this issue. I am currently working to get 3.0 out as soon as possible so I am short on time. Do you think you can create a PR for this?

jshailes · 2018-09-20T13:15:25Z

I'm sorry but from looking at the master vs 3.0.0 branch I don't understand what changes need to be made. No worries about getting it fixed in 2.x, I can wait until 3.0 is out.

### Fixed - fixed calling `parent()` on `documentFragment`s children (#927) - parser is not focusable anymore (#908) - `SVG.Element.click(null)` correctly unbinds the event (#878) - fix memory leak (#905) ### Added - `SVG.Set` now accepts another Set as input (#893) - `on()/off()` accepts multiple event names as input (backport from 3.0)

Fuzzyma · 2018-11-13T20:15:49Z

I backported the fix so that shouldnt be an issue anymore. See 2.7.0

Fuzzyma closed this as completed Nov 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exception removing event listener #905

Exception removing event listener #905

jshailes commented Sep 18, 2018 •

edited

Loading

Fuzzyma commented Sep 18, 2018

jshailes commented Sep 18, 2018

jshailes commented Sep 18, 2018

Fuzzyma commented Sep 18, 2018

jshailes commented Sep 19, 2018

Fuzzyma commented Sep 19, 2018

jshailes commented Sep 19, 2018

Fuzzyma commented Sep 20, 2018

jshailes commented Sep 20, 2018

Fuzzyma commented Sep 20, 2018

jshailes commented Sep 20, 2018

Fuzzyma commented Sep 20, 2018

jshailes commented Sep 20, 2018

Fuzzyma commented Sep 20, 2018

jshailes commented Sep 20, 2018

Fuzzyma commented Nov 13, 2018

Exception removing event listener #905

Exception removing event listener #905

Comments

jshailes commented Sep 18, 2018 • edited Loading

Fuzzyma commented Sep 18, 2018

jshailes commented Sep 18, 2018

jshailes commented Sep 18, 2018

Fuzzyma commented Sep 18, 2018

jshailes commented Sep 19, 2018

Fuzzyma commented Sep 19, 2018

jshailes commented Sep 19, 2018

Fuzzyma commented Sep 20, 2018

jshailes commented Sep 20, 2018

Fuzzyma commented Sep 20, 2018

jshailes commented Sep 20, 2018

Fuzzyma commented Sep 20, 2018

jshailes commented Sep 20, 2018

Fuzzyma commented Sep 20, 2018

jshailes commented Sep 20, 2018

Fuzzyma commented Nov 13, 2018

jshailes commented Sep 18, 2018 •

edited

Loading