Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fast memory leak/creep when cy.get fails to find the element, and eventually crashes #18549

Closed
haskida opened this issue Oct 19, 2021 · 8 comments · Fixed by #19311
Closed

Fast memory leak/creep when cy.get fails to find the element, and eventually crashes #18549

haskida opened this issue Oct 19, 2021 · 8 comments · Fixed by #19311

Comments

@haskida
Copy link

haskida commented Oct 19, 2021

Current behavior

While cy.get() retries to find an element , the memory usage of the tab shoots up quickly

See video https://youtu.be/N5jtyBdd05Q

Desired behavior

Memory usage should not increase this quickly

Test code to reproduce

describe('Memory issue', () => {
it('Simple memory test', () => {
cy.visit('https://www.androidpolice.com')
cy.get('.garbageId')
})
})

Cypress Version

5.1.0 and latest 8.6.0

Other

Initially I found this issue while testing my company's website, then I found that it is easily reproducible with other websites as well

image

Pre-requisistes

  • numTestsKeptInMemory has to be set to 1 or higher
  • set defaultCommandTimeout to a higher value (eg. 50000) so there's more time for the memory to increase

Steps to reproduce

  1. Visit a website (eg. https://www.androidpolice.com)
  2. Use cy.get() to find an non-existent element

As you can see in the video linked above, the memory usage starts shooting up when Cypress is trying to find the element.
When you re-run the test without closing the tab, the memory doesn't reset and keeps adding up. Eventually when the usage reaches 4GB, the tab crashes with the "Aw snap" message.

This issue is a lot more apparent if the test is longer with more steps, which can put the memory usage close to the ceiling of 4GB.

So far the reliable workaround is to set numTestsKeptInMemory to 0, which stops the memory creep altogether.

Very occasionally I can observe the garbage collection kicking in and reducing the memory usage, however the cy.get() still rapidly increases the memory usage after

@haskida
Copy link
Author

haskida commented Nov 4, 2021

Can someone take a look? I can provide more info if needed

@haskida haskida changed the title Fast memory creep when cy.get fails to find the element, and eventually crashes Fast memory leak/creep when cy.get fails to find the element, and eventually crashes Nov 8, 2021
@haskida
Copy link
Author

haskida commented Nov 17, 2021

ping @jennifer-shehane

@BlueWinds
Copy link
Contributor

BlueWinds commented Nov 18, 2021

Thanks for the report (with easy to follow repro steps! <3) and the bump, this is indeed a serious issue.

I played with this a bit, and you're definitely right that there's a serious memory leak. All my tests are in Cypress 9.0.0. Some results:

  • Electron 94 (bundled with Cypress 9.0.0) climbs in memory usage at about 100mb / s.
  • Chrome 95 and Chromium 95 did the same, memory growing steadily until it froze or crashed.
  • Firefox 90 doesn't show any such issue, it appears perfectly healthy regardless of how long it's run.

Some initial debugging:

  • Setting { log: false } on the cy.get() command didn't help. Not that this would be a valid workaround, just that it would have been useful to know if the issue was related to our logging.
  • The issue isn't unique to get(). For example cy.get('body').find('.garbageId') or cy.focused() (when there is no focused element) cause the same leak.
  • It's not all failing commands that have this issue though. For example, cy.get('body').type('foo) does not cause a memory leak, even though it also eventually throws an error.

So basically: Commands that continually query the DOM seem to be causing issues in non-Firefox browsers. I'll make sure someone on the team looks into it - there are several other issues open recently that this might be a root cause for.

Edit: Inspired by #17853, I tried this back on Cypress 8.1.0, with the same results - memory leak is also present there.

@haskida
Copy link
Author

haskida commented Nov 18, 2021

Awesome! thanks for looking into this!

@cypress-bot cypress-bot bot added stage: backlog and removed stage: ready for work The issue is reproducible and in scope labels Nov 19, 2021
@BlueWinds
Copy link
Contributor

BlueWinds commented Dec 8, 2021

Poking around here further, it's because of DOM snapshotting. Whenever an assertion fails (badSelector doesn't exist on the page), we call Cypress.log(). https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/cy/assertions.ts#L200. This happens several times per second as we retry the query, and because this is a DOM-related assertion, we snapshot the dom for each failure.

In Firefox, these snapshots get cleaned up efficiently, but in the current electron version (and certain chromium / chrome versions) they stick around in memory. While it might be possible to clean up the snapshots better, I'm still investigating why we make dozens of copies of the DOM per second. Even if they're GCed regularly, it still seems wasteful. Leaking memory at 100mb / second means that we're allocating memory at 100mb / second.

All this is fairly deem in the Cypress internals, and not easy to debug / change regardless.

@cypress-bot cypress-bot bot added stage: needs review The PR code is done & tested, needs review stage: work in progress and removed stage: to do stage: needs review The PR code is done & tested, needs review labels Dec 8, 2021
@BlueWinds
Copy link
Contributor

So a bit of an update here - the retries were the dragon's tail. Tug it, and you get not a snake but a dragon nesting in the depths of the codebase with its treasure horde and toothy grin.

I've updated the attached PR with a better solution, which should reduce unnecessary DOM snapshotting in any situation where implicit assertions that an element exists in the DOM fail - including temporarily while we're waiting for an element to be actionable in tests that eventually pass.

@cypress-bot cypress-bot bot added stage: needs review The PR code is done & tested, needs review stage: work in progress and removed stage: work in progress stage: needs review The PR code is done & tested, needs review labels Dec 16, 2021
@cypress-bot cypress-bot bot added stage: work in progress stage: needs review The PR code is done & tested, needs review and removed stage: work in progress stage: needs review The PR code is done & tested, needs review labels Dec 17, 2021
@cypress-bot cypress-bot bot added stage: pending release and removed stage: needs review The PR code is done & tested, needs review labels Dec 20, 2021
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 20, 2021

The code for this is done in cypress-io/cypress#19311, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 21, 2021

Released in 9.2.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v9.2.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Dec 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants