Memory Leak when using Affinity

[erik-ciq]

• Output of node_modules/.bin/electron --version: v4.0.1
• Operating System (Platform and Version): Windows 10 Home v1803

Expected Behavior
Not leak memory when using affinity

Actual behavior
Memory leak only when using affinity

To Reproduce
git clone git@github.com:erik-ciq/electron-affinity-leak.git
cd electron-affinity-leak && npm install
npm run leak

Repo readme has more details.

Screenshots

Additional Information
Only requires a single browser window, with affinity, and using location.reload to refresh. Leaks ~ 1.5Mb of memory in renderer process each time. You can run npm start to run the same code without affinity prop set. You'll see that there is no leak. The leakage only occurs with affinity.

The repo provided also takes heap snapshots at each interval, and provides additional memory usage data if needed.

Probably the base case of #12027

There are many similar issues reported in chromium, but it seems like they are related to context leaking from a child iframe into a parent window (causing a strong reference that prevents the iframe from being garbage collected). Here are some links around this for posterity:

https://bugs.chromium.org/p/chromium/issues/detail?id=529941
https://bugs.chromium.org/p/chromium/issues/detail?id=852408
https://bugs.chromium.org/p/chromium/issues/detail?id=270000
https://bugs.chromium.org/p/chromium/issues/detail?id=609137

I also tried to reproduce in chrome using iframes, but it seems like it's only electron:

https://www.bryntum.com/blog/debugging-memory-leaks-in-web-applications-using-iframes/

Using the gist below, you can cause the chromium leak by holding a reference from the child iframe in the parent window. You can "fix" the leak by removing this reference.
Reproduce chromium iframe leak gist

Given that I was able to "remove" the iframe leak by removing the child reference in the parent (from the gist), and given running the repo I created for electron without affinity does not seem to leak, it points towards this being an issue with the affinity implementation in electron.

Diff from electron-quick-start: https://github.com/erik-ciq/electron-affinity-leak/pull/1/files#diff-7a9076d6d94e62c13d641aa71f19ae8eR63

@emmkimme any ideas on what might cause this? Not sure if this is a regression or has existed since #11501

[welcome]

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

To help make it easier for us to investigate your issue, please follow the contributing guidelines.

[erik-ciq]

I was able to add the process.getHeapStatistics function into electron 2.0.0-beta.1 and confirm that this is not a regression, and has been there since affinity was added.

[erik-ciq]

Comes from:

electron/atom/renderer/atom_renderer_client.cc

Line 153 in 52e2576

// node::FreeEnvironment(env);

. Will investigate.

[jaehayoo]

Thanks, @erik-ciq

Is there any update? :)

This issue is same in Electron v3.x v2.x

We are developing the application which have many sub game windows.
So, We should use 'affinity' since there is performance issue for many sub windows when we did not use affinity.
By the way, This app's sub windows will be frequently closed and opened by users.
So, This memory leak issue with 'affinity' is so so super important for us.
Please patch the memory leak issue about using affinity.

Actually, I am waiting feedback from #12027

Thanks,
Best regards,

[erik-ciq]

@MarshallOfSound you mentioned some work on this issue related to a PR you were developing. Is there any status update on that PR?

[erik-ciq]

Tested on a build of 5-0-x branch with nodeIntegrationInSubFrames: true in BrowserWindow webPreferences. Fixes the leak. Would be nice to have the leak fixed in code paths where nodeIntegrationInSubFrames: false.

[erik-ciq]

@MarshallOfSound are there plans to incorporate this todo into a release? 0890b0c#diff-0cdfa12fff513e022fac830c6af9c19aR160

[MarshallOfSound]

@erik-ciq To be honest, my current personal thinking is affinity should be removed from the code base. That TODO will eventually reach other code paths but it will take a long time. I have a plan for this stuff that I'll be posting as a discussion issue some time this week.

Edit --> Discussion Issue: #18397

[emmkimme]

@MarshallOfSound, Please, this feature is used in our released flagship product. Our product is managing a hundred of pages, Be able to gather some of them in a single process, save memory and prevent to reach the limit of number of processes in Chromium, this is critical for us. We can add the 'Experimental' flag on this feature to mitigate the usage. Thanks

[MarshallOfSound]

@emmkimme Are those hundreds of pages on the same origin? The goal here is to just enable the same process model as Chromium. Maybe I'm missing your use case here, but at least in my mind there is no reason to change from Chromiums process model if we restore that (same origin will be in the same process). It sounds like what you want is a clean way to use Process Per Site

See #18397 for the plans to restore Chromiums process model.

Please note that we haven't even discussed affinity or potentially removing it, what I mentioned above was entirely my opinion not a direction of the project 👍 It just so happens that removing the patches mentioned in that post would also remove the affinity patches.

[emmkimme]

we can not use the process-per-site option as all our pages have the same origin (domain of the company)

[MarshallOfSound]

@emmkimme Noted, but my opinion still stands. I'll raise this at the next WG meeting

[thorsent]

Love affinity, but I think process-per-site for us would address the memory leak in our most common use case. I think the ability to set the process model with a command line switch would be useful.

I wonder if process-per-tab provides a path to affinity-like behavior for @emmkimme's situation?

[Xylobol]

@MarshallOfSound, Please, this feature is used in our released flagship product. Our product is managing a hundred of pages, Be able to gather some of them in a single process, save memory and prevent to reach the limit of number of processes in Chromium, this is critical for us. We can add the 'Experimental' flag on this feature to mitigate the usage. Thanks

I'd like to second the Experimental flag. In a project I'm working on, we're also using affinity quite extensively. Removing it would almost definitely result in us completely forking Electron to keep the feature (we already have an in-house fork for simpler things).

[cynx]

@MarshallOfSound, Please, this feature is used in our released flagship product. Our product is managing a hundred of pages, Be able to gather some of them in a single process, save memory and prevent to reach the limit of number of processes in Chromium, this is critical for us. We can add the 'Experimental' flag on this feature to mitigate the usage. Thanks

I'd like to second the Experimental flag. In a project I'm working on, we're also using affinity quite extensively. Removing it would almost definitely result in us completely forking Electron to keep the feature (we already have an in-house fork for simpler things).

@MarshallOfSound I second this as well. we have use-case to open multiple windows and using affinity is performant.

[erik-ciq]

Worth noting that any objects defined in the context for a window with affinity will also leak. I updated the leak branch of git@github.com:erik-ciq/electron-affinity-leak.git to include a large string in the renderer.js file.

[erik-ciq]

I added some additional command line flags to git@github.com:erik-ciq/electron-affinity-leak.git to demonstrate leaking when you close and reopen windows with the same affinity.

Closing a window with affinity (terminating the process), and then opening a new window with that same affinity will produce a leak.

If you use a random affinity each time you close and reopen processes you will not have the leaking issue. It's only when you re-use affinity. If you close all windows within an affinity to terminate the process, and then follow that by creating a new window with that affinity, you will reap leaked memory equal to whatever was in that same affinity previously.

You can recover the memory for a given affinity by using process.crash in the render. This is the best way I found to recover leaked memory without constantly randomizing affinity: webContents.on('crash', recover); Example of closing and reopening with the same affinity here:

^ notice how the memory leak is recovered after the crash.

git@github.com:erik-ciq/electron-affinity-leak.git should have everything people need to investigate their own issues more thoroughly.

[electron-triage]

Thank you for taking the time to report this issue and helping to make Electron better.

The version of Electron you reported this on has been superseded by newer releases.

If you're still experiencing this issue in Electron 6.x.y or later, please add a comment specifying the version you're testing with and any other new information that a maintainer trying to reproduce the issue should know.

I'm setting the blocked/need-info label for the above reasons. This issue will be closed 7 days from now if there is no response.

Thanks in advance! Your help is appreciated.

[electron-triage]

Thank you for your issue!

We haven't gotten a response to our questions in our comment above. With only the information that is currently in the issue, we don't have enough information to take action. I'm going to close this but don't hesitate to reach out if you have or find the answers we need, we'll be happy to reopen the issue.

[Liting1]

The problem still exists
"electron": "^10.1.6"

[emmkimme]

Unfortunately, it had been decided to remove the affinity feature from Electron, so no fix/update are planned.
See #26874