jQuery DOM event handlers potentially leak memory

[nickjs]

Did you know that every tg Page in the admin instantiates a new instance of NextNavigation? How about every _form partial leaks an entire tinymce editor? I discovered this today and now am :crying_cat_face:

I would like to fix this, but need some discussion around the issue first. Is define really the root cause here, or just a symptom? Is it turbograft itself actually causing the problem? Whoooo knows.

@DrewMartin @qq99 @tmlayton /cc @Shopify/admin-fed

[qq99]

cc @Shopify/tnt

[tmlayton]

Does twine not handle clean up? I was aware that even with tg-static new instances of Drawer and nextNavigation were instantiated each time but figured old instances were cleaned up.

[DrewMartin]

Twine sort of cleans up after itself in that it calls down the teardown method defined on a particular binding, but that's just to remove the event handlers that Twine itself set up. define has no teardown and it's still possible to leak.

An object that sets up jQuery event listeners is definitely susceptible to a leak. You have to call a jQuery teardown function of some sort to get them removed: either explicitly call .off with that event or replace it/remove it with a jQuery function (remove, empty, html, just about anything that modifies the DOM aside from detach) which is why we have https://github.com/Shopify/shopify/blob/master/app/assets/javascripts/admin/lib/modules.coffee#L85-L86 to listen for when turbograft replaces a node and remove it with jQuery as well.

[DrewMartin]

Oh and that stuff in modules.coffee will only clean up events listening on the node where the object is defined or below. Like if you have

constructor: (node) ->
  $(node).on 'click', -> # do stuff

then it will be cleaned up, but listening on events on the window or document won't. We'll need to clean those up manually using Page.onReplace. NextNavigation is one like that (fix here: https://github.com/Shopify/shopify/pull/45360)

Tinymce leak: I've looked in to this but didn't have time to solve it. It's setting up jquery event listeners all over the place and nothing's cleaning them up.

[DrewMartin]

All that to say:

Is define really the root cause here, or just a symptom? Is it turbograft itself actually causing the problem?

Seeing them leak is just the symptom. It mostly has to do with jQuery storing the event callbacks internally, which keeps the whole object it was defined in alive if the function was bound or in a closure. Like both of these event listeners will cause a leak:

constructor: ->
  $(document).on 'resize' =>
    @resizeCallback
  $(document).on 'scroll', @scrollCallback

  resizeCallback: ->
  scrollCallback: =>

the resize listener leaks because it contains a closure that references the object, and the scroll callback leaks because scrollCallback is bound to the object which keeps it around.

This does not leak (but is probably not very useful for our purposes) because the callback retains no reference to the original object:

constructor: ->
  $(document).on 'resize', ->
    console.log('window resized!')

[qq99]

Interesting, those examples above with $(document).on don't leak by virtue of binding on document which never gets replaced?

[DrewMartin]

Basically. They leak because we're never calling $.remove on them like we do on nodes in the body when on a partial or full refresh

[pushrax]

Yep, another :+1: that define isn't the cause here – it will be collected along with the node if there are no external references.

[nickjs]

Right, so what if (and this is crazy, I know) what if we remove all event listeners from the document on every page load? That would require any modules to be proactive about readding themselves after a page load, but at least that's more explicit than the subtle bug of leaking memory because you forget to remove yourself after a page load.

[lemonmade]

Is it even possible to just detach all listeners, including those not added via jQuery? I feel like this is invisible behaviour that will make code look like it's leaking when it's not. A component that has a resize listener on window looks fine when it attaches only on $ ->, but will look like it's leaking if we force it to reattach on every page:load. Maybe that's no biggie and we will get over it, but I feel like we shouldn't enable sloppy coding.

[DrewMartin]

Knee jerk reaction is that that's a bad idea for a couple reasons:

• Libraries we use might be setting up listeners that we would then be tearing down, breaking them when you start navigating around
• In addition to window and document leaks could happen if you're listening on any element that is not refreshed.

[nickjs]

@lemonmade No, it's only possible to remove all jQuery listeners. But that very well may solve the problem enough, since all of our code should be adding listeners via jQuery.

[DrewMartin]

I'm not sure if event listeners added with native DOM code leak. The browser might be able to do something smart about it during garbage collection. With jQuery it's internally storing the callbacks and that's where it leaks

[nickjs]

@DrewMartin yup, like I said it's pretty crazy. However anything that uses the tg load event will be fine, and we could technically swizzle $() to use the tg load event.

Anyway it's not worth exploring this idea for now, let's keep fixing individual leaks as we find them.

[nickjs]

I'm not sure if event listeners added with native DOM code leak. The browser might be able to do something smart about it during garbage collection

Pretty sure all listeners will leak, unless it's a listener on a node and that node has been removed from the DOM, will get gc'd, and the closure doesn't reference any objects that reference the same node.

[marutypes]

I kind of feel like the best answer to this is just having our own on method that sets up the Page.onReplace remove logic itself. Maybe that's crazy though?