Memory leak & onFatalError in DragDropContext

[faiwer]

Hello. I started to check memory leaks in our application, and found a lot of them :(
One of them relies on react-beautiful-dnd. The leak is in the DragDropContext. Let's dive in:

• We have class-component <DragDropContext/>
• This class contains: onFatalError = (error: Error) => {...}
• and onWindowError = (error: Error) => this.onFatalError(error)
• and in componentDidMount method we make a subscription: window.addEventListener('error', this.onWindowError)
• ... in componentWillUnmount - window.addEventListener('error', this.onWindowError);

What's wrong with it? These subscriptions retain in memory <DragDropContext/> instances forever. And all their links too, of course. They aren't removed from the memory because of onWindowError and onFatalError. Both of these methods are declared in the class-scope and have link to this object. So we have memory leak.

^^^ a lot of them :)

I don't know why DragDropContext needs these subscriptions, but it would be much better to remove them, or rewrite this code without memory leaks ;)

[alexreardon]

I can see the issue. It should be a simple fix. I can get to it on Monday 👌

…

On Fri, 17 Aug 2018 at 8:49 pm, faiwer ***@***.***> wrote: Hello. I started to check memory leaks in our application, and found a lot of them :( One of them relies on react-beautiful-dnd. The leak is in the DragDropContext. Let's dive in: - We have class-component <DragDropContext/> - This class contains: onFatalError = (error: Error) => {...} - and onWindowError = (error: Error) => this.onFatalError(error) - and in componentDidMount method we make a subscription: window.addEventListener('error', this.onWindowError) - ... in componentWillUnmount - window.addEventListener('error', this.onWindowError); What's wrong with it? These subscriptions retain in memory <DragDropContext/> instances forever. And all their links too, of course. They aren't removed from the memory because of onWindowError and onFatalError. Both of these methods are declared in the class-scope and have link to this object. So we have memory leak. [image: img_17_08_01_049a068531d] <https://user-images.githubusercontent.com/744114/44262407-c3b5a900-a23c-11e8-91fd-70cdcfa6d42a.png> ^^^ a lot of them :) [image: img_17_08_01_8efde6c043c] <https://user-images.githubusercontent.com/744114/44262447-eb0c7600-a23c-11e8-9d38-77b9f7769c6b.png> I don't know why DragDropContext needs these subscriptions, but it would be much better to remove them, or rewrite this code without memory leaks ;) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#715>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACFN7cXjgWaA4E-YsLBTNZGNS2749Slnks5uRp-igaJpZM4WBXNV> .

[faiwer]

There's also 1 more memory leak. But I haven't understood them yet. This one is more intricate. It's about this code:

  var unbind = function unbind() {
    if (!isBound) {
      return;
    }

    isBound = false;
    unbindEvents(getWindow(), pointerEvents, {
      capture: true
    });
  };

bind and unBind methods in createPostDragEventPreventer.

We again have window object and some relations with subscriptions. But.. also we have a matreshka of unbindWindowEvents :)

[alexreardon]

Let me know if you see any others

…

On Fri, 17 Aug 2018 at 10:26 pm, faiwer ***@***.***> wrote: There's also 1 more memory leak. But I haven't understood them yet. This one is more intricate. It's about this code: var unbind = function unbind() { if (!isBound) { return; } isBound = false; unbindEvents(getWindow(), pointerEvents, { capture: true }); }; bind and unBind methods in createPostDragEventPreventer. [image: img_17_08_01_d0ed444efcd] <https://user-images.githubusercontent.com/744114/44266004-bc959780-a24a-11e8-889d-80519951fdb2.png> We again have window object and some relations with subscriptions. But.. also we have a matreshka of unbindWindowEvents :) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#715 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACFN7fgFsNaZKz0fF5NrmhOioRNQeyRGks5uRrZqgaJpZM4WBXNV> .

[alexreardon]

Also, what tool are you using?

…

On Fri, 17 Aug 2018 at 10:39 pm, Alex Reardon ***@***.***> wrote: Let me know if you see any others On Fri, 17 Aug 2018 at 10:26 pm, faiwer ***@***.***> wrote: > There's also 1 more memory leak. But I haven't understood them yet. This > one is more intricate. It's about this code: > > var unbind = function unbind() { > if (!isBound) { > return; > } > > isBound = false; > unbindEvents(getWindow(), pointerEvents, { > capture: true > }); > }; > > bind and unBind methods in createPostDragEventPreventer. > [image: img_17_08_01_d0ed444efcd] > <https://user-images.githubusercontent.com/744114/44266004-bc959780-a24a-11e8-889d-80519951fdb2.png> > > We again have window object and some relations with subscriptions. But.. > also we have a matreshka of unbindWindowEvents :) > > — > You are receiving this because you commented. > > > Reply to this email directly, view it on GitHub > <#715 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ACFN7fgFsNaZKz0fF5NrmhOioRNQeyRGks5uRrZqgaJpZM4WBXNV> > . >

[faiwer]

It's about the previous one. These unbind methods retain all their HTML & Fiber nodes.

But I can't say the certain place in the code and certain reason, that occurs this leak.

Also, what tool are you using?

I use ChromeDevTools. Their Memory tab.

• I make a snapshot of the application
• then I work with my application (click, click, click, click...)
• then click to "collect garbage" icon (trash-icon)
• then I make another snapshot
• then I compare them and search in differences what they are and why they are :)

It's a tricky deal, but it's only what I know about how to fight with memory leaks :)

[alexreardon]

I can fix the first one easily. The second one will take some more investigation