Patch memory leaks caused by event handlers bound to document #9337
Conversation
|
Hi @arindam1993, |
|
@iamanvesh I posted some results from some digging in #9126 , could we continue the conversation there? |
src/ui/handler/box_zoom.js
Outdated
| window.document.removeEventListener('mousemove', this._onMouseMove, false); | ||
| window.document.removeEventListener('keydown', this._onKeyDown, false); | ||
| window.document.removeEventListener('mouseup', this._onMouseUp, false); | ||
| } | ||
|
|
||
| teardown() { |
There was a problem hiding this comment.
In the rest of the codebase, we use remove() for this action. Let's keep up the consistency.
src/ui/handler/box_zoom.js
Outdated
| _finish() { | ||
| this._active = false; | ||
|
|
||
| _unbind() { |
There was a problem hiding this comment.
Let's guard against the race condition/double invocation that could occur when the user removes the map while a handler is active.
There was a problem hiding this comment.
I don't think thats possible, infact the changes this PR would prevent that from happening.
Scenario one:
If a handler callback is executing, map.remove() can't be called, because well, JS is single threaded.
Scenario two:
And if map.remove() is called while a handler is still active i.e the user has the mouse held down, the callback gets unbound, so it would never invoke once the user eventually say, does a mouseup. Previous to this it would actually invoke the mouseup.
There was a problem hiding this comment.
If a handler callback is executing,
map.remove()can't be called, because well, JS is single threaded.
You can call map.remove() from the callback ;)
There was a problem hiding this comment.
Yea.. but why would we ever do that?
( Famous last words)
There was a problem hiding this comment.
It might not be intentional, e.g. an application could transition away from a map view (and calling map.remove()) while the user is still interacting with it.
There was a problem hiding this comment.
But that would be Scenario 2, which this fixes? map.remove() would still not be synchronously called in our callback, because thats our code and not user code?
That transition away would run in a different run-loop tick, so any of the enteraction callbacks would have been fully executed.
|
We should probably wait until #9365 lands for an easier time reconciling the code. |
|
Sticking the |
|
Closing since #9508 addresses thise. |
This potentially addresses #9126.
We temporarily bind handlers to
documentin our event handlers when certain interactions are in flight. If the map is removed during that time, it causes the entireMapobject to be retained, since its in the scope of the event handlers.<changelog>Fixed a memory leak that occurred if the map was removed while certain user interactions were in progress.</changelog>