lifeart / ember-ref-bucket

This is list of handy ember primitives, created to simplify class-based dom workflow
MIT License
36 stars 14 forks source link

Memory leak when using {{create-ref}} #50

Closed tniezurawski closed 1 year ago

tniezurawski commented 1 year ago

Something is leaking when using {{create-ref}}. When running a memory snapshot after running tests I see an application hanging around. I was able to pinpoint it to this addon and {{create-ref}} usage.

image

The way I use it (source):

<span {{create-ref "FavouriteNode"}}>Lorem ipsum</span>

Steps to reproduce:

  1. I created a minimal repo for reproduction. Please clone it -> https://github.com/tniezurawski/ember-ref-bucket-memory-leak

The repo is bare-bone. All I did was:

  1. Run tests with this command: ember test -s -f "Integration | Component | tomek-test". No more tests there but you'll be sure that you are running only the one.
  2. Open Chrome devtools > Open Memory > click "Take snapshot"
  3. Search for "Container" as this indicates an application instance hanging around
  4. Select "Container" as on the screenshot and look at "Retainers". You'll find references to this addon

I didn't check yet why this is happening as I don't have knowledge about internals but I hope we can get it fixed together :)

lifeart commented 1 year ago

Hey @tniezurawski! Thank you for report! Likely it's related to global ref workaround https://github.com/lifeart/ember-ref-bucket/blob/master/addon/utils/ref.js#L17 there is no cleanup logic for it. We could try to register app destructor to null this ref

tniezurawski commented 1 year ago

Thanks @lifeart I literally was about to write about it 😅 Because I was playing with the addon code directly in node_modules and introducing a clean-up function that fixes the memory leak:

image

image

Likely it's related to global ref workaround

Could you tell me more about the workaround? Would a code like that on my screenshots be ok? I can prepare a PR tonight/tomorrow.

EDIT: That implementation is probably too naive. I'm afraid of the "last" word in the name of the variable. I guess, before removing it we should check if that's the same "local ref" as "last global ref" if I understand it correctly. Anyway, I'll know more with some explanation about the workaround 😉

lifeart commented 1 year ago

@tniezurawski we have global ref for few cases:

  1. If there is no context on template.
  2. If we need to have access to DOM element from different parts of application.

Ideally, we need to set globalRef in application initializer (once), and create application level destructor to null it once application destroyed.

But, I don't remember why we don't have it in app instance initializer.. (maybe I'm lazy ass to write it initially). That's why we set it in modifier, because once anybody use this addon, modifier will exist anyway.

Proposed fix looks good (binding cleanup to modifier cleanup, but may have side effects with multiple modifiers - first removed will null global ref and may cause runtime erros)

We could try do few things:

  1. register destructor inside modifier, but binded to app instance (getOwner(this)).
  2. implement app-initializer flow to register / remove globalRef