-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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: set explicit screen when spawning own xvfb #6199
Conversation
Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.
PR Review ChecklistIf any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'. User Experience
Functionality
Maintainability
Quality
Internal
|
@@ -9,6 +9,7 @@ const util = require('../util') | |||
|
|||
const xvfb = Promise.promisifyAll(new Xvfb({ | |||
timeout: 30000, // milliseconds | |||
xvfb_args: ['-screen', '0', '1280x1024x24'], // need to explicitly define screen otherwise electron will crash |
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.
I took this value from freedesktop/xorg-xserver but there has to be something rotten with ubuntu's xvfb, because it's mentioned in man as well as in code that -screen
is default to 1280x1024x24
🤔
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.
Actually ubuntu uses this packag bionic/xvfb which contains older version with:
#define VFB_DEFAULT_WIDTH 1280
#define VFB_DEFAULT_HEIGHT 1024
#define VFB_DEFAULT_DEPTH 8
So I thought that Electron is freaking out because of the bit depth, but when I start Xvbf with 8bits electron is still fine with that even without -screen
at all 🤷♂
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.
So latest development indicates that it could be problem somewhere else and this is just red herring:
[2031:0121/105922.460578:FATAL:memory.cc(22)] Out of memory. size=65536
We detected that the Chromium Renderer process just crashed.
This is the equivalent to seeing the 'sad face' when Chrome dies.
(...)
Not sure if related thou, because some of our tests pass. Definitely not related, it was /dev/shm set to 16MB
This reverts commit d26e037.
I have built |
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.
looks good and solves the problem from my testing
Added small unit test in 9ac5fe3 |
Awesome, thank you @bahmutov 🙇 |
User facing changelog
Fixes
cypress verify
crash on some systems caused by not defined-screen
.Additional details
cypress verify
crashes in a docker image with this environment:When I run it with
ELECTRON_ENABLE_STACK_DUMPING
I get this:Directly running
Xvfb
and invoking cypress binary from node works well:Even checked with
xwd
what is in the framebuffer and all fine.So I got suspicious Xvfb is not working and added
ps aux
before and after smoke test is executed and turned out that Xvfb is missing-screen
. Not sure if this should not be handled in@cypress/xvfb
, up to maintainers.Feel free to cancel if you already have fix elsewhere.
How has the user experience changed?
Before

After

PR Tasks