-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
fix: memory leak caused by storing base64 encoded files recieved by CDP Network.requestWillBeSent
#22460
fix: memory leak caused by storing base64 encoded files recieved by CDP Network.requestWillBeSent
#22460
Conversation
Thanks for taking the time to open a PR!
|
@DJSdev, Thanks so much for this PR. Would it be possible to get a test written for it? |
I'll do my best to get a test written for it. It's in a weird place. |
I'd be fine with a test that just emits a |
grammar :) Co-authored-by: Sam Tsai <samtsai@gmail.com>
grammar :) Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
Unit tests have been added. I added some bonus tests around
I wasn't exactly sure how the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
We have been having some flake problems with our automated test suite recently, which we're working on addressing in several other PRs. Given that the currently failing tests were known to be flaky before this branch, I'm going to exercise my judgement and merge this without waiting for a completely green build. |
Is this fix available on 10.2.0 version? |
@Chili82 No. 10.2 was released 2 days ago and this PR got approved just today. |
Ok, thanks. I guess it will be in the next release. |
We're hoping to get 10.2.1 out tomorrow, but it's not ready quite yet. |
Thank you! |
Does this fix also resolves the issue where it closes the test before it kick start with the error "The Test Runner unexpectedly exited via a close event with signal SIGILL"? |
Doesn't sound related to this fix or the linked issue. |
…esser/CLOUD-577-spec-list-display-latest-runs-batching * muaz/CLOUD-577-spec-list-display-latest-runs: fix: Update "Request Access" button state after requesting access (ACI) (#22499) feat: Support "Queued" latest run status (#22497) fix: remove ctx.cloud.reset in tests, handle infinite loop in stale results (#22483) chore: add reporter webpack to gulp watch script (#22386) fix: Increase timeout for npm-webpack-dev-server tests (#22489) fix: Time out unmatched prerequests in proxy to avoid leaking memory (#22462) fix: Sort results in findCrossOriginLogs test helper to deterministic (#22481) fix: memory leak caused by storing base64 encoded files recieved by CDP `Network.requestWillBeSent` (#22460) fix: Improve cross-origin cookie handling (#22320) feat: Add button to clear value from search fields (#22202) chore: Add test to verify settings panels are collapsed by default (#22382) fix: process_profiler follow up work for v10 (#22363) chore: Update Chrome (stable) to 103.0.5060.53 (#22441) refactor: use design system windicss config (#21503) chore: update readme logo (#22433) chore: Update Chrome (beta) to 103.0.5060.53 (#22351) chore: updating version (#22432) Trigger Build
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
User facing changelog
Fixed a memory leak in Chromium-based browsers caused by storing
data:
url containing base64 encoded files unnecessarily. Fixes #17853Additional details
Summary
There is a memory leak in the
CdpAutomation
class that causes Cypress to store data urls as requests in thependingBrowserPreRequests
array.Background
After updating Cypress to any version higher than 8.2, my e2e suite crashes with the following error after about an hour:
While the tests run, I inspected task manager and I noticed the Cypress server process growing in size until the machine's memory is exhausted causes this issue.
Investigation
I started off by adding code to Cypress to have it dump it's heap memory every 90 seconds.
After 3 minutes of tests, you can see a big difference in the heap sizes between the Cypress 8.2 (first entry) and Cypress 8.1 (second entry)
After inspecting the heap memory in the DevTools console I discovered that Cypress is storing hundreds/thousands of copies of a font file.
This is a font-file used by my application and it's referenced like so:
I discovered in Chrome, when a CSS style sheet uses the
url()
function in thesrc
field with a base64 encoded file (adata:
url), it still triggers Chrome to "send" a network request even though it's not actually going to send one.Cypress subscribes to the
Network.requestWillBeSent
event in CDP. When Chrome goes to 'fetch' the base64 encoded file, it sends arequestWillBeSent
event to Cypress which stores the url (literally a whole file) in thependingBrowserPreRequests
array. Every time the page is loaded with the CSS, a request is sent adding to the buffer until Cypress eventually crashes.Solution
The solution I went with is very basic. Simply filtering out
data:
urls from from theNetwork.requestWillBeSent
event handling logic should clear it up.Another thought was to have logic clear out stale entries from the array, but that's a lot of work for URLs that shouldn't be stored to begin with.
Steps to test
I can generate a test repo that reproduces this issue if anyone requests it.
I tested this with on version 8.2 and 10.2 and verified the issue existed on both versions. (Currently, my team is on 8.0 where this issue doesn't happen. This issue has stopped us from upgrading for a very long time).
I verified after implementing this fix on 10.2, the memory leak no longer happens and data urls are no longer stored in the
pendingBrowserPreRequests
array.How has the user experience changed?
Nothing has changed!
PR Tasks
cypress-documentation
?type definitions
?