search

devrelm / resize-observer

Other
98 stars 12 forks source link

Fix memory & cpu leak after disconnect() #93

Closed thgreasi closed 3 years ago

thgreasi commented 3 years ago

ResizeObserver instances are not eligible for garbage collection since the top level resizeObservers still has a reference to them. As a result whatever was referenced by the callback provided during instantiation, is also not garbage collected.

devrelm commented 3 years ago

Great catch! Thanks for your help!

devrelm commented 3 years ago

@thgreasi

This has been released with v1.0.1.

Thanks again!

devrelm commented 3 years ago

@thgreasi

I may have been a little quick to merge this.

The disconnect() is only supposed to clear all observationTargets and activeTargets.

As-is, this PR goes a step further to prevent the ResizeObserver from being used again.

To counter this, we would need to re-add the ResizeObserver to the resizeObservers list.

I'm going to unpublish v1.0.1, and release a v1.0.2 with this fix in the next couple hours.

thgreasi commented 3 years ago

As-is, this PR goes a step further to prevent the ResizeObserver from being used again.

@devrelm apologies for not noticing.

To counter this, we would need to re-add the ResizeObserver to the resizeObservers list.

But that would bring back the memory leak b/c the provided callback still being reachable through the ResizeObserver instance in the resizeObservers list. Perhaps the should be using a WeekSet to avoid this (when available). What do you think? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakSet/WeakSet

No that ^ wouldn't work, b/c you can't enumerate the contents of a WeakSet, which we need here...

thgreasi commented 3 years ago

@devrelm here is a proper fix for this: https://github.com/pelotoncycle/resize-observer/pull/95

devrelm commented 3 years ago

@thgreasi This is now fixed with v1.0.2.