Fix resource leak on Safari on map.remove()

[joewoodhouse]

…ners and dereference canvas and other containers to prevent resource leak on Safari

Brief Description

I have been investigating an issue where our mobile application - which uses mapbox-gl-js - was crashing after repeatedly opening and closing a page containing a map.

In examining heap snapshots I noticed that event listeners and Map instances were being leaked.

The final line of the changes I believe is a resource leak on every platform - the container element is an existing element in the DOM passed to us by the library user (either by ID or as a direct HTMLElement) which the existing code then attaches an eventListener to for 'scroll' but never removes it. This results in the browser not being able to reclaim the resources for the mapboxgl.Map instance even after the map has been removed by calling map.remove().

The changes above that relate more specifically to iOS Safari where I was again seeing instances of mapboxgl.Map being leaked, and on inspection of the heapmap the retaining element was these contextLost/contextRestored event listeners. In theory this should not be required I think, but I know that iOS Safari has a lot of well-known issues around performance and memory particular in relation to canvas.

Testing/Reproduction

I have created a simple test site to show the problem.

To Recreate the issue use: https://joyful-granita-e529dc.netlify.app/existing.html. This recreates the problem using the latest version of mapbox-gl-js (2.10.0). The test page simply loads and then unloads a map in a continual loop until you hit Stop.

• Run in Safari
• Input a valid mapbox token into the token input at the top
• Hit Start
• Let it load and unload a map for 20+ iterations
• Hit Stop
• Open Web Inspector > Timelines > Javascript Allocations
• Take a heap snapshot (camera icon)
• Inspect the snapshot

On inspecting the snapshot you will see a large retained size, and inspecting the heap you will see all the leaked Map instances are still present. Safari devtool helpfully shows that _onMapScroll is the retaining reference here.

To view the fixed version, repeat the above steps using this link: https://joyful-granita-e529dc.netlify.app/fixed.html

This is the same reproduction but pointing out a custom build of mapbox-gl-js that contains the fixes attached to this PR. If you repeat the steps above, on inspection of the heap snapshot you will see there are no leaked Map instances.

The above can also be repeated in Chrome using the Performance Monitor to view JS Heap size. On the "existing" version the heap will increase indefinitely and manually triggering a GC will not bring memory down. On the "fixed" version heap size will remain stable.

Launch Checklist

• briefly describe the changes in this PR
• include before/after visuals or gifs if this PR includes visual changes
• write tests for all new functionality
• document any changes to public APIs
• post benchmark scores
• manually test the debug page
• tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
• tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
• apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fix memory leak when removing maps</changelog>

[CLAassistant]

All committers have signed the CLA.

[mourner]

This looks great overall, thanks so much for investigating this and submitting a fix!

Aside from the inline comment, I'm also wondering if there's any way to write a unit test for this to make sure we don't accidentally reintroduce a similar leak? E.g. write a test that stubs _container.addEventListener and _container.addEventListener and then makes sure every call the former also corresponds to the latter after map.remove().

[mourner]

Is it important to remove the DOM element references here if the map object itself is discarded? And if so, should we remove more references, in particular those retained transitively — e.g. map.dragPan._el, or map._logoControl._container etc.?

[joewoodhouse]

My thought was that suppose the map object is not discared. For example if you do

const container = document.createElement('div');
const map = new mapboxgl.Map({
   container,
   ...
});
....
map.remove();

After this code, without removing the DOM references then the browser is not able to discard those elements - if you look in Chrome dev tools heap map you'll end up with a bunch of "Discarded HTML*" elements, because the elements have been removed from the DOM but the retaining reference means they can't be garbage collected.

In terms of the other references, for example map._logoControl I think is OK because that control is properly removed and the ._controls array is emptied in the map.remove() - so the DOM elements are removed and the controls are discarded.

The dragPan I'm not too sure about, I can't see elements/resources being leaked after my fix so I assume those parts are fine.

[mourner]

map._logoControl reference is retained after map.remove() even if _controls array is emptied. You can see references to the canvas container element in the handler objects too. So I'm just wondering why some of those retained references leak (like those explicitly cleared in this PR) and some don't — maybe that's worth investigating.

[joewoodhouse]

This looks great overall, thanks so much for investigating this and submitting a fix!

Aside from the inline comment, I'm also wondering if there's any way to write a unit test for this to make sure we don't accidentally reintroduce a similar leak? E.g. write a test that stubs _container.addEventListener and _container.addEventListener and then makes sure every call the former also corresponds to the latter after map.remove().

@mourner I've added this as a unit test now.

[mourner]

Overall this is a clear improvement so we should merge it, but we might want to follow up to resolve the questions brought up above later. Thanks @joewoodhouse!