Calling map remove method does not remove FullscreenControl event listener

[stephengsumo]

Calling map.remove() does not remove Controls from the map. I'm using the FullScreen control and each time a new map is created, it adds a document event listener onfullscreenchange. This listener has a reference to the map and so memory is being leaked.

mapbox-gl-js version: 0.47.0

browser: Chrome Version 67.0.3396.99

Steps to Trigger Behavior

Go to https://jsfiddle.net/pmxgoqh3/8/

1. In Chrome console, do getEventListeners(document). You'll see an array of only 1 listener.
2. Press reload map link 5 times
3. In Chrome console, do getEventListeners(document). You'll see the array of listeners go from 1 to 6 (5 more listeners than step 1).

Link to Demonstration

https://jsfiddle.net/pmxgoqh3/8/

Expected Behavior

Calling map.remove() should remove Controls from the map.

Actual Behavior

Calling map.remove() does not remove Controls from the map.

[mollymerp]

I think this memory leak is only relevant to the FullscreenControl because the other controls only add event listeners on control-owned DOM nodes so they are garbage collected when the control container node is removed, but the FullscreenControl adds document level event listener. Because the map does not store a reference to the FullscreenControl instance, it is currently unable to remove the associated event listener when map.remove() is called.

A workaround for this issue pending a more permanent fix is to keep a reference to the control and call map.removeControl(fsControl) prior to calling map.remove()

[jfirebaugh]

A workaround for this issue pending a more permanent fix is to keep a reference to the control and call map.removeControl(fsControl) prior to calling map.remove()

This sounds like the right fix to me. In fact, the map should call removeControl on all the controls that have been added with addControl, because someone might have added their own custom control that needs to do cleanup.

[stephengsumo]

Yeah for now I am calling map.removeControl(fsControl) before map.remove(). I just assumed that map.remove() would remove all controls too. The API documentation doesn't say that controls are removed, but it also doesn't say we should remove controls before calling map.remove(). https://www.mapbox.com/mapbox-gl-js/api/#map#remove

[jfirebaugh]

Oh, I think I misread @mollymerp's suggestion. She was suggesting an end-user workaround. I agree @stephengsumo, map.remove() should remove all controls too.

[mollymerp]

closed by #7042