Site navigation creates increasingly detached DOM nodes
diegodorado opened this issue · 15 comments
Description
When I navigate to another page, new DOM nodes are created, but old one aren't disposed or even cached.
Steps to reproduce
gatsby new gatsby-site
cd gatsby-siteEdit index.js and page-2.js to get as minimal as posible:
// index.js
import React from "react"
import { Link } from "gatsby"
const IndexPage = () => (<Link to="/page-2/">Go to page 2</Link>)
export default IndexPage// page-2.js
import React from "react"
import { Link } from "gatsby"
const SecondPage = () => (<Link to="/">Go back to the homepage</Link>)
export default SecondPagegatsby build
gatsby serveOpen localhost:9000 on chrome, and open the Performance Monitor.
Every time you click on the links, DOM Nodes increase (but never gets garbage collected)
Expected result
DOM Nodes should remain more or less steady
Actual result
DOM Nodes keeps increasing on each click
Environment
System:
OS: Linux 4.15 Linux Mint 19 (Tara)
CPU: (8) x64 Intel(R) Core(TM) i7-6700HQ CPU @ 2.60GHz
Shell: 5.4.2 - /usr/bin/zsh
Binaries:
Node: 10.15.3 - ~/.nvm/versions/node/v10.15.3/bin/node
Yarn: 1.16.0 - ~/.nvm/versions/node/v10.15.3/bin/yarn
npm: 6.4.1 - ~/.nvm/versions/node/v10.15.3/bin/npm
Languages:
Python: 2.7.15 - /usr/bin/python
Browsers:
Chrome: 72.0.3626.119
Firefox: 65.0.1
npmPackages:
gatsby: ^2.8.2 => 2.8.2
gatsby-image: ^2.1.2 => 2.1.2
gatsby-plugin-manifest: ^2.1.1 => 2.1.1
gatsby-plugin-offline: ^2.1.1 => 2.1.1
gatsby-plugin-react-helmet: ^3.0.12 => 3.0.12
gatsby-plugin-sharp: ^2.1.3 => 2.1.3
gatsby-source-filesystem: ^2.0.38 => 2.0.38
gatsby-transformer-sharp: ^2.1.21 => 2.1.21
npmGlobalPackages:
gatsby-cli: 2.6.4
Here's a gif showing this behaviour
This has been discussed in a previous issue for reference: #12198
I’m willing to work on this, any hints to where I should begin with?
Will try and work on it this weekend!
Will try and work on it this weekend!
Let me know if I can be of any help.
Sooo, I dug around a bit, created a repro repo, managed to get some insights with the Chrome Dev Tools, however it appears my knowledge of the inner workings of Gatsby isn’t deep enough to know where to start 😅
What I did:
- Created a repro repository following @diegodorado’s repro: https://github.com/phacks/gatsby-memory-leak-repro. Bumped the number of
<a>s to 10 so that it’s easier to see the impact. - Played around with snapshots, and I noticed that the increased DOM size seems to come from “Detached DOM Nodes” :
Before clicking on a link:
After clicking on a link:
Per this SO answer, Detached DOM Nodes are:
HTML elements are instance of objects that consume memory. Each such element can store on them event listeners and data associated with it. Now "Detached DOM Trees" are nothing but DOMs that are in the Browser's memory but are NOT attached to the main DOM tree aka "Document DOM Trees".
Hopefully this may help @diegodorado or someone else have some ideas about where to start!
I wonder whether this issue is related to facebook/react#15157
@oorestisime Interesting issue, nice catch!
When I patch, in the repro, react-dom by the discordapp#react suggested dirty fix (mentioned in facebook/react#15157 (comment)), no “Detached DOM Nodes” appear in subsequent snapshots, so it seems to be doing something.
However, the DOM Node counter is still increasing, which gets me even more perplex.
I updated the repro repo (https://github.com/phacks/gatsby-memory-leak-repro) to explain how to reproduce (and how to visualise) the leak, so hopefully it’ll be easier to help along.
To test out Discord’s hotfix, you may change the following in the package.json:
- "react-dom": "^16.8.6",
+ "react-dom": "discordapp/react#cfda84f6b3c49a1398709cf43b3b959366f7e01a",Hiya!
This issue has gone quiet. Spooky quiet. 👻
We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contributefor more information about opening PRs, triaging issues, and contributing!
Thanks for being a part of the Gatsby community! 💪💜
Looks like a memory leak was fixed in React very recently (see this long discussion about the topic).
I’ll try and have a look this week to see whether that fixes the problem in Gatsby!
Update: tried running the example repo against the master branch of react and react-dom, and it did not fix the memory leak.
Jumping in to take a look at the repro repo to see if I can glean some insight from it! I'll keep a running update as an edit of this comment for now.
Update 1
I'm noticing that there's a direct correlation to how many intersection observers are also detached, as well as event listeners related to the intersection observers, screenshot below.
If I had to guess, it has something to do with GatsbyLink's refs or createIntersectionObserver method holding on to a reference of an element somewhere.
Oops, I fixed the IntersectionObserver leak a while back but completely forgot to update this issue!
Your insight is spot on, you can see how it was fixed in this PR: #17056
I’ll close the issue now to avoid further confusion, however if there’s another leak in there we can totally reopen it. Sorry about that @RyKilleen !