Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unregister viewport size listener on setTarget(null) #3420

Merged
merged 2 commits into from Apr 3, 2015
Merged

Unregister viewport size listener on setTarget(null) #3420

merged 2 commits into from Apr 3, 2015

Conversation

elemoine
Copy link
Member

This PR addresses the memory leak problem reported in #3292. It fixes the problem by unregistering the viewport resize listener when setTarget(null) is called to removed the map from the map.

@fredj, this PR is an alternative to #3300, making it unnecessary to have to call (and expose) dispose on the map. You can just do map.setTarget(null); map = null; to make the map a candidate for garbage collection.

This PR also adds a commit modifying the tile queue code to unregister "change" listeners the tile queue sets on tiles.

Please review.

This commit unregisters the viewport resize listener when setTarget(null) is called, and registers it when setTarget(target) is called, and when we don't have that listener registered yet.

This prevents memory leaks where references to map objects are retained because of these viewport resize listeners.
When the tile queue loads a tile it registers a "change" listener on that tile. But currently that listener is not unregistered, unless the tile cache containing that tile fills up and "dipose" is called on that tile.

This commit makes handleTileChange deregister the "change" listener on the tile, if the tile is now in "loaded" state.
@bartvde bartvde added this to the v3.5.0 milestone Apr 3, 2015
@tschaub
Copy link
Member

tschaub commented Apr 3, 2015

Looks great @elemoine. Please merge.

elemoine pushed a commit that referenced this pull request Apr 3, 2015
Unregister viewport size listener on setTarget(null)
@elemoine elemoine merged commit 7c0977d into openlayers:master Apr 3, 2015
@bartvde bartvde mentioned this pull request Apr 4, 2015
@elemoine elemoine mentioned this pull request Apr 4, 2015
@elemoine elemoine deleted the leaks branch April 27, 2015 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants