memory leaks with polyfill

[aFarkas]

I made a simple demo that modifies the polyfill so that even browsers with native support use the polyfill code instead of the native one and that showcases that the polyfill leaks memory. You can see this in action here: https://jsbin.com/fajahenoca/edit?html,js,output

Additionally I created a proof of concept, that shows a way to add an IO polyfill, that does not leak memory.

A demo of this can be found here: https://jsbin.com/necilumuze/edit?html,js,output.

And a diff can be seen here:
aFarkas/IntersectionObserver@gh-pages...memory-leaks-poc

The trick I'm using here, is to not create strong reference by adding the elements to an array, but to add a marker attribute to the elements, that need to be observed. This way the observer can search for those marked elements in the document.

It's obvious that this technique is not so clean, but it's the only way I can think of to not produce memory leaks.

In my eyes having a not leaking IO polyfill is key to use it in production.

[philipwalton]

Seems like the primary problem here is the polyfill not accounting for DOM element removal. I think this could be fixed by adding a check when elements that are also observation targets are removed from the DOM, and if an IntersectionObserver instance doesn't contain any targets in the DOM, it's removed the the registry.

One problem with this is I believe as spec'ed an IntersectionObserver instance should continue observing a target that's put back in the DOM, which would break my above idea.

Another option would be use to use WeakMap to hold a reference to targets in browsers that support it.

[aFarkas]

Thanks @philipwalton for your feedback.

Seems like the primary problem here is the polyfill not accounting for DOM element removal.

With this and the following sentences you mean the current state of the polyfill, not my modification, right?

Basically all this is solved by the technique above. As soon as the DOM element is removed from the document it is not observed from my version and if it is put back into the document it is re-observed again.

The only thing that my modification can not fully handle is the situation, where the page author removes all observed elements from the document and doesn't have any reference to those elements to put them back in the document.

In that case the scroll listeners can't be removed, because we can't differentiate this case from having a reference to put some target back to the document.

But in my eyes this is already a big improved to the current situation, where the listener also stays added, but additionally the check method runs over all removed elements with each scroll/mutation, altough they are removed from the document etc..

Another option would be use to use WeakMap to hold a reference to targets in browsers that support it.

I haven't worked yet with WeakMap, but as I thought about it, I couldn't make WeakMap work here, because you need to have something like an iterable WeakSet or another kind of a weak reference to the elements.

[agustinhaller]

@philipwalton I've seen an implementation of the IntersectionObserver polyfill using WeakMap. Here's the github repo by @que-etc https://github.com/que-etc/intersection-observer-polyfill.
Maybe it's worth merging both polyfills.

[aFarkas]

@agustinhaller The polyfill by @que-etc doesn't use a WeakMap to reference the observed targets, hence it is also leaking memory. (WeakMap is used for something else.)

But studying other implementations might inspire things.

[aFarkas]

What do you think about this POC? Should I work on this? It's currently the only IntersectionObserver polyfill without an obvious memory leak.

[ArrowBaZ]

Hello @aFarkas your polyfill work better like this, thanks you !

[Rendez]

Could someone please make clear to me whether the Chrome implementation holds a weak or strong reference to the observed elements? I had a Set() of elements which I was debugging and I tried .clear() and .unobserve() on the Observer instance and the .takeRecords() function kept returning the 1 element. Maybe it was while in the devtools debugging so I wanted to know for sure. Mind I wasn't loading the polyfill for Chrome.

[gerasimvol]

I'm not using any polyfils. When I .unobserve() browser still keep in memory unobserved elements?

I'm using IntersectionObserver in Vue directive, so amount of Vue instances are growing, when using site. I think it is because IntersectionObserver keep some data in memory.

[miketaylr]

The polyfill is being removed in #499 (see #433 for discussion).

If you can still reproduce bugs in the native implementations, filing bugs in the various bug tracks would be really helpful.

I'm going to close this as wontfix, but thanks for taking the time to report this issue!